[PATCH v2] apparmor: make unpacked rename and attach strings owned copies

[PATCH v2] apparmor: make unpacked rename and attach strings owned copies

[Qihang]

unpack_profile() stores the optional rename and attach strings using
aa_unpack_str(), which returns pointers into the policy blob.
profile->rename is later freed with kfree_sensitive(), causing an
invalid free of an interior pointer on the failed-unpack path.
profile->attach.xmatch_str can instead become a dangling pointer when
the original policy blob is freed after compressed binary policy export.

The dangling xmatch_str is read by seq_profile_attach_show() through
the world-readable per-profile "attach" file, giving an unprivileged
slab-use-after-free read. ent->new->rename is also passed to
__lookup_replace() in the same load syscall, and the rawdata dedup skip
path frees the profile again, double freeing the interior rename
pointer.

Use aa_unpack_strdup() for both strings and free xmatch_str with the
attachment.

Fixes: 736ec752d95e9 ("AppArmor: policy routines for loading and unpacking policy")
Fixes: 63c16c3a76085 ("apparmor: Initial implementation of raw policy blob compression")
Cc: stable@vger.kernel.org
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
---
 security/apparmor/policy.c        | 1 +
 security/apparmor/policy_unpack.c | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index 397f9ff78aeb1..695bea2a70820 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -282,6 +282,7 @@ static void free_attachment(struct aa_attachment *attach)
 	for (i = 0; i < attach->xattr_count; i++)
 		kfree_sensitive(attach->xattrs[i]);
 	kfree_sensitive(attach->xattrs);
+	kfree_sensitive(attach->xmatch_str);
 	aa_put_pdb(attach->xmatch);
 }
 
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index f1fc48e72d0e1..6da4579bbd8ed 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -1121,6 +1121,7 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
 	size_t ns_len;
 	struct rhashtable_params params = { 0 };
 	char *key = NULL, *disconnected = NULL;
+	char *rename = NULL, *attach_str = NULL;
 	struct aa_data *data;
 	int error = -EPROTO;
 	kernel_cap_t tmpcap;
@@ -1160,10 +1161,12 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name)
 	rules = profile->label.rules[0];
 
 	/* profile renaming is optional */
-	(void) aa_unpack_str(e, &profile->rename, "rename");
+	(void) aa_unpack_strdup(e, &rename, "rename");
+	profile->rename = rename;
 
 	/* attachment string is optional */
-	(void) aa_unpack_str(e, &profile->attach.xmatch_str, "attach");
+	(void) aa_unpack_strdup(e, &attach_str, "attach");
+	profile->attach.xmatch_str = attach_str;
 
 	/* xmatch is optional and may be NULL */
 	error = unpack_pdb(e, &profile->attach.xmatch, false, false, &info);
-- 
2.54.0 (Apple Git-157)