[PATCH] apparmor: fix integer overflow in verify_tags() bounds check

[PATCH] apparmor: fix integer overflow in verify_tags() bounds check

[Fabrice Derepas]

verify_tags() validates the tagset table unpacked from a policy blob.
For each set it reads a count and checks that advancing the index by
that count stays inside sets.table[]:

	u32 cnt = tags->sets.table[i];

	if (i+cnt >= tags->sets.size) {

i, cnt and sets.size are all u32, so i+cnt is evaluated modulo 2^32.
sets.table[] is filled by unpack_tagsets() with aa_unpack_u32(), so
every entry is a raw unbounded 32-bit word taken from the policy blob,
and verify_tags() is the function that is supposed to validate it.  A
count close to U32_MAX makes the sum wrap to a small value, the guard
passes, and the inner loop then walks sets.table[++i] past the end of
the kcalloc(size, sizeof(u32)) allocation.

Note that sets.size is bounded by 65535, because unpack_tagsets() reads
it with aa_unpack_array() as a u16, so the wrap cannot be reached by
growing the table; it is reached purely through the attacker-supplied
count.

With sets.size = 2 and sets.table = { 0, 0xffffffff }:

  i = 0: cnt = 0, guard 0 + 0 >= 2 is false, inner loop does not run
  i = 1: cnt = 0xffffffff, guard (1 + 0xffffffff) mod 2^32 == 0 >= 2 is
         false, so the guard is bypassed and the inner loop reads
         sets.table[2] -- one element past a two element allocation

The walk continues until an out-of-bounds value happens to be >=
hdrs.size or the access faults, so a crafted policy yields an
out-of-bounds read on the policy load path
(aa_replace_profiles -> aa_unpack -> unpack_policydb -> unpack_tags ->
verify_tags).  unpack_tags() runs before the perms and DFA tables are
unpacked, so no other table needs to be well formed to reach it.

Policy load is gated by aa_may_manage_policy(), which checks
CAP_MAC_ADMIN relative to the subject's own user namespace rather than
the init user namespace, so with the default
unprivileged_userns_apparmor_policy=1 the path is reachable from an
unprivileged task in a matched-level nested namespace, not only by a
globally privileged one.

Perform the addition in u64 so that it cannot wrap, restoring the
intended i + cnt < sets.size guarantee.

Fixes: 3d28e2397af7 ("apparmor: add support loading per permission tagging")
Signed-off-by: Fabrice Derepas <fabrice.derepas@canonical.com>
---
 security/apparmor/policy_unpack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index d9dcff1..15b5e92 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -730,7 +730,7 @@ static bool verify_tags(struct aa_tags_struct *tags, const char **info)
 		/* count followed by count indexes into hdrs */
 		u32 cnt = tags->sets.table[i];
 
-		if (i+cnt >= tags->sets.size) {
+		if ((u64)i + cnt >= tags->sets.size) {
 			AA_DEBUG(DEBUG_UNPACK,
 				 "tagset too large %d+%d > sets.table[%d]",
 				 i, cnt, tags->sets.size);
-- 
2.43.0

Re: [PATCH] apparmor: fix integer overflow in verify_tags() bounds check

[John Johansen]

On 8/9/26 07:29, Fabrice Derepas wrote:
> verify_tags() validates the tagset table unpacked from a policy blob.
> For each set it reads a count and checks that advancing the index by
> that count stays inside sets.table[]:
> 
> 	u32 cnt = tags->sets.table[i];
> 
> 	if (i+cnt >= tags->sets.size) {
> 
> i, cnt and sets.size are all u32, so i+cnt is evaluated modulo 2^32.
> sets.table[] is filled by unpack_tagsets() with aa_unpack_u32(), so
> every entry is a raw unbounded 32-bit word taken from the policy blob,
> and verify_tags() is the function that is supposed to validate it.  A
> count close to U32_MAX makes the sum wrap to a small value, the guard
> passes, and the inner loop then walks sets.table[++i] past the end of
> the kcalloc(size, sizeof(u32)) allocation.
> 
> Note that sets.size is bounded by 65535, because unpack_tagsets() reads
> it with aa_unpack_array() as a u16, so the wrap cannot be reached by
> growing the table; it is reached purely through the attacker-supplied
> count.
> 
> With sets.size = 2 and sets.table = { 0, 0xffffffff }:
> 
>    i = 0: cnt = 0, guard 0 + 0 >= 2 is false, inner loop does not run
>    i = 1: cnt = 0xffffffff, guard (1 + 0xffffffff) mod 2^32 == 0 >= 2 is
>           false, so the guard is bypassed and the inner loop reads
>           sets.table[2] -- one element past a two element allocation
> 
> The walk continues until an out-of-bounds value happens to be >=
> hdrs.size or the access faults, so a crafted policy yields an
> out-of-bounds read on the policy load path
> (aa_replace_profiles -> aa_unpack -> unpack_policydb -> unpack_tags ->
> verify_tags).  unpack_tags() runs before the perms and DFA tables are
> unpacked, so no other table needs to be well formed to reach it.
> 
> Policy load is gated by aa_may_manage_policy(), which checks
> CAP_MAC_ADMIN relative to the subject's own user namespace rather than
> the init user namespace, so with the default
> unprivileged_userns_apparmor_policy=1 the path is reachable from an
> unprivileged task in a matched-level nested namespace, not only by a
> globally privileged one.
> 
> Perform the addition in u64 so that it cannot wrap, restoring the
> intended i + cnt < sets.size guarantee.
> 
> Fixes: 3d28e2397af7 ("apparmor: add support loading per permission tagging")
> Signed-off-by: Fabrice Derepas <fabrice.derepas@canonical.com>

Acked-by: John Johansen <john.johansen@canonical.com>

> ---
>   security/apparmor/policy_unpack.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index d9dcff1..15b5e92 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -730,7 +730,7 @@ static bool verify_tags(struct aa_tags_struct *tags, const char **info)
>   		/* count followed by count indexes into hdrs */
>   		u32 cnt = tags->sets.table[i];
>   
> -		if (i+cnt >= tags->sets.size) {
> +		if ((u64)i + cnt >= tags->sets.size) {
>   			AA_DEBUG(DEBUG_UNPACK,
>   				 "tagset too large %d+%d > sets.table[%d]",
>   				 i, cnt, tags->sets.size);

Re: [PATCH] apparmor: fix integer overflow in verify_tags() bounds check

[John Johansen]

On 8/9/26 07:29, Fabrice Derepas wrote:
> verify_tags() validates the tagset table unpacked from a policy blob.
> For each set it reads a count and checks that advancing the index by
> that count stays inside sets.table[]:
> 
> 	u32 cnt = tags->sets.table[i];
> 
> 	if (i+cnt >= tags->sets.size) {
> 
> i, cnt and sets.size are all u32, so i+cnt is evaluated modulo 2^32.
> sets.table[] is filled by unpack_tagsets() with aa_unpack_u32(), so
> every entry is a raw unbounded 32-bit word taken from the policy blob,
> and verify_tags() is the function that is supposed to validate it.  A
> count close to U32_MAX makes the sum wrap to a small value, the guard
> passes, and the inner loop then walks sets.table[++i] past the end of
> the kcalloc(size, sizeof(u32)) allocation.
> 
> Note that sets.size is bounded by 65535, because unpack_tagsets() reads
> it with aa_unpack_array() as a u16, so the wrap cannot be reached by
> growing the table; it is reached purely through the attacker-supplied
> count.
> 
> With sets.size = 2 and sets.table = { 0, 0xffffffff }:
> 
>    i = 0: cnt = 0, guard 0 + 0 >= 2 is false, inner loop does not run
>    i = 1: cnt = 0xffffffff, guard (1 + 0xffffffff) mod 2^32 == 0 >= 2 is
>           false, so the guard is bypassed and the inner loop reads
>           sets.table[2] -- one element past a two element allocation
> 
> The walk continues until an out-of-bounds value happens to be >=
> hdrs.size or the access faults, so a crafted policy yields an
> out-of-bounds read on the policy load path
> (aa_replace_profiles -> aa_unpack -> unpack_policydb -> unpack_tags ->
> verify_tags).  unpack_tags() runs before the perms and DFA tables are
> unpacked, so no other table needs to be well formed to reach it.
> 
> Policy load is gated by aa_may_manage_policy(), which checks
> CAP_MAC_ADMIN relative to the subject's own user namespace rather than
> the init user namespace, so with the default
> unprivileged_userns_apparmor_policy=1 the path is reachable from an
> unprivileged task in a matched-level nested namespace, not only by a
> globally privileged one.
> 
> Perform the addition in u64 so that it cannot wrap, restoring the
> intended i + cnt < sets.size guarantee.
> 
> Fixes: 3d28e2397af7 ("apparmor: add support loading per permission tagging")
> Signed-off-by: Fabrice Derepas <fabrice.derepas@canonical.com>

I have pulled this into the apparmor tree

> ---
>   security/apparmor/policy_unpack.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index d9dcff1..15b5e92 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -730,7 +730,7 @@ static bool verify_tags(struct aa_tags_struct *tags, const char **info)
>   		/* count followed by count indexes into hdrs */
>   		u32 cnt = tags->sets.table[i];
>   
> -		if (i+cnt >= tags->sets.size) {
> +		if ((u64)i + cnt >= tags->sets.size) {
>   			AA_DEBUG(DEBUG_UNPACK,
>   				 "tagset too large %d+%d > sets.table[%d]",
>   				 i, cnt, tags->sets.size);