Re: [PATCH] apparmor: fix query_data() output overlapping the lookup key

[John Johansen <john.johansen@canonical.com>]

On 10/12/26 14:57, Cen Zhang (Microsoft) wrote:
> In query_data(), buf and query point into the same multi_transaction
> buffer: buf at the start, query a few bytes in, and the lookup key
> right after the label string that query points to. Reusing the buffer
> is by design and is safe as long as the query is fully parsed before
> buf is written to, but the key is not: it is dereferenced on every
> iteration of label_for_each_confined(), after output has already been
> written. So when the label has more than one profile with a data
> table, the data of the first matching profile is written to buf while
> the key is still needed, and overwrites it if the data is large enough.
> The later lookups then run on a corrupted key. Besides, if the data has
> no NUL byte from the key's position to the end of the allocation,
> strhash() -> strlen() -> jhash() reads past the 4096-byte slab object,
> triggering an out-of-bound read:
> 
>    BUG: KASAN: slab-out-of-bounds in jhash+0x595/0x5e0
>    Read of size 1 at addr ff1100001159d000 by task exploit/155
>    CPU: 0 UID: 1000 PID: 155 Comm: exploit Not tainted 7.3.0-rc7 #1
>    Call Trace:
>     jhash+0x595/0x5e0
>     aa_write_access+0x880/0x1400
>     vfs_write+0x26f/0xd20
>     ksys_write+0x115/0x210
> 
> Writing to the .access file is unprivileged (0666), but triggering this
> requires loaded profiles that carry a data table, which needs
> CAP_MAC_ADMIN and is not the case on default distribution policies.
> 
> Fix by copying the key with kstrdup() before writing any output to buf,
> so the key stays valid across all loop iterations regardless of what
> gets written to the overlapping buffer.
> 
> Fixes: 317d9a054e1c ("apparmor: update query interface to support label queries")
> Cc: stable@vger.kernel.org # v4.13+
> Cc: Francis Perron <francis@akrites.dev>
> Reported-by: AutonomousCodeSecurity@microsoft.com
> Signed-off-by: Cen Zhang (Microsoft) <cenzhang@linux.microsoft.com>

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

this has been pulled into the apparmor tree

> ---
>   security/apparmor/apparmorfs.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 5b42140e12e8..9df06fdd894f 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -919,7 +919,7 @@ static void profile_query_cb(const struct aa_profile *profile,
>   static ssize_t query_data(char *buf, size_t buf_len,
>   			  char *query, size_t query_len)
>   {
> -	char *out;
> +	char *out, *keydup;
>   	const char *key;
>   	struct label_it i;
>   	struct aa_label *label, *curr;
> @@ -941,11 +941,18 @@ static ssize_t query_data(char *buf, size_t buf_len,
>   	if (buf_len < sizeof(bytes) + sizeof(blocks))
>   		return -EINVAL; /* not enough space */
>   
> +	/* buf overlaps query, keep a copy of the key for the later lookups */
> +	keydup = kstrdup(key, GFP_KERNEL);
> +	if (!keydup)
> +		return -ENOMEM;
> +
>   	curr = begin_current_label_crit_section(&needput);
>   	label = aa_label_parse(curr, query, GFP_KERNEL, false, false);
>   	end_current_label_crit_section(curr, needput);
> -	if (IS_ERR(label))
> +	if (IS_ERR(label)) {
> +		kfree(keydup);
>   		return PTR_ERR(label);
> +	}
>   
>   	/* We are going to leave space for two numbers. The first is the total
>   	 * number of bytes we are writing after the first number. This is so
> @@ -963,13 +970,14 @@ static ssize_t query_data(char *buf, size_t buf_len,
>   		if (!profile->data)
>   			continue;
>   
> -		data = rhashtable_lookup_fast(profile->data, &key,
> +		data = rhashtable_lookup_fast(profile->data, &keydup,
>   					      profile->data->p);
>   
>   		if (data) {
>   			if (out + sizeof(outle32) + data->size > buf +
>   			    buf_len) {
>   				aa_put_label(label);
> +				kfree(keydup);
>   				return -EINVAL; /* not enough space */
>   			}
>   			outle32 = __cpu_to_le32(data->size);
> @@ -981,6 +989,7 @@ static ssize_t query_data(char *buf, size_t buf_len,
>   		}
>   	}
>   	aa_put_label(label);
> +	kfree(keydup);
>   
>   	outle32 = __cpu_to_le32(out - buf - sizeof(bytes));
>   	memcpy(buf, &outle32, sizeof(outle32));