[apparmor] [PATCH v1.3 05/11] parser: Allow change_profile rules to accept an exec mode modifier
John Johansen
john.johansen at canonical.com
Tue May 31 20:31:46 UTC 2016
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 <tyhicks at canonical.com>
Acked-by: John Johansen <john.johansen at canonical.com>
> ---
>
> * 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."));
>
>
More information about the AppArmor
mailing list