apparmor: convert attaching profiles via xattrs to use dfa matching

This converts profile attachment based on xattrs to a fixed extended
conditional using dfa matching.

This has a couple of advantages
- pattern matching can be used for the xattr match

- xattrs can be optional for an attachment or marked as required

- the xattr attachment conditional will be able to be combined with
  other extended conditionals when the flexible extended conditional
  work lands.

The xattr fixed extended conditional is appended to the xmatch
conditional. If an xattr attachment is specified the profile xmatch
will be generated regardless of whether there is a pattern match on
the executable name.

Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>

5 files changed, 43 insertions, 57 deletions

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 1e63ff2e5b85..35e6b240fb14 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -2147,6 +2147,10 @@ static struct aa_sfs_entry aa_sfs_entry_signal[] = {
 	{ }
 };
 
+static struct aa_sfs_entry aa_sfs_entry_attach[] = {
+	AA_SFS_FILE_BOOLEAN("xattr", 1),
+	{ }
+};
 static struct aa_sfs_entry aa_sfs_entry_domain[] = {
 	AA_SFS_FILE_BOOLEAN("change_hat",	1),
 	AA_SFS_FILE_BOOLEAN("change_hatv",	1),
@@ -2155,6 +2159,7 @@ static struct aa_sfs_entry aa_sfs_entry_domain[] = {
 	AA_SFS_FILE_BOOLEAN("stack",		1),
 	AA_SFS_FILE_BOOLEAN("fix_binfmt_elf_mmap",	1),
 	AA_SFS_FILE_BOOLEAN("post_nnp_subset",	1),
+	AA_SFS_DIR("attach_conditions",		aa_sfs_entry_attach),
 	AA_SFS_FILE_STRING("version", "1.2"),
 	{ }
 };
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 6bcafe8d226d..6a1279f11fcc 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -306,11 +306,12 @@ static int change_profile_perms(struct aa_profile *profile,
  * aa_xattrs_match - check whether a file matches the xattrs defined in profile
  * @bprm: binprm struct for the process to validate
  * @profile: profile to match against (NOT NULL)
+ * @state: state to start match in
  *
  * Returns: number of extended attributes that matched, or < 0 on error
  */
 static int aa_xattrs_match(const struct linux_binprm *bprm,
-			   struct aa_profile *profile)
+			   struct aa_profile *profile, unsigned int state)
 {
 	int i;
 	size_t size;
@@ -321,27 +322,40 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
 	if (!bprm || !profile->xattr_count)
 		return 0;
 
+	/* transition from exec match to xattr set */
+	state = aa_dfa_null_transition(profile->xmatch, state);
+
 	d = bprm->file->f_path.dentry;
 
 	for (i = 0; i < profile->xattr_count; i++) {
 		size = vfs_getxattr_alloc(d, profile->xattrs[i], &value,
 					  value_size, GFP_KERNEL);
-		if (size < 0) {
-			ret = -EINVAL;
-			goto out;
-		}
+		if (size >= 0) {
+			u32 perm;
 
-		/* Check the xattr value, not just presence */
-		if (profile->xattr_lens[i]) {
-			if (profile->xattr_lens[i] != size) {
+			/* Check the xattr value, not just presence */
+			state = aa_dfa_match_len(profile->xmatch, state, value,
+						 size);
+			perm = dfa_user_allow(profile->xmatch, state);
+			if (!(perm & MAY_EXEC)) {
 				ret = -EINVAL;
 				goto out;
 			}
-
-			if (memcmp(value, profile->xattr_values[i], size)) {
+		}
+		/* transition to next element */
+		state = aa_dfa_null_transition(profile->xmatch, state);
+		if (size < 0) {
+			/*
+			 * No xattr match, so verify if transition to
+			 * next element was valid. IFF so the xattr
+			 * was optional.
+			 */
+			if (!state) {
 				ret = -EINVAL;
 				goto out;
 			}
+			/* don't count missing optional xattr as matched */
+			ret--;
 		}
 	}
 
@@ -403,13 +417,16 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
 			perm = dfa_user_allow(profile->xmatch, state);
 			/* any accepting state means a valid match. */
 			if (perm & MAY_EXEC) {
-				int ret = aa_xattrs_match(bprm, profile);
+				int ret = aa_xattrs_match(bprm, profile, state);
 
 				/* Fail matching if the xattrs don't match */
 				if (ret < 0)
 					continue;
 
-				/* The new match isn't more specific
+				/*
+				 * TODO: allow for more flexible best match
+				 *
+				 * The new match isn't more specific
 				 * than the current best match
 				 */
 				if (profile->xmatch_len == len &&
@@ -428,9 +445,11 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
 				xattrs = ret;
 				conflict = false;
 			}
-		} else if (!strcmp(profile->base.name, name) &&
-			   aa_xattrs_match(bprm, profile) >= 0)
-			/* exact non-re match, no more searching required */
+		} else if (!strcmp(profile->base.name, name))
+			/*
+			 * old exact non-re match, without conditionals such
+			 * as xattrs. no more searching required
+			 */
 			return profile;
 	}
 
@@ -652,7 +671,8 @@ static struct aa_label *profile_transition(struct aa_profile *profile,
 			 * met, and fail execution otherwise
 			 */
 			label_for_each(i, new, component) {
-				if (aa_xattrs_match(bprm, component) < 0) {
+				if (aa_xattrs_match(bprm, component, state) <
+				    0) {
 					error = -EACCES;
 					info = "required xattrs not present";
 					perms.allow &= ~MAY_EXEC;
diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
index 02bde92ebb5c..c93b9ed55490 100644
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -151,8 +151,6 @@ struct aa_profile {
 
 	int xattr_count;
 	char **xattrs;
-	size_t *xattr_lens;
-	char **xattr_values;
 
 	struct aa_rlimit rlimits;
 
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index 7fee546ba10d..c07493ce2376 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -228,13 +228,9 @@ void aa_free_profile(struct aa_profile *profile)
 	aa_free_cap_rules(&profile->caps);
 	aa_free_rlimit_rules(&profile->rlimits);
 
-	for (i = 0; i < profile->xattr_count; i++) {
+	for (i = 0; i < profile->xattr_count; i++)
 		kzfree(profile->xattrs[i]);
-		kzfree(profile->xattr_values[i]);
-	}
 	kzfree(profile->xattrs);
-	kzfree(profile->xattr_lens);
-	kzfree(profile->xattr_values);
 	kzfree(profile->dirname);
 	aa_put_dfa(profile->xmatch);
 	aa_put_dfa(profile->policy.dfa);
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 98d019185e57..8a31ddd474d7 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -540,8 +540,7 @@ static bool unpack_xattrs(struct aa_ext *e, struct aa_profile *profile)
 
 		size = unpack_array(e, NULL);
 		profile->xattr_count = size;
-		profile->xattrs = kcalloc(size, sizeof(char *),
-						GFP_KERNEL);
+		profile->xattrs = kcalloc(size, sizeof(char *), GFP_KERNEL);
 		if (!profile->xattrs)
 			goto fail;
 		for (i = 0; i < size; i++) {
@@ -554,38 +553,6 @@ static bool unpack_xattrs(struct aa_ext *e, struct aa_profile *profile)
 			goto fail;
 	}
 
-	if (unpack_nameX(e, AA_STRUCT, "xattr_values")) {
-		int i, size;
-
-		size = unpack_array(e, NULL);
-
-		/* Must be the same number of xattr values as xattrs */
-		if (size != profile->xattr_count)
-			goto fail;
-
-		profile->xattr_lens = kcalloc(size, sizeof(size_t),
-						    GFP_KERNEL);
-		if (!profile->xattr_lens)
-			goto fail;
-
-		profile->xattr_values = kcalloc(size, sizeof(char *),
-						      GFP_KERNEL);
-		if (!profile->xattr_values)
-			goto fail;
-
-		for (i = 0; i < size; i++) {
-			profile->xattr_lens[i] = unpack_blob(e,
-					      &profile->xattr_values[i], NULL);
-			profile->xattr_values[i] =
-				kvmemdup(profile->xattr_values[i],
-					 profile->xattr_lens[i]);
-		}
-
-		if (!unpack_nameX(e, AA_ARRAYEND, NULL))
-			goto fail;
-		if (!unpack_nameX(e, AA_STRUCTEND, NULL))
-			goto fail;
-	}
 	return 1;
 
 fail: