[PATCH] apparmor: fix NULL ctx->peer derefs in unix socket ctx updates

Maxime Bélair <[email protected]>
Newsgroups org.kernel.vger.linux-security-module
Message-ID <[email protected]>
aa_unix_file_perm lazily refreshes the AppArmor context cached on a unix
socket. Two of the helpers it uses assume ctx->peer has already been set:

	update_peer_ctx ->  l = aa_label_merge(old, label, GFP_ATOMIC);
	update_sk_ctx   ->  } else if (aa_label_is_subset(plabel, old)) {

where @old is ctx->peer. Neither aa_label_merge nor aa_label_is_subset
allows NULL. So both fault on the aa_label->size load.

  BUG: kernel NULL pointer dereference, address: 000000000000004c
  RIP: 0010:__aa_label_next_not_in_set+0xb/0xd0
  Call Trace:
   aa_label_is_subset+0x3f/0x70
   aa_unix_file_perm+0x5e8/0x9d0
   aa_file_perm+0x45a/0x550
   apparmor_file_permission+0x44/0xb0
   security_file_permission+0x40/0x100
   rw_verify_area+0x56/0x180
   vfs_write+0x7c/0x480
   ksys_write+0xbf/0xf0

ctx->peer is only recorded for stream connections and socket pairs.
unix_dgram_connect sets unix_peer(sk) without going through that path,
so a connected AF_UNIX datagram socket has unix_peer(sk) set while
ctx->peer is still NULL, and the first write that needs revalidation
reaches the helpers above.

Both derefs date back to the Fixes: commit, but the update_sk_ctx() one
was dormant until commit 4483efe4f215 ("apparmor: fix shadowing of plabel
that prevents cache from being updated") stopped @plabel being shadowed,
which is why bisecting the oops lands there.

A NULL @old just means no peer label has been recorded yet, so install
the label directly instead of merging or comparing against it.

Fixes: 88fec3526e84 ("apparmor: make sure unix socket labeling is correctly updated.")
Reported-by: Aurelien Jarno <[email protected]>
Closes: https://bugs.debian.org/1145111
Cc: [email protected]
Signed-off-by: Maxime Bélair <[email protected]>
---
 security/apparmor/af_unix.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
index b908e744818c..d04d9cd268aa 100644
--- a/security/apparmor/af_unix.c
+++ b/security/apparmor/af_unix.c
@@ -682,7 +682,7 @@ static void update_sk_ctx(struct sock *sk, struct aa_label *label,
 		if (old == plabel) {
 			rcu_assign_pointer(ctx->peer_lastupdate,
 					   aa_get_label(plabel));
-		} else if (aa_label_is_subset(plabel, old)) {
+		} else if (!old || aa_label_is_subset(plabel, old)) {
 			rcu_assign_pointer(ctx->peer_lastupdate,
 					   aa_get_label(plabel));
 			rcu_assign_pointer(ctx->peer, aa_get_label(plabel));
@@ -700,13 +700,17 @@ static void update_peer_ctx(struct sock *sk, struct aa_sk_ctx *ctx,
 	spin_lock(&unix_sk(sk)->lock);
 	old = rcu_dereference_protected(ctx->peer,
 					lockdep_is_held(&unix_sk(sk)->lock));
-	l = aa_label_merge(old, label, GFP_ATOMIC);
-	if (l) {
-		if (l != old) {
-			rcu_assign_pointer(ctx->peer, l);
-			aa_put_label(old);
-		} else
-			aa_put_label(l);
+	if (!old)
+		rcu_assign_pointer(ctx->peer, aa_get_label(label));
+	else {
+		l = aa_label_merge(old, label, GFP_ATOMIC);
+		if (l) {
+			if (l != old) {
+				rcu_assign_pointer(ctx->peer, l);
+				aa_put_label(old);
+			} else
+				aa_put_label(l);
+		}
 	}
 	spin_unlock(&unix_sk(sk)->lock);
 }
-- 
2.51.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.