[PATCH] apparmor: check address length before reading AF_UNSPEC bind address

[PATCH] apparmor: check address length before reading AF_UNSPEC bind address

[Michail Andreas Michalopoulos]

bind_map_addr() handles AF_UNSPEC on an IPv4 socket by reading
addr4->sin_addr.s_addr to check for INADDR_ANY. At that point the only
length check done is addrlen >= offsetofend(struct sockaddr, sa_family),
i.e. 2 bytes, while sin_addr sits at offset 4. move_addr_to_kernel()
copies only addrlen bytes into the on-stack sockaddr_storage, so a
bind() with a short AF_UNSPEC address makes AppArmor branch on bytes the
caller never supplied.

Depending on those bytes, a confined task gets -EAFNOSUPPORT instead of
the -EINVAL that inet_bind_sk() returns for the same request. Kernels
built with CONFIG_INIT_STACK_ALL_ZERO read zeroes and happen to behave
correctly.

Check the length first, as inet_bind_sk() does, and as the AF_INET6
case above already does for inet6_bind_sk().

Fixes: 89ac6aa752ec ("apparmor: push inet mediation into profile callbacks, and improve auditing")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Michail Andreas Michalopoulos <mike.michalo@gmail.com>
---
Tested on v7.3 built with CONFIG_INIT_STACK_NONE in QEMU, with a task
confined by a profile allowing all network access, calling bind() on an
AF_INET UDP socket with {.sa_family = AF_UNSPEC} and addrlen 2 and 4
(200 times each, after leaving a non-zero address in the kernel buffer):
unpatched returns -EAFNOSUPPORT every time, patched returns -EINVAL
every time, matching unconfined tasks. Other AF_UNSPEC, AF_INET and
AF_INET6 bind cases behave identically with and without the patch.
---
 security/apparmor/af_inet.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/security/apparmor/af_inet.c b/security/apparmor/af_inet.c
index 516c9016537c..a617f8058634 100644
--- a/security/apparmor/af_inet.c
+++ b/security/apparmor/af_inet.c
@@ -196,6 +196,9 @@ static int bind_map_addr(const struct sock *sk, struct sockaddr *addr,
 		 * AF_UNSPEC if the address is INADDR_ANY
 		 */
 		addr4 = (struct sockaddr_in *)addr;
+		/* Length check from inet_bind_sk() */
+		if (addrlen < sizeof(struct sockaddr_in))
+			return -EINVAL;
 		if (addr4->sin_addr.s_addr != htonl(INADDR_ANY))
 			return -EAFNOSUPPORT;
 		family = AF_INET;
-- 
2.56.0

Re: [PATCH] apparmor: check address length before reading AF_UNSPEC bind address

[sashiko-bot]

> bind_map_addr() handles AF_UNSPEC on an IPv4 socket by reading
> addr4->sin_addr.s_addr to check for INADDR_ANY. At that point the only
> length check done is addrlen >= offsetofend(struct sockaddr, sa_family),
> i.e. 2 bytes, while sin_addr sits at offset 4. move_addr_to_kernel()
> copies only addrlen bytes into the on-stack sockaddr_storage, so a
> bind() with a short AF_UNSPEC address makes AppArmor branch on bytes the
> caller never supplied.
> 
> Depending on those bytes, a confined task gets -EAFNOSUPPORT instead of
> the -EINVAL that inet_bind_sk() returns for the same request. Kernels
> built with CONFIG_INIT_STACK_ALL_ZERO read zeroes and happen to behave
> correctly.
> 
> Check the length first, as inet_bind_sk() does, and as the AF_INET6
> case above already does for inet6_bind_sk().
> 
> Fixes: 89ac6aa752ec ("apparmor: push inet mediation into profile callbacks, and improve auditing")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Michail Andreas Michalopoulos <mike.michalo@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020140030.92643-1-mike.michalo@gmail.com?part=1