Date:   Sun, 11 Feb 2018 04:31:11 +0000
From:   Ben Hutchings <ben@...adent.org.uk>
To:     linux-kernel@...r.kernel.org, stable@...r.kernel.org
CC:     akpm@...ux-foundation.org,
        "John Johansen" <john.johansen@...onical.com>
Subject: [PATCH 3.16 101/136] apparmor: ensure that undecidable profile
 attachments fail

3.16.54-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: John Johansen <john.johansen@...onical.com>

commit 844b8292b6311ecd30ae63db1471edb26e01d895 upstream.

Profiles that have an undecidable overlap in their attachments are
being incorrectly handled. Instead of failing to attach the first one
encountered is being used.

eg.
  profile A /** { .. }
  profile B /*foo { .. }

have an unresolvable longest left attachment, they both have an exact
match on / and then have an overlapping expression that has no clear
winner.

Currently the winner will be the profile that is loaded first which
can result in non-deterministic behavior. Instead in this situation
the exec should fail.

Fixes: 898127c34ec0 ("AppArmor: functions for domain transitions")
Signed-off-by: John Johansen <john.johansen@...onical.com>
[bwh: Backported to 3.16:
 - Add 'info' parameter to x_to_profile(), done upstream in commit
   93c98a484c49 "apparmor: move exec domain mediation to using labels"
 - Adjust context]
Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
---
 security/apparmor/domain.c | 46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -126,6 +126,7 @@ static struct file_perms change_profile_
  * __attach_match_ - find an attachment match
  * @name - to match against  (NOT NULL)
  * @head - profile list to walk  (NOT NULL)
+ * @info - info message if there was an error (NOT NULL)
  *
  * Do a linear search on the profiles in the list.  There is a matching
  * preference where an exact match is preferred over a name which uses
@@ -137,28 +138,44 @@ static struct file_perms change_profile_
  * Returns: profile or NULL if no match found
  */
 static struct aa_profile *__attach_match(const char *name,
-					 struct list_head *head)
+					 struct list_head *head,
+					 const char **info)
 {
 	int len = 0;
+	bool conflict = false;
 	struct aa_profile *profile, *candidate = NULL;
 
 	list_for_each_entry_rcu(profile, head, base.list) {
 		if (profile->flags & PFLAG_NULL)
 			continue;
-		if (profile->xmatch && profile->xmatch_len > len) {
-			unsigned int state = aa_dfa_match(profile->xmatch,
-							  DFA_START, name);
-			u32 perm = dfa_user_allow(profile->xmatch, state);
-			/* any accepting state means a valid match. */
-			if (perm & MAY_EXEC) {
-				candidate = profile;
-				len = profile->xmatch_len;
+		if (profile->xmatch) {
+			if (profile->xmatch_len == len) {
+				conflict = true;
+				continue;
+			} else if (profile->xmatch_len > len) {
+				unsigned int state;
+				u32 perm;
+
+				state = aa_dfa_match(profile->xmatch,
+						     DFA_START, name);
+				perm = dfa_user_allow(profile->xmatch, state);
+				/* any accepting state means a valid match. */
+				if (perm & MAY_EXEC) {
+					candidate = profile;
+					len = profile->xmatch_len;
+					conflict = false;
+				}
 			}
 		} else if (!strcmp(profile->base.name, name))
 			/* exact non-re match, no more searching required */
 			return profile;
 	}
 
+	if (conflict) {
+		*info = "conflicting profile attachments";
+		return NULL;
+	}
+
 	return candidate;
 }
 
@@ -167,16 +184,18 @@ static struct aa_profile *__attach_match
  * @ns: the current namespace  (NOT NULL)
  * @list: list to search  (NOT NULL)
  * @name: the executable name to match against  (NOT NULL)
+ * @info: info message if there was an error
  *
  * Returns: profile or NULL if no match found
  */
 static struct aa_profile *find_attach(struct aa_namespace *ns,
-				      struct list_head *list, const char *name)
+				      struct list_head *list, const char *name,
+				      const char **info)
 {
 	struct aa_profile *profile;
 
 	rcu_read_lock();
-	profile = aa_get_profile(__attach_match(name, list));
+	profile = aa_get_profile(__attach_match(name, list, info));
 	rcu_read_unlock();
 
 	return profile;
@@ -298,7 +317,8 @@ static struct aa_profile *x_table_lookup
  * Returns: refcounted profile or NULL if not found available
  */
 static struct aa_profile *x_to_profile(struct aa_profile *profile,
-				       const char *name, u32 xindex)
+				       const char *name, u32 xindex,
+				       const char **info)
 {
 	struct aa_profile *new_profile = NULL;
 	struct aa_namespace *ns = profile->ns;
@@ -312,11 +332,11 @@ static struct aa_profile *x_to_profile(s
 		if (xindex & AA_X_CHILD)
 			/* released by caller */
 			new_profile = find_attach(ns, &profile->base.profiles,
-						  name);
+						  name, info);
 		else
 			/* released by caller */
 			new_profile = find_attach(ns, &ns->base.profiles,
-						  name);
+						  name, info);
 		break;
 	case AA_X_TABLE:
 		/* released by caller */
@@ -385,7 +405,8 @@ int apparmor_bprm_set_creds(struct linux
 			/* change_profile on exec already been granted */
 			new_profile = aa_get_profile(cxt->onexec);
 		else
-			new_profile = find_attach(ns, &ns->base.profiles, name);
+			new_profile = find_attach(ns, &ns->base.profiles, name,
+						  &info);
 		if (!new_profile)
 			goto cleanup;
 		/*
@@ -421,7 +442,7 @@ int apparmor_bprm_set_creds(struct linux
 
 	if (perms.allow & MAY_EXEC) {
 		/* exec permission determine how to transition */
-		new_profile = x_to_profile(profile, name, perms.xindex);
+		new_profile = x_to_profile(profile, name, perms.xindex, &info);
 		if (!new_profile) {
 			if (perms.xindex & AA_X_INHERIT) {
 				/* (p|c|n)ix - don't change profile but do

Powered by blists - more mailing lists