[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 12:16:47 UTC 2016


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);

> +	}
>  
>  	({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

>  		    !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

> +		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."));
>  
> 




More information about the AppArmor mailing list