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 <[email protected]> > > 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.
>
>> !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?
Also, do I need ALL_AA_EXEC_UNSAFE here or would
(AA_EXEC_UNSAFE << AA_USER_SHIFT) be more appropriate?
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."));
>>
>>
>
signature.asc
Description: OpenPGP digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
