On 05/31/2016 01:17 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]>
Acked-by: John Johansen <[email protected]> > --- > > * Changes from v1.2: > - Create a SUB_ID_WS mode that eats whitespace and have > CHANGE_PROFILE_MODE push state their whenever it encounters an ARROW > - Drop the optional trailing {WS} and \n match following an ARROW in > CHANGE_PROFILE_MODE > > parser/parser_lex.l | 25 ++++++++++++++++++++++--- > parser/parser_regex.c | 44 +++++++++++++++++++++++++++++++++++++------- > parser/parser_yacc.y | 28 ++++++++++++++++++++++++---- > 3 files changed, 83 insertions(+), 14 deletions(-) > > diff --git a/parser/parser_lex.l b/parser/parser_lex.l > index 49b1f22..a59daa6 100644 > --- a/parser/parser_lex.l > +++ b/parser/parser_lex.l > @@ -239,6 +239,7 @@ LT_EQUAL <= > > /* IF adding new state please update state_names table at eof */ > %x SUB_ID > +%x SUB_ID_WS > %x SUB_VALUE > %x EXTCOND_MODE > %x EXTCONDLIST_MODE > @@ -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,SUB_ID_WS,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>{ > {WS}+ { DUMP_PREPROCESS; /* Ignoring whitespace */ } > } > > @@ -329,6 +330,14 @@ LT_EQUAL <= > } > } > > +<SUB_ID_WS>{ > + ({IDS}|{QUOTED_ID}) { > + /* Go into separate state to match generic ID strings */ > + yylval.id = processid(yytext, yyleng); > + POP_AND_RETURN(TOK_ID); > + } > +} > + > <SUB_VALUE>{ > ({IDS}|{QUOTED_ID}) { > /* Go into separate state to match generic VALUE strings */ > @@ -439,7 +448,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(SUB_ID_WS, TOK_ARROW); > + } > > ({IDS}|{QUOTED_ID}) { > yylval.id = processid(yytext, yyleng); > @@ -632,7 +650,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_ID_WS,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>{ > [^\n] { > DUMP_PREPROCESS; > /* Something we didn't expect */ > @@ -647,6 +665,7 @@ include/{WS} { > unordered_map<int, string> state_names = { > STATE_TABLE_ENT(INITIAL), > STATE_TABLE_ENT(SUB_ID), > + STATE_TABLE_ENT(SUB_ID_WS), > STATE_TABLE_ENT(SUB_VALUE), > STATE_TABLE_ENT(EXTCOND_MODE), > STATE_TABLE_ENT(EXTCONDLIST_MODE), > diff --git a/parser/parser_regex.c b/parser/parser_regex.c > index 8cc08c6..bdc3a58 100644 > --- a/parser/parser_regex.c > +++ b/parser/parser_regex.c > @@ -494,6 +494,23 @@ static int process_profile_name_xmatch(Profile *prof) > > static int warn_change_profile = 1; > > +static bool is_change_profile_mode(int mode) > +{ > + /** > + * A change_profile entry will have the AA_CHANGE_PROFILE bit set. > + * It could also have the (AA_EXEC_BITS | ALL_AA_EXEC_UNSAFE) bits > + * set by the frontend parser. That means that it is incorrect to > + * identify change_profile modes using a test like this: > + * > + * (mode & ~AA_CHANGE_PROFILE) > + * > + * The above test would incorrectly return true on a > + * change_profile mode that has the > + * (AA_EXEC_BITS | ALL_AA_EXEC_UNSAFE) bits set. > + */ > + return mode & AA_CHANGE_PROFILE; > +} > + > static int process_dfa_entry(aare_rules *dfarules, struct cod_entry *entry) > { > std::string tbuf; > @@ -504,7 +521,7 @@ static int process_dfa_entry(aare_rules *dfarules, struct > cod_entry *entry) > return TRUE; > > > - if (entry->mode & ~AA_CHANGE_PROFILE) > + if (!is_change_profile_mode(entry->mode)) > filter_slashes(entry->name); > ptype = convert_aaregex_to_pcre(entry->name, 0, glob_default, tbuf, > &pos); > if (ptype == ePatternInvalid) > @@ -530,13 +547,14 @@ 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) && > + !is_change_profile_mode(entry->mode) && > !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 (!is_change_profile_mode(entry->mode)) { > if (!dfarules->add_rule(tbuf.c_str(), entry->deny, entry->mode, > entry->audit, dfaflags)) > return FALSE; > @@ -563,12 +581,13 @@ static int process_dfa_entry(aare_rules *dfarules, > struct cod_entry *entry) > if (!dfarules->add_rule_vec(entry->deny, perms, entry->audit & > AA_LINK_BITS, 2, vec, dfaflags)) > return FALSE; > } > - if (entry->mode & AA_CHANGE_PROFILE) { > + if (is_change_profile_mode(entry->mode)) { > const char *vec[3]; > std::string lbuf, xbuf; > 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 +629,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)); > + 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); > + } 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.")); > > -- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
