[PATCH] apparmor: check accept index + 1 is in bounds

[PATCH] apparmor: check accept index + 1 is in bounds

[Guanglei Zhu]

aa_lookup_condperms() returns perms[index + 1] when an accept entry has
ACCEPT_FLAG_OWNER set and the subject uid does not match the file uid,
so such an entry consumes two slots in the perms table.  But
verify_dfa_accept_index() only checks that index is within the perms
table, so a policy that sets the owner flag on the last entry is
accepted, and the runtime lookup then reads perms[index + 1] one entry
past the end of the table.

The perms table is allocated by unpack_perms_table() with a u16 size
read from the policy blob, so a crafted policy can place the owner flag
at index size - 1 to reach the out-of-bounds read.

Check that index + 1 is also in bounds when the owner flag is set.

Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
Found by code inspection: aa_lookup_condperms() reads perms[index + 1]
for owner-conditional entries, but verify_dfa_accept_index() does not
bound index + 1.

 security/apparmor/policy_unpack.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index f1fc48e72..1ea89e9c5 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -1489,6 +1489,15 @@ static bool verify_dfa_accept_index(const struct aa_dfa *dfa, int table_size)
 	for (i = 0; i < dfa->tables[YYTD_ID_ACCEPT]->td_lolen; i++) {
 		if (ACCEPT_TABLE(dfa)[i] >= table_size)
 			return false;
+		/*
+		 * Accept indexes for owner-conditional permissions come in
+		 * pairs, so the non-owner entry at index + 1 must also be
+		 * in bounds.
+		 */
+		if (dfa->tables[YYTD_ID_ACCEPT2] &&
+		    (ACCEPT_TABLE2(dfa)[i] & ACCEPT_FLAG_OWNER) &&
+		    ACCEPT_TABLE(dfa)[i] + 1 >= table_size)
+			return false;
 	}
 	return true;
 }
-- 
2.43.0

Re: [PATCH] apparmor: check accept index + 1 is in bounds

[John Johansen]

On 9/16/26 04:53, Guanglei Zhu wrote:
> aa_lookup_condperms() returns perms[index + 1] when an accept entry has
> ACCEPT_FLAG_OWNER set and the subject uid does not match the file uid,
> so such an entry consumes two slots in the perms table.  But
> verify_dfa_accept_index() only checks that index is within the perms
> table, so a policy that sets the owner flag on the last entry is
> accepted, and the runtime lookup then reads perms[index + 1] one entry
> past the end of the table.
> 
> The perms table is allocated by unpack_perms_table() with a u16 size
> read from the policy blob, so a crafted policy can place the owner flag
> at index size - 1 to reach the out-of-bounds read.
> 
> Check that index + 1 is also in bounds when the owner flag is set.
> 
> Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>

so this isn't quite right, the index+1 constraint only applies to some
of the dfas, it very much depends on how they are used and encoded.
I am going to take this as is, but add a follow on patch to address
that the check is actually conditional upon which dfa and the condition
under which it is loaded.

I have pulled this into the apparmor tree

> ---
> Found by code inspection: aa_lookup_condperms() reads perms[index + 1]
> for owner-conditional entries, but verify_dfa_accept_index() does not
> bound index + 1.
> 
>   security/apparmor/policy_unpack.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index f1fc48e72..1ea89e9c5 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -1489,6 +1489,15 @@ static bool verify_dfa_accept_index(const struct aa_dfa *dfa, int table_size)
>   	for (i = 0; i < dfa->tables[YYTD_ID_ACCEPT]->td_lolen; i++) {
>   		if (ACCEPT_TABLE(dfa)[i] >= table_size)
>   			return false;
> +		/*
> +		 * Accept indexes for owner-conditional permissions come in
> +		 * pairs, so the non-owner entry at index + 1 must also be
> +		 * in bounds.
> +		 */
> +		if (dfa->tables[YYTD_ID_ACCEPT2] &&
> +		    (ACCEPT_TABLE2(dfa)[i] & ACCEPT_FLAG_OWNER) &&
> +		    ACCEPT_TABLE(dfa)[i] + 1 >= table_size)
> +			return false;
>   	}
>   	return true;
>   }