[apparmor] [PATCH v1.1 05/11] parser: Allow change_profile rules to accept an exec mode modifier

Tyler Hicks tyhicks at canonical.com
Sat May 28 16:42:11 UTC 2016


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

* Changes since v1:
  - parser_lex.l: Remove CHANGE_PROFILE_TARGET_MODE, added by v1 of this
    patch, in favor of using the existing SUB_ID rule to handle the
    change_profile transition target.
  - parser_lexl: Add SUB_ID to the list of rules that eats whitespace.
    Without this change, the parser would reject change_profile rules the
    one found in
    parser/tst/simple_tests//vars/vars_simple_assignment_13.sd
  - parser_regex.c: Create a new helper function,
    is_change_profile_mode(), and clearly document why
    (mode & ~AA_CHANGE_PROFILE) is not safe to use.
  - parser_regex.c: Revert the change made in v1 of this patch to identify
    modes that do not correspond to link rules.


 parser/parser_lex.l   | 13 +++++++++++--
 parser/parser_regex.c | 44 +++++++++++++++++++++++++++++++++++++-------
 parser/parser_yacc.y  | 28 ++++++++++++++++++++++++----
 3 files changed, 72 insertions(+), 13 deletions(-)

diff --git a/parser/parser_lex.l b/parser/parser_lex.l
index 49b1f22..cff8a7b 100644
--- a/parser/parser_lex.l
+++ b/parser/parser_lex.l
@@ -268,7 +268,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,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 */ }
 }
 
@@ -439,7 +439,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, TOK_ARROW);
+	}
 
 	({IDS}|{QUOTED_ID}) {
 		yylval.id = processid(yytext, yyleng);
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."));
 
-- 
2.7.4




More information about the AppArmor mailing list