-
Notifications
You must be signed in to change notification settings - Fork 212
Fix apparmor vulnerabilities (QID-45097) #560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
saiarcot895
wants to merge
1
commit into
sonic-net:master
Choose a base branch
from
saiarcot895:fix-apparmor
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
56 changes: 56 additions & 0 deletions
56
...s-sonic/qsa-2026-apparmor/0001-apparmor-validate-DFA-start-states-are-in-bounds-in-.patch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| From 0bc5b34e913eadcd3bcc51e4d365bf35f81b5c1f Mon Sep 17 00:00:00 2001 | ||
| From: Massimiliano Pellizzer <massimiliano.pellizzer@canonical.com> | ||
| Date: Thu, 15 Jan 2026 15:30:50 +0100 | ||
| Subject: [PATCH 01/11] apparmor: validate DFA start states are in bounds in | ||
| unpack_pdb | ||
|
|
||
| Start states are read from untrusted data and used as indexes into the | ||
| DFA state tables. The aa_dfa_next() function call in unpack_pdb() will | ||
| access dfa->tables[YYTD_ID_BASE][start], and if the start state exceeds | ||
| the number of states in the DFA, this results in an out-of-bound read. | ||
|
|
||
| ================================================================== | ||
| BUG: KASAN: slab-out-of-bounds in aa_dfa_next+0x2a1/0x360 | ||
| Read of size 4 at addr ffff88811956fb90 by task su/1097 | ||
| ... | ||
|
|
||
| Reject policies with out-of-bounds start states during unpacking | ||
| to prevent the issue. | ||
|
|
||
| Fixes: ad5ff3db53c6 ("AppArmor: Add ability to load extended policy") | ||
| Reported-by: Qualys Security Advisory <qsa@qualys.com> | ||
| Tested-by: Salvatore Bonaccorso <carnil@debian.org> | ||
| Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com> | ||
| Reviewed-by: Cengiz Can <cengiz.can@canonical.com> | ||
| Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer@canonical.com> | ||
| Signed-off-by: John Johansen <john.johansen@canonical.com> | ||
| --- | ||
| security/apparmor/policy_unpack.c | 12 +++++++++++- | ||
| 1 file changed, 11 insertions(+), 1 deletion(-) | ||
|
|
||
| diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c | ||
| index 3483c595f999..e472442274d1 100644 | ||
| --- a/security/apparmor/policy_unpack.c | ||
| +++ b/security/apparmor/policy_unpack.c | ||
| @@ -760,7 +760,17 @@ static int unpack_pdb(struct aa_ext *e, struct aa_policydb **policy, | ||
| if (!aa_unpack_u32(e, &pdb->start[AA_CLASS_FILE], "dfa_start")) { | ||
| /* default start state for xmatch and file dfa */ | ||
| pdb->start[AA_CLASS_FILE] = DFA_START; | ||
| - } /* setup class index */ | ||
| + } | ||
| + | ||
| + size_t state_count = pdb->dfa->tables[YYTD_ID_BASE]->td_lolen; | ||
| + | ||
| + if (pdb->start[0] >= state_count || | ||
| + pdb->start[AA_CLASS_FILE] >= state_count) { | ||
| + *info = "invalid dfa start state"; | ||
| + goto fail; | ||
| + } | ||
| + | ||
| + /* setup class index */ | ||
| for (i = AA_CLASS_FILE + 1; i <= AA_CLASS_LAST; i++) { | ||
| pdb->start[i] = aa_dfa_next(pdb->dfa, pdb->start[0], | ||
| i); | ||
| -- | ||
| 2.53.0 | ||
|
|
||
40 changes: 40 additions & 0 deletions
40
patches-sonic/qsa-2026-apparmor/0002-apparmor-fix-memory-leak-in-verify_header.patch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| From 66ecb6689b1ee73fd2ecf7aea226ef21d72163cd Mon Sep 17 00:00:00 2001 | ||
| From: Massimiliano Pellizzer <massimiliano.pellizzer@canonical.com> | ||
| Date: Tue, 20 Jan 2026 15:24:04 +0100 | ||
| Subject: [PATCH 02/11] apparmor: fix memory leak in verify_header | ||
|
|
||
| The function sets `*ns = NULL` on every call, leaking the namespace | ||
| string allocated in previous iterations when multiple profiles are | ||
| unpacked. This also breaks namespace consistency checking since *ns | ||
| is always NULL when the comparison is made. | ||
|
|
||
| Remove the incorrect assignment. | ||
| The caller (aa_unpack) initializes *ns to NULL once before the loop, | ||
| which is sufficient. | ||
|
|
||
| Fixes: dd51c8485763 ("apparmor: provide base for multiple profiles to be replaced at once") | ||
| Reported-by: Qualys Security Advisory <qsa@qualys.com> | ||
| Tested-by: Salvatore Bonaccorso <carnil@debian.org> | ||
| Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com> | ||
| Reviewed-by: Cengiz Can <cengiz.can@canonical.com> | ||
| Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer@canonical.com> | ||
| Signed-off-by: John Johansen <john.johansen@canonical.com> | ||
| --- | ||
| security/apparmor/policy_unpack.c | 1 - | ||
| 1 file changed, 1 deletion(-) | ||
|
|
||
| diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c | ||
| index e472442274d1..0232fc29a234 100644 | ||
| --- a/security/apparmor/policy_unpack.c | ||
| +++ b/security/apparmor/policy_unpack.c | ||
| @@ -1140,7 +1140,6 @@ static int verify_header(struct aa_ext *e, int required, const char **ns) | ||
| { | ||
| int error = -EPROTONOSUPPORT; | ||
| const char *name = NULL; | ||
| - *ns = NULL; | ||
|
|
||
| /* get the interface version */ | ||
| if (!aa_unpack_u32(e, &e->version, "version")) { | ||
| -- | ||
| 2.53.0 | ||
|
|
86 changes: 86 additions & 0 deletions
86
...s-sonic/qsa-2026-apparmor/0003-apparmor-replace-recursive-profile-removal-with-iter.patch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,86 @@ | ||||||||
| From 83417f15ea8253d96e6636fbb1c1cf95c0e6b6af Mon Sep 17 00:00:00 2001 | ||||||||
| From: Massimiliano Pellizzer <massimiliano.pellizzer@canonical.com> | ||||||||
| Date: Tue, 13 Jan 2026 09:09:43 +0100 | ||||||||
| Subject: [PATCH 03/11] apparmor: replace recursive profile removal with | ||||||||
| iterative approach | ||||||||
|
|
||||||||
| The profile removal code uses recursion when removing nested profiles, | ||||||||
| which can lead to kernel stack exhaustion and system crashes. | ||||||||
|
|
||||||||
| Reproducer: | ||||||||
| $ pf='a'; for ((i=0; i<1024; i++)); do | ||||||||
| echo -e "profile $pf { \n }" | apparmor_parser -K -a; | ||||||||
| pf="$pf//x"; | ||||||||
| done | ||||||||
| $ echo -n a > /sys/kernel/security/apparmor/.remove | ||||||||
|
|
||||||||
| Replace the recursive __aa_profile_list_release() approach with an | ||||||||
| iterative approach in __remove_profile(). The function repeatedly | ||||||||
| finds and removes leaf profiles until the entire subtree is removed, | ||||||||
| maintaining the same removal semantic without recursion. | ||||||||
|
|
||||||||
| Fixes: c88d4c7b049e ("AppArmor: core policy routines") | ||||||||
| Reported-by: Qualys Security Advisory <qsa@qualys.com> | ||||||||
| Tested-by: Salvatore Bonaccorso <carnil@debian.org> | ||||||||
| Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com> | ||||||||
| Reviewed-by: Cengiz Can <cengiz.can@canonical.com> | ||||||||
| Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer@canonical.com> | ||||||||
| Signed-off-by: John Johansen <john.johansen@canonical.com> | ||||||||
| --- | ||||||||
| security/apparmor/policy.c | 30 +++++++++++++++++++++++++++--- | ||||||||
| 1 file changed, 27 insertions(+), 3 deletions(-) | ||||||||
|
|
||||||||
| diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c | ||||||||
| index 105706abf281..f6869ea00d1d 100644 | ||||||||
| --- a/security/apparmor/policy.c | ||||||||
| +++ b/security/apparmor/policy.c | ||||||||
| @@ -184,19 +184,43 @@ static void __list_remove_profile(struct aa_profile *profile) | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| - * __remove_profile - remove old profile, and children | ||||||||
| - * @profile: profile to be replaced (NOT NULL) | ||||||||
| + * __remove_profile - remove profile, and children | ||||||||
| + * @profile: profile to be removed (NOT NULL) | ||||||||
| * | ||||||||
| * Requires: namespace list lock be held, or list not be shared | ||||||||
| */ | ||||||||
| static void __remove_profile(struct aa_profile *profile) | ||||||||
| { | ||||||||
| + struct aa_profile *curr, *to_remove; | ||||||||
| + | ||||||||
| AA_BUG(!profile); | ||||||||
| AA_BUG(!profile->ns); | ||||||||
| AA_BUG(!mutex_is_locked(&profile->ns->lock)); | ||||||||
|
|
||||||||
| /* release any children lists first */ | ||||||||
| - __aa_profile_list_release(&profile->base.profiles); | ||||||||
| + if (!list_empty(&profile->base.profiles)) { | ||||||||
| + curr = list_first_entry(&profile->base.profiles, struct aa_profile, base.list); | ||||||||
|
||||||||
| + curr = list_first_entry(&profile->base.profiles, struct aa_profile, base.list); | |
| + curr = list_first_entry(&profile->base.profiles, | |
| + struct aa_profile, base.list); |
52 changes: 52 additions & 0 deletions
52
...s-sonic/qsa-2026-apparmor/0004-apparmor-fix-limit-the-number-of-levels-of-policy-na.patch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| From 4fd74536d8c7e2fc69dc58907a2b082e393cfff4 Mon Sep 17 00:00:00 2001 | ||
| From: John Johansen <john.johansen@canonical.com> | ||
| Date: Tue, 3 Mar 2026 11:08:02 -0800 | ||
| Subject: [PATCH 04/11] apparmor: fix: limit the number of levels of policy | ||
| namespaces | ||
|
|
||
| Currently the number of policy namespaces is not bounded relying on | ||
| the user namespace limit. However policy namespaces aren't strictly | ||
| tied to user namespaces and it is possible to create them and nest | ||
| them arbitrarily deep which can be used to exhaust system resource. | ||
|
|
||
| Hard cap policy namespaces to the same depth as user namespaces. | ||
|
|
||
| Fixes: c88d4c7b049e8 ("AppArmor: core policy routines") | ||
| Reported-by: Qualys Security Advisory <qsa@qualys.com> | ||
| Reviewed-by: Ryan Lee <ryan.lee@canonical.com> | ||
| Reviewed-by: Cengiz Can <cengiz.can@canonical.com> | ||
| Signed-off-by: John Johansen <john.johansen@canonical.com> | ||
| --- | ||
| security/apparmor/include/policy_ns.h | 2 ++ | ||
| security/apparmor/policy_ns.c | 2 ++ | ||
| 2 files changed, 4 insertions(+) | ||
|
|
||
| diff --git a/security/apparmor/include/policy_ns.h b/security/apparmor/include/policy_ns.h | ||
| index d646070fd966..cc6e84151812 100644 | ||
| --- a/security/apparmor/include/policy_ns.h | ||
| +++ b/security/apparmor/include/policy_ns.h | ||
| @@ -18,6 +18,8 @@ | ||
| #include "label.h" | ||
| #include "policy.h" | ||
|
|
||
| +/* Match max depth of user namespaces */ | ||
| +#define MAX_NS_DEPTH 32 | ||
|
|
||
| /* struct aa_ns_acct - accounting of profiles in namespace | ||
| * @max_size: maximum space allowed for all profiles in namespace | ||
| diff --git a/security/apparmor/policy_ns.c b/security/apparmor/policy_ns.c | ||
| index 1f02cfe1d974..06c0bde97a63 100644 | ||
| --- a/security/apparmor/policy_ns.c | ||
| +++ b/security/apparmor/policy_ns.c | ||
| @@ -223,6 +223,8 @@ static struct aa_ns *__aa_create_ns(struct aa_ns *parent, const char *name, | ||
| AA_BUG(!name); | ||
| AA_BUG(!mutex_is_locked(&parent->lock)); | ||
|
|
||
| + if (parent->level > MAX_NS_DEPTH) | ||
| + return ERR_PTR(-ENOSPC); | ||
| ns = alloc_ns(parent->base.hname, name); | ||
| if (!ns) | ||
| return ERR_PTR(-ENOMEM); | ||
| -- | ||
| 2.53.0 | ||
|
|
124 changes: 124 additions & 0 deletions
124
...s-sonic/qsa-2026-apparmor/0005-apparmor-fix-side-effect-bug-in-match_char-macro-usa.patch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| From 05302d7b0886b14de3d0e91a1b0c332f9ea0d292 Mon Sep 17 00:00:00 2001 | ||
| From: Massimiliano Pellizzer <massimiliano.pellizzer@canonical.com> | ||
| Date: Thu, 29 Jan 2026 17:08:25 +0100 | ||
| Subject: [PATCH 05/11] apparmor: fix side-effect bug in match_char() macro | ||
| usage | ||
|
|
||
| The match_char() macro evaluates its character parameter multiple | ||
| times when traversing differential encoding chains. When invoked | ||
| with *str++, the string pointer advances on each iteration of the | ||
| inner do-while loop, causing the DFA to check different characters | ||
| at each iteration and therefore skip input characters. | ||
| This results in out-of-bounds reads when the pointer advances past | ||
| the input buffer boundary. | ||
|
|
||
| [ 94.984676] ================================================================== | ||
| [ 94.985301] BUG: KASAN: slab-out-of-bounds in aa_dfa_match+0x5ae/0x760 | ||
| [ 94.985655] Read of size 1 at addr ffff888100342000 by task file/976 | ||
|
|
||
| [ 94.986319] CPU: 7 UID: 1000 PID: 976 Comm: file Not tainted 6.19.0-rc7-next-20260127 #1 PREEMPT(lazy) | ||
| [ 94.986322] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 | ||
| [ 94.986329] Call Trace: | ||
| [ 94.986341] <TASK> | ||
| [ 94.986347] dump_stack_lvl+0x5e/0x80 | ||
| [ 94.986374] print_report+0xc8/0x270 | ||
| [ 94.986384] ? aa_dfa_match+0x5ae/0x760 | ||
| [ 94.986388] kasan_report+0x118/0x150 | ||
| [ 94.986401] ? aa_dfa_match+0x5ae/0x760 | ||
| [ 94.986405] aa_dfa_match+0x5ae/0x760 | ||
| [ 94.986408] __aa_path_perm+0x131/0x400 | ||
| [ 94.986418] aa_path_perm+0x219/0x2f0 | ||
| [ 94.986424] apparmor_file_open+0x345/0x570 | ||
| [ 94.986431] security_file_open+0x5c/0x140 | ||
| [ 94.986442] do_dentry_open+0x2f6/0x1120 | ||
| [ 94.986450] vfs_open+0x38/0x2b0 | ||
| [ 94.986453] ? may_open+0x1e2/0x2b0 | ||
| [ 94.986466] path_openat+0x231b/0x2b30 | ||
| [ 94.986469] ? __x64_sys_openat+0xf8/0x130 | ||
| [ 94.986477] do_file_open+0x19d/0x360 | ||
| [ 94.986487] do_sys_openat2+0x98/0x100 | ||
| [ 94.986491] __x64_sys_openat+0xf8/0x130 | ||
| [ 94.986499] do_syscall_64+0x8e/0x660 | ||
| [ 94.986515] ? count_memcg_events+0x15f/0x3c0 | ||
| [ 94.986526] ? srso_alias_return_thunk+0x5/0xfbef5 | ||
| [ 94.986540] ? handle_mm_fault+0x1639/0x1ef0 | ||
| [ 94.986551] ? vma_start_read+0xf0/0x320 | ||
| [ 94.986558] ? srso_alias_return_thunk+0x5/0xfbef5 | ||
| [ 94.986561] ? srso_alias_return_thunk+0x5/0xfbef5 | ||
| [ 94.986563] ? fpregs_assert_state_consistent+0x50/0xe0 | ||
| [ 94.986572] ? srso_alias_return_thunk+0x5/0xfbef5 | ||
| [ 94.986574] ? arch_exit_to_user_mode_prepare+0x9/0xb0 | ||
| [ 94.986587] ? srso_alias_return_thunk+0x5/0xfbef5 | ||
| [ 94.986588] ? irqentry_exit+0x3c/0x590 | ||
| [ 94.986595] entry_SYSCALL_64_after_hwframe+0x76/0x7e | ||
| [ 94.986597] RIP: 0033:0x7fda4a79c3ea | ||
|
|
||
| Fix by extracting the character value before invoking match_char, | ||
| ensuring single evaluation per outer loop. | ||
|
|
||
| Fixes: 074c1cd798cb ("apparmor: dfa move character match into a macro") | ||
| Reported-by: Qualys Security Advisory <qsa@qualys.com> | ||
| Tested-by: Salvatore Bonaccorso <carnil@debian.org> | ||
| Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com> | ||
| Reviewed-by: Cengiz Can <cengiz.can@canonical.com> | ||
| Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer@canonical.com> | ||
| Signed-off-by: John Johansen <john.johansen@canonical.com> | ||
| --- | ||
| security/apparmor/match.c | 30 ++++++++++++++++++++---------- | ||
| 1 file changed, 20 insertions(+), 10 deletions(-) | ||
|
|
||
| diff --git a/security/apparmor/match.c b/security/apparmor/match.c | ||
| index 12e036f8ce0f..5fbfdd75afb5 100644 | ||
| --- a/security/apparmor/match.c | ||
| +++ b/security/apparmor/match.c | ||
| @@ -408,13 +408,18 @@ aa_state_t aa_dfa_match_len(struct aa_dfa *dfa, aa_state_t start, | ||
| if (dfa->tables[YYTD_ID_EC]) { | ||
| /* Equivalence class table defined */ | ||
| u8 *equiv = EQUIV_TABLE(dfa); | ||
| - for (; len; len--) | ||
| - match_char(state, def, base, next, check, | ||
| - equiv[(u8) *str++]); | ||
| + for (; len; len--) { | ||
| + u8 c = equiv[(u8) *str]; | ||
| + | ||
| + match_char(state, def, base, next, check, c); | ||
| + str++; | ||
| + } | ||
| } else { | ||
| /* default is direct to next state */ | ||
| - for (; len; len--) | ||
| - match_char(state, def, base, next, check, (u8) *str++); | ||
| + for (; len; len--) { | ||
| + match_char(state, def, base, next, check, (u8) *str); | ||
| + str++; | ||
| + } | ||
| } | ||
|
|
||
| return state; | ||
| @@ -448,13 +453,18 @@ aa_state_t aa_dfa_match(struct aa_dfa *dfa, aa_state_t start, const char *str) | ||
| /* Equivalence class table defined */ | ||
| u8 *equiv = EQUIV_TABLE(dfa); | ||
| /* default is direct to next state */ | ||
| - while (*str) | ||
| - match_char(state, def, base, next, check, | ||
| - equiv[(u8) *str++]); | ||
| + while (*str) { | ||
| + u8 c = equiv[(u8) *str]; | ||
| + | ||
| + match_char(state, def, base, next, check, c); | ||
| + str++; | ||
| + } | ||
| } else { | ||
| /* default is direct to next state */ | ||
| - while (*str) | ||
| - match_char(state, def, base, next, check, (u8) *str++); | ||
| + while (*str) { | ||
| + match_char(state, def, base, next, check, (u8) *str); | ||
| + str++; | ||
| + } | ||
| } | ||
|
|
||
| return state; | ||
| -- | ||
| 2.53.0 | ||
|
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch introduces a new local variable declaration (state_count) after executable statements inside unpack_pdb(). This will trigger kernel checkpatch warnings for mixed declarations and code; please move the declaration to the start of the enclosing block (or function scope) before any non-declaration statements.