[PATCH V2] security/apparmor: fix matching on presence of extended attributes

[PATCH V2] security/apparmor: fix matching on presence of extended attributes

[Eric Chiang]

AppArmor recently added the ability for profiles to match extended
attributes, with the intent of targeting "security.ima" and
"security.evm" to differentiate between sign and unsigned files.

The current implementation uses a path glob to match the extended
attribute value. To require the presence of a extended attribute,
profiles supply a wildcard:

    # Match any file with the "security.apparmor" attribute
    profile test /** xattrs=(security.apparmor="**") {
        # ...
    }

However, the glob matching implementation is intended for file paths and
doesn't handle null characters correctly. It's currently impossible to
write a profile that targets IMA and EVM attributes, since the
signatures can contain a null byte.

Add the ability for AppArmor to check the presence of an extended
attribute, and not its value. This fixes the profile matching allowing
profiles conditional on EVM and IMA signatures:

    profile signed_binary /** xattrs=(security.evm security.ima) {
        # ...
    }

A modified apparmor_parser and associated regression tests to exercise
this fix can be found at:

    https://gitlab.com/ericchiang/apparmor/commits/parser-xattrs-keys

Signed-off-by: Eric Chiang <ericchiang@google.com>
CC: stable@vger.kernel.org
---
 security/apparmor/apparmorfs.c     |  1 +
 security/apparmor/domain.c         | 25 +++++++++++++++++++++----
 security/apparmor/include/policy.h |  6 ++++++
 security/apparmor/policy.c         |  3 +++
 security/apparmor/policy_unpack.c  | 18 ++++++++++++++++++
 5 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 8963203319ea..03d9b7f8a2fb 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -2212,6 +2212,7 @@ static struct aa_sfs_entry aa_sfs_entry_signal[] = {
 
 static struct aa_sfs_entry aa_sfs_entry_attach[] = {
 	AA_SFS_FILE_BOOLEAN("xattr", 1),
+	AA_SFS_FILE_BOOLEAN("xattr_key", 1),
 	{ }
 };
 static struct aa_sfs_entry aa_sfs_entry_domain[] = {
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 08c88de0ffda..9f223756b416 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -317,16 +317,33 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
 	ssize_t size;
 	struct dentry *d;
 	char *value = NULL;
-	int value_size = 0, ret = profile->xattr_count;
+	int value_size = 0;
+	int ret = profile->xattr_count + profile->xattr_keys_count;
 
-	if (!bprm || !profile->xattr_count)
+	if (!bprm)
 		return 0;
 
+	d = bprm->file->f_path.dentry;
+
+	if (profile->xattr_keys_count) {
+		/* validate that these attributes are present, ignore values */
+		for (i = 0; i < profile->xattr_keys_count; i++) {
+			size = vfs_getxattr_alloc(d, profile->xattr_keys[i],
+						  &value, value_size,
+						  GFP_KERNEL);
+			if (size < 0) {
+				ret = -EINVAL;
+				goto out;
+			}
+		}
+	}
+
+	if (!profile->xattr_count)
+		goto out;
+
 	/* 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);
diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
index 8e6707c837be..8ed1d30de7ce 100644
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -112,6 +112,10 @@ struct aa_data {
  * @policy: general match rules governing policy
  * @file: The set of rules governing basic file access and domain transitions
  * @caps: capabilities for the profile
+ * @xattr_count: number of xattrs values
+ * @xattrs: extended attributes whose values must match the xmatch
+ * @xattr_keys_count: number of xattr keys values
+ * @xattr_keys: extended attributes that must be present to match the profile
  * @rlimits: rlimits for the profile
  *
  * @dents: dentries for the profiles file entries in apparmorfs
@@ -152,6 +156,8 @@ struct aa_profile {
 
 	int xattr_count;
 	char **xattrs;
+	int xattr_keys_count;
+	char **xattr_keys;
 
 	struct aa_rlimit rlimits;
 
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index df9c5890a878..e0f9cf8b8318 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -231,6 +231,9 @@ void aa_free_profile(struct aa_profile *profile)
 	for (i = 0; i < profile->xattr_count; i++)
 		kzfree(profile->xattrs[i]);
 	kzfree(profile->xattrs);
+	for (i = 0; i < profile->xattr_keys_count; i++)
+		kzfree(profile->xattr_keys[i]);
+	kzfree(profile->xattr_keys);
 	for (i = 0; i < profile->secmark_count; i++)
 		kzfree(profile->secmark[i].label);
 	kzfree(profile->secmark);
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 379682e2a8d5..d1fd75093260 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -535,6 +535,24 @@ static bool unpack_xattrs(struct aa_ext *e, struct aa_profile *profile)
 			goto fail;
 	}
 
+	if (unpack_nameX(e, AA_STRUCT, "xattr_keys")) {
+		int i, size;
+
+		size = unpack_array(e, NULL);
+		profile->xattr_keys_count = size;
+		profile->xattr_keys = kcalloc(size, sizeof(char *), GFP_KERNEL);
+		if (!profile->xattr_keys)
+			goto fail;
+		for (i = 0; i < size; i++) {
+			if (!unpack_strdup(e, &profile->xattr_keys[i], NULL))
+				goto fail;
+		}
+		if (!unpack_nameX(e, AA_ARRAYEND, NULL))
+			goto fail;
+		if (!unpack_nameX(e, AA_STRUCTEND, NULL))
+			goto fail;
+	}
+
 	return 1;
 
 fail:
-- 
2.20.1.415.g653613c723-goog