[apparmor] [PATCH 05/11] parser: Allow change_profile rules to accept an exec mode modifier
John Johansen
john.johansen at canonical.com
Fri May 27 20:37:23 UTC 2016
On 05/27/2016 08:28 AM, Tyler Hicks wrote:
> On 05/27/2016 07:16 AM, John Johansen wrote:
>> On 05/25/2016 01:59 PM, Tyler Hicks wrote:
>>> https://launchpad.net/bugs/1584069
>>>
>>> This patch allows policy authors to specify how exec transitions should
>>> be handled with respect to setting AT_SECURE in the new process'
>>> auxiliary vector and, ultimately, having libc scrub (or not scrub) the
>>> environment.
>>>
>>> An exec mode of 'safe' means that the environment will be scrubbed and
>>> this is the default in kernels that support AppArmor profile stacking.
>>> An exec mode of 'unsafe' means that the environment will not be scrubbed
>>> and this is the default and only supported change_profile exec mode in
>>> kernels that do not support AppArmor profile stacking.
>>>
>>> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
>>
>> See below
>>
>>> ---
>>> parser/parser_lex.l | 26 ++++++++++++++++++++++----
>>> parser/parser_regex.c | 22 +++++++++++++++++-----
>>> parser/parser_yacc.y | 28 ++++++++++++++++++++++++----
>>> 3 files changed, 63 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/parser/parser_lex.l b/parser/parser_lex.l
>>> index 49b1f22..a49c7dd 100644
>>> --- a/parser/parser_lex.l
>>> +++ b/parser/parser_lex.l
>>> @@ -255,6 +255,7 @@ LT_EQUAL <=
>>> %x PTRACE_MODE
>>> %x UNIX_MODE
>>> %x CHANGE_PROFILE_MODE
>>> +%x CHANGE_PROFILE_TARGET_MODE
>>
>> not needed you can use SUB_ID
>>
>>> %x INCLUDE
>>>
>>> %%
>>> @@ -268,7 +269,7 @@ LT_EQUAL <=
>>> }
>>> %}
>>>
>>> -<INITIAL,INCLUDE,LIST_VAL_MODE,EXTCOND_MODE,LIST_COND_VAL,LIST_COND_PAREN_VAL,LIST_COND_MODE,EXTCONDLIST_MODE,ASSIGN_MODE,NETWORK_MODE,CHANGE_PROFILE_MODE,RLIMIT_MODE,MOUNT_MODE,DBUS_MODE,SIGNAL_MODE,PTRACE_MODE,UNIX_MODE>{
>>> +<INITIAL,INCLUDE,LIST_VAL_MODE,EXTCOND_MODE,LIST_COND_VAL,LIST_COND_PAREN_VAL,LIST_COND_MODE,EXTCONDLIST_MODE,ASSIGN_MODE,NETWORK_MODE,CHANGE_PROFILE_MODE,CHANGE_PROFILE_TARGET_MODE,RLIMIT_MODE,MOUNT_MODE,DBUS_MODE,SIGNAL_MODE,PTRACE_MODE,UNIX_MODE>{
>>> {WS}+ { DUMP_PREPROCESS; /* Ignoring whitespace */ }
>>> }
>>>
>>> @@ -439,7 +440,16 @@ LT_EQUAL <=
>>> }
>>>
>>> <CHANGE_PROFILE_MODE>{
>>> - {ARROW} { RETURN_TOKEN(TOK_ARROW); }
>>> + safe { RETURN_TOKEN(TOK_SAFE); }
>>> + unsafe { RETURN_TOKEN(TOK_UNSAFE); }
>>> +
>>> + {ARROW} {
>>> + /**
>>> + * Push state so that we can return TOK_ID even when the
>>> + * change_profile target is 'safe' or 'unsafe'.
>>> + */
>>> + PUSH_AND_RETURN(CHANGE_PROFILE_TARGET_MODE, TOK_ARROW);
>>
>> PUSH_AND_RETURN(SUB_ID, TOK_ARROW);
>
> Aha! Thanks - this makes the patch quite a bit smaller.
>
>>
>>> + }
>>>
>>> ({IDS}|{QUOTED_ID}) {
>>> yylval.id = processid(yytext, yyleng);
>>> @@ -447,6 +457,13 @@ LT_EQUAL <=
>>> }
>>> }
>>>
>>> +<CHANGE_PROFILE_TARGET_MODE>{
>>> + ({IDS}|{QUOTED_ID}) {
>>> + yylval.id = processid(yytext, yyleng);
>>> + POP_AND_RETURN(TOK_ID);
>>> + }
>>> +}
>>> +
>>
>> again not needed
>>
>>> <RLIMIT_MODE>{
>>> -?{NUMBER} {
>>> yylval.var_val = strdup(yytext);
>>> @@ -619,7 +636,7 @@ include/{WS} {
>>> PUSH_AND_RETURN(state, token);
>>> }
>>>
>>> -<INITIAL,NETWORK_MODE,RLIMIT_MODE,CHANGE_PROFILE_MODE,MOUNT_MODE,DBUS_MODE,SIGNAL_MODE,PTRACE_MODE,UNIX_MODE>{
>>> +<INITIAL,NETWORK_MODE,RLIMIT_MODE,CHANGE_PROFILE_MODE,CHANGE_PROFILE_TARGET_MODE,MOUNT_MODE,DBUS_MODE,SIGNAL_MODE,PTRACE_MODE,UNIX_MODE>{
>>
>> dito
>>
>>> {END_OF_RULE} {
>>> if (YY_START != INITIAL)
>>> POP_NODUMP();
>>> @@ -632,7 +649,7 @@ include/{WS} {
>>> }
>>> }
>>>
>>> -<INITIAL,SUB_ID,SUB_VALUE,LIST_VAL_MODE,EXTCOND_MODE,LIST_COND_VAL,LIST_COND_PAREN_VAL,LIST_COND_MODE,EXTCONDLIST_MODE,ASSIGN_MODE,NETWORK_MODE,CHANGE_PROFILE_MODE,MOUNT_MODE,DBUS_MODE,SIGNAL_MODE,PTRACE_MODE,UNIX_MODE>{
>>> +<INITIAL,SUB_ID,SUB_VALUE,LIST_VAL_MODE,EXTCOND_MODE,LIST_COND_VAL,LIST_COND_PAREN_VAL,LIST_COND_MODE,EXTCONDLIST_MODE,ASSIGN_MODE,NETWORK_MODE,CHANGE_PROFILE_MODE,CHANGE_PROFILE_TARGET_MODE,MOUNT_MODE,DBUS_MODE,SIGNAL_MODE,PTRACE_MODE,UNIX_MODE>{
>>
>> dito
>>
>>> [^\n] {
>>> DUMP_PREPROCESS;
>>> /* Something we didn't expect */
>>> @@ -663,5 +680,6 @@ unordered_map<int, string> state_names = {
>>> STATE_TABLE_ENT(PTRACE_MODE),
>>> STATE_TABLE_ENT(UNIX_MODE),
>>> STATE_TABLE_ENT(CHANGE_PROFILE_MODE),
>>> + STATE_TABLE_ENT(CHANGE_PROFILE_TARGET_MODE),
>>
>> dito
>>
>>> STATE_TABLE_ENT(INCLUDE),
>>> };
>>> diff --git a/parser/parser_regex.c b/parser/parser_regex.c
>>> index 8cc08c6..b8bf7fb 100644
>>> --- a/parser/parser_regex.c
>>> +++ b/parser/parser_regex.c
>>> @@ -530,13 +530,13 @@ static int process_dfa_entry(aare_rules *dfarules, struct cod_entry *entry)
>>> * TODO: split link and change_profile entries earlier
>>> */
>>> if (entry->deny) {
>>> - if ((entry->mode & ~(AA_LINK_BITS | AA_CHANGE_PROFILE)) &&
>>> + if (!(entry->mode & (AA_LINK_BITS | AA_CHANGE_PROFILE)) &&
>> No this is potentially wrong because link bits can show up in file rules (/foo rwl), there needs to
>> be some refactoring up front to generate 2 rules for these
>
> Judging by your next comment, I think this could work:
>
> if ((entry->mode & ~AA_LINK_BITS) &&
> !(entry->mode & AA_CHANGE_PROFILE) &&
> ...) {
> ...
> }
>
> It is kind of sloppy and I'd want to clearly document what's going on.
> What do you think? Is that acceptable or do I need to do the refactoring
> up front? I don't fully understand what's needed there because I haven't
> looked much into the parsing/encoding of file or link rules.
>
Yes that should work but I vote for just leaving atm.
I looked at doing a simple refactor up front and it is a mess. yacc wants to
return a single element, but we need to return two, so we need to return
a list, but then we need to rework how the elements get added, ...
it is doable but I would throw that all away with the hybrid RD work where
we get to pass our block state down to the rules so they don't have to
be passed back up. blah, blah, blah ...
Basically I have some work in progress to clean some of this up so I don't
thin its worth doing what is needed under the current code.
>>
>>> !dfarules->add_rule(tbuf.c_str(), entry->deny,
>>> entry->mode & ~(AA_LINK_BITS | AA_CHANGE_PROFILE),
>>> entry->audit & ~(AA_LINK_BITS | AA_CHANGE_PROFILE),
>>> dfaflags))
>>> return FALSE;
>>> - } else if (entry->mode & ~AA_CHANGE_PROFILE) {
>>> + } else if (!(entry->mode & AA_CHANGE_PROFILE)) {
>> this should be okay because change_profile is completely separated up front.
>>
>>> if (!dfarules->add_rule(tbuf.c_str(), entry->deny, entry->mode,
>>> entry->audit, dfaflags))
>>> return FALSE;
>>> @@ -569,6 +569,7 @@ static int process_dfa_entry(aare_rules *dfarules, struct cod_entry *entry)
>>> autofree char *ns = NULL;
>>> autofree char *name = NULL;
>>> int index = 1;
>>> + uint32_t onexec_perms = AA_ONEXEC;
>>>
>>> if ((warnflags & WARN_RULE_DOWNGRADED) && entry->audit && warn_change_profile) {
>>> /* don't have profile name here, so until this code
>>> @@ -610,12 +611,23 @@ static int process_dfa_entry(aare_rules *dfarules, struct cod_entry *entry)
>>> }
>>>
>>> /* regular change_profile rule */
>>> - if (!dfarules->add_rule_vec(entry->deny, AA_CHANGE_PROFILE | AA_ONEXEC, 0, index - 1, &vec[1], dfaflags))
>>> + if (!dfarules->add_rule_vec(entry->deny,
>>> + AA_CHANGE_PROFILE | onexec_perms,
>>> + 0, index - 1, &vec[1], dfaflags))
>>> return FALSE;
>>> +
>>> /* onexec rules - both rules are needed for onexec */
>>> - if (!dfarules->add_rule_vec(entry->deny, AA_ONEXEC, 0, 1, vec, dfaflags))
>>> + if (!dfarules->add_rule_vec(entry->deny, onexec_perms,
>>> + 0, 1, vec, dfaflags))
>>> return FALSE;
>>> - if (!dfarules->add_rule_vec(entry->deny, AA_ONEXEC, 0, index, vec, dfaflags))
>>> +
>>> + /**
>>> + * pick up any exec bits, from the frontend parser, related to
>>> + * unsafe exec transitions
>>> + */
>>> + onexec_perms |= (entry->mode & (AA_EXEC_BITS | ALL_AA_EXEC_UNSAFE));
>>
>> AA_EXEC_BITS shouldn't be needed but it doesn't hurt leaving it in
>
> I'd rather not set bits that aren't needed but accept_perms() strips out
> the ALL_AA_EXEC_UNSAFE bits unless AA_EXEC_BITS are set. Should I modify
> accept_perms() to handle AA_CHANGE_PROFILE rules slightly different?
>
hrmmm right.
modifying accept_perms() is a possibility, I'm not particularly thrilled
with including the x bits here, but it shouldn't conflict with anything
(there is a bug if it does) and I'm not sure it worth modifying
accept_perms() to special case this.
> Also, do I need ALL_AA_EXEC_UNSAFE here or would
> (AA_EXEC_UNSAFE << AA_USER_SHIFT) be more appropriate?
>
because of how the kernel is picking the unsafe up you need
ALL_AA_EXEC_UNSAFE
> Thanks for your reviews and help!
>
> Tyler
>
>>
>>> + if (!dfarules->add_rule_vec(entry->deny, onexec_perms,
>>> + 0, index, vec, dfaflags))
>>> return FALSE;
>>> }
>>> return TRUE;
>>> diff --git a/parser/parser_yacc.y b/parser/parser_yacc.y
>>> index 91c6d68..bb40f09 100644
>>> --- a/parser/parser_yacc.y
>>> +++ b/parser/parser_yacc.y
>>> @@ -1474,11 +1474,31 @@ file_mode: TOK_MODE
>>> free($1);
>>> }
>>>
>>> -change_profile: TOK_CHANGE_PROFILE opt_id opt_named_transition TOK_END_OF_RULE
>>> +change_profile: TOK_CHANGE_PROFILE opt_unsafe opt_id opt_named_transition TOK_END_OF_RULE
>>> {
>>> struct cod_entry *entry;
>>> - char *exec = $2;
>>> - char *target = $3;
>>> + int mode = AA_CHANGE_PROFILE;
>>> + int exec_mode = $2;
>>> + char *exec = $3;
>>> + char *target = $4;
>>> +
>>> + if (exec_mode) {
>>> + if (!exec)
>>> + yyerror(_("Exec condition is required when unsafe or safe keywords are present"));
>>> +
>>> + if (exec_mode == 1) {
>>> + mode |= (AA_EXEC_BITS | ALL_AA_EXEC_UNSAFE);
>>
>> dito
>>
>>> + } else if (exec_mode == 2 &&
>>> + !kernel_supports_stacking &&
>>> + warnflags & WARN_RULE_DOWNGRADED) {
>>> + pwarn("downgrading change_profile safe rule to unsafe due to lack of necessary kernel support\n");
>>> + /**
>>> + * No need to do anything because the 'unsafe'
>>> + * variant is the only supported type of
>>> + * change_profile rules in non-stacking kernels
>>> + */
>>> + }
>>> + }
>>>
>>> if (exec && !(exec[0] == '/' || strncmp(exec, "@{", 2) == 0))
>>> yyerror(_("Exec condition must begin with '/'."));
>>> @@ -1492,7 +1512,7 @@ change_profile: TOK_CHANGE_PROFILE opt_id opt_named_transition TOK_END_OF_RULE
>>> yyerror(_("Memory allocation error."));
>>> }
>>>
>>> - entry = new_entry(target, AA_CHANGE_PROFILE, exec);
>>> + entry = new_entry(target, mode, exec);
>>> if (!entry)
>>> yyerror(_("Memory allocation error."));
>>>
>>>
>>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160527/28f031ac/attachment.pgp>
More information about the AppArmor
mailing list