Re: [PATCH] apparmor: handle diff-encoded states in leftmatch_fb()

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

On 10/26/26 06:36, Patrick Grössing wrote:
> leftmatch_fb(), used by aa_dfa_leftmatch() and therefore by find_attach()
> to match profile attachments, open-codes the DFA transition and treats the
> default transition as final:
> 
> 	if (check[pos] == state)
> 		state = next[pos];
> 	else
> 		state = def[state];
> 
> For a state flagged MATCH_FLAG_DIFF_ENCODE the default state must instead
> be tried again with the same input character, which is what match_char()
> does for every other matcher. apparmor_parser emits differentially encoded
> DFAs by default when the kernel advertises policy/diff_encode, so larger
> attachment patterns end in a non-accepting state and the exec silently
> stays unconfined. No conflict is detected, so nothing is audited.
> 
> One visible victim is Ubuntu's firefox profile, whose attachment
> 
>    /{usr/lib/firefox{,-esr,-beta,-devedition,-nightly},opt/firefox}/firefox{,-esr,-bin}
> 
> never matches. Firefox then runs unconfined, is moved to unprivileged_userns
> and is denied CAP_SYS_ADMIN, losing its user namespace sandbox. The same
> profile compiled with -O no-diff-encode attaches.
> 
> Use match_char() in both branches of leftmatch_fb().
> 
> Fixes: 21f606610502 ("apparmor: improve overlapping domain attachment resolution")
> Cc: stable@vger.kernel.org
> Link: https://gitlab.com/apparmor/apparmor-kernel/-/issues/8
> Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2172229
> Signed-off-by: Patrick Grössing <invarianztheorem@posteo.com>

thank you for the patch, unfortunately this issue has already been addressed
by daa9ef365c0ed apparmor: convert duplicated dfa transitions to match_char()


> ---
> Tested on a kernel built from Ubuntu linux 7.0.0-38.38 (upstream 7.0.14):
> with this patch the diff-encoded reproducer profile from the GitLab issue
> attaches, and the stock Ubuntu firefox profile attaches to
> /usr/lib/firefox/firefox-bin (all processes "firefox (unconfined)", no
> unprivileged_userns denials). Without it both stay unconfined, also on the
> stock 7.0.0-38-generic kernel. Compile-tested with W=1.
> Based on apparmor-next as of 2026-10-26.
> 
>   security/apparmor/match.c | 16 +++++-----------
>   1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index 7713484..796126d 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -751,7 +751,7 @@ static aa_state_t leftmatch_fb(const struct aa_dfa *dfa, aa_state_t start,
>   	u32 *base = BASE_TABLE(dfa);
>   	u32 *next = NEXT_TABLE(dfa);
>   	u32 *check = CHECK_TABLE(dfa);
> -	aa_state_t state = start, pos;
> +	aa_state_t state = start;
>   
>   	AA_BUG(!dfa);
>   	AA_BUG(!str);
> @@ -769,13 +769,10 @@ static aa_state_t leftmatch_fb(const struct aa_dfa *dfa, aa_state_t start,
>   		/* default is direct to next state */
>   		while (*str) {
>   			unsigned int adjust;
> +			u8 c = equiv[(u8) *str++];
>   
>   			wb->history[wb->pos] = state;
> -			pos = base_idx(base[state]) + equiv[(u8) *str++];
> -			if (check[pos] == state)
> -				state = next[pos];
> -			else
> -				state = def[state];
> +			match_char(state, def, base, next, check, c);
>   			if (is_loop(wb, state, &adjust)) {
>   				state = aa_dfa_match(dfa, state, str);
>   				*count -= adjust;
> @@ -788,13 +785,10 @@ static aa_state_t leftmatch_fb(const struct aa_dfa *dfa, aa_state_t start,
>   		/* default is direct to next state */
>   		while (*str) {
>   			unsigned int adjust;
> +			u8 c = (u8) *str++;
>   
>   			wb->history[wb->pos] = state;
> -			pos = base_idx(base[state]) + (u8) *str++;
> -			if (check[pos] == state)
> -				state = next[pos];
> -			else
> -				state = def[state];
> +			match_char(state, def, base, next, check, c);
>   			if (is_loop(wb, state, &adjust)) {
>   				state = aa_dfa_match(dfa, state, str);
>   				*count -= adjust;