| Age | Commit message (Collapse) | Author |
|---|
|
If profile create fails before policy_init is complete the list heads
are not properly initialized causing profile_free() sanity checks to
trigger the following splat.
AppArmor WARN aa_policy_destroy: (((!list_empty(&policy->profiles) && (&policy->profiles)->prev != ((void *) 0x122 + (0xdead000000000000UL))))):
WARNING: security/apparmor/lib.c:509 at aa_policy_destroy+0x164/0x1b0 security/apparmor/lib.c:509, CPU#0: syz.0.17/5541
Modules linked in:
CPU: 0 UID: 0 PID: 5541 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT(full)
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
RIP: 0010:aa_policy_destroy+0x16b/0x1b0 security/apparmor/lib.c:509
Code: 85 ed 7e 4d e8 96 bc 37 fd 5b 41 5c 41 5e 41 5f 5d e9 19 27 4e 07 cc e8 83 bc 37 fd 48 8d 3d 0c f0 d3 0b 48 c7 c6 a4 eb 38 8e <67> 48 0f b9 3a e9 04 ff ff ff e8 66 bc 37 fd 48 8d 3d ff ef d3 0b
RSP: 0018:ffffc9000345eaa0 EFLAGS: 00010293
RAX: ffffffff848f530d RBX: ffff88803f734800 RCX: ffff88801af2a580
RDX: 0000000000000000 RSI: ffffffff8e38eba4 RDI: ffffffff90634320
RBP: 0000000000000000 R08: 0000000000000cc0 R09: 00000000ffffffff
R10: dffffc0000000000 R11: fffffbfff1d95913 R12: dead000000000122
R13: ffff88803f734800 R14: ffff88803f734828 R15: dffffc0000000000
FS: 00007f5f6a1836c0(0000) GS:ffff88808c519000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055d02407b048 CR3: 0000000012aa9000 CR4: 0000000000352ef0
Call Trace:
<TASK>
aa_free_profile+0x9d/0x9f0 security/apparmor/policy.c:334
aa_alloc_profile+0x1e4/0x3e0 security/apparmor/policy.c:416
unpack_profile security/apparmor/policy_unpack.c:1153 [inline]
aa_unpack+0x17db/0x7430 security/apparmor/policy_unpack.c:1748
aa_replace_profiles+0x226/0x2a20 security/apparmor/policy.c:1183
policy_update+0x234/0x4a0 security/apparmor/apparmorfs.c:505
profile_load+0x1cb/0x320 security/apparmor/apparmorfs.c:522
vfs_write+0x296/0xba0 fs/read_write.c:685
ksys_write+0x150/0x270 fs/read_write.c:739
do_syscall_x64 arch/x86/entry/syscall_64.c:61 [inline]
do_syscall_64+0x166/0x520 arch/x86/entry/syscall_64.c:84
entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f5f6939e0d9
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 e8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f5f6a183028 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007f5f69625fa0 RCX: 00007f5f6939e0d9
RDX: 0000000000000041 RSI: 0000200000000400 RDI: 0000000000000003
RBP: 00007f5f6a183090 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
R13: 00007f5f69626038 R14: 00007f5f69625fa0 R15: 00007ffe23725c18
Reported-by: syzbot+faed97c4ed43bfe7fee5@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=faed97c4ed43bfe7fee5
Fixes: fe6bb31f590c9 ("apparmor: split out shared policy_XXX fns to lib")
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
The use of change_hat when in complain mode can cause a deadlock
when the hat doesn't exist and a new learning profile is created
for the missing profile. This is because change_hat() has taken
the lock to search the hat list and creating the new learning
profile needs to take the lock to add it to the list.
From the bug report:
Originally found in 7.0.0 in LTS ubuntu 26.04 with pam_apparmor + su
in complain mode set to change hats. Then verified in newest
available vanilla kernel I've compiled to see if still present:
7.2-rc7 vanilla -> affected
checked also some other kernels:
6.18.44 vanilla -> affected
6.12.95 with debian patches -> unaffected
On systems without bug (for example 6.12.95 debian) it just prints:
aa_change_hat rc=0
On systems with bug, the executable always hangs, prints nothing and
becomes unkillable. (And once stuck this way, it will cause any
further hat changes to also cause the changing process to get stuck)
Then in syslog you can find hint about cause:
kernel: INFO: task hat:3409 blocked for more than 483 seconds.
kernel: Not tainted 7.2.0-rc7 #1
kernel: "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kernel: task:hat state:D stack:0 pid:3409 tgid:3409 ppid:2605 task_flags:0x400000 flags:0x00080800
kernel: Call Trace:
kernel: <TASK>
kernel: __schedule+0x48f/0xfe0
kernel: schedule+0x27/0xa0
kernel: schedule_preempt_disabled+0x15/0x30
kernel: __mutex_lock.constprop.0+0x569/0xa10
kernel: aa_new_learning_profile+0x15f/0x210
kernel: build_change_hat+0x19f/0x3b0
kernel: change_hat.isra.0+0x5dd/0xd60
kernel: aa_change_hat+0x2f3/0x710
kernel: aa_setprocattr_changehat+0x121/0x1f0
kernel: do_setattr+0x28c/0x340
kernel: apparmor_setselfattr+0x20/0x50
kernel: security_setselfattr+0xf6/0x110
kernel: __x64_sys_lsm_set_self_attr+0x53/0x90
kernel: do_syscall_64+0xdd/0x5e0
kernel: ? __mod_memcg_lruvec_state+0xfd/0x260
kernel: ? lruvec_stat_mod_folio+0x8d/0xd0
kernel: ? __folio_mod_stat+0x2d/0x90
kernel: ? map_anon_folio_pte_nopf+0xd1/0x1f0
kernel: ? do_anonymous_page+0x184/0xa10
kernel: ? __handle_mm_fault+0x805/0x870
kernel: ? count_memcg_events+0xef/0x230
kernel: ? handle_mm_fault+0x1f0/0x2f0
kernel: ? do_user_addr_fault+0x2bb/0x7b0
kernel: ? do_syscall_64+0x94/0x5e0
kernel: ? exc_page_fault+0x75/0x160
kernel: entry_SYSCALL_64_after_hwframe+0x76/0x7e
kernel: RIP: 0033:0x7f815e134c8d
kernel: RSP: 002b:00007fff6df94ea8 EFLAGS: 00000246 ORIG_RAX: 00000000000001cc
kernel: RAX: ffffffffffffffda RBX: 0000556d8c81d040 RCX: 00007f815e134c8d
kernel: RDX: 0000000000000046 RSI: 0000556d8c81d040 RDI: 0000000000000064
kernel: RBP: 00007fff6df94ef0 R08: 00007f815e212ac8 R09: 000000000000000c
kernel: R10: 0000000000000000 R11: 0000000000000246 R12: 0000556d8c81d010
kernel: R13: 0000000000000026 R14: 0000000000000046 R15: 0000000000000064
kernel: </TASK>
kernel: INFO: task hat:3409 is blocked on a mutex likely owned by task hat:3409.
To fix the issue, lift the locking out of the core of
aa_new_learning_profile(), introduce a wrapper function that takes the
lock where needed, and have build_change_hat() call the core function
that no longer takes the lock.
In addition fix 4 other issues introduced by commit
32e92764d6f8d ("apparmor: grab ns lock and refresh when looking up changehat child profiles")
- aa_get_profile_rcu() was replaced-by: aa_get_profile without the
accompanying rcu_dereference_protected()
- an extra aa_get_label(label) was introduced at the start of
change_hat() without an accompanying aa_put_label() causing a
reference count leak.
- a reference count leak was introduced in the label_is_stale(label)
case, where the newest profile would be leaked instead of the
label passed to the function.
- a potential UAF when the lookup walks up the tree with new_ns != ns
the new label reference is put, and then used for the next lookup.
The mutex_lock, will block replacement, and removal in the locked
ns. However there are two cases where putting the reference can
result in the label being freed even with the lock held.
1. the label does not have a list reference (possible for temporary
or special profiles) in which case the put can trigger the
cleanup.
2. the new label reference is in a different namespace, which does
not have a lock held on it. This extends case 1 to also include
replacement, and removal that could be occurring in the namespace
new is in.
Reported-by: Martin Petricek <mp@petricek.net>
Link: https://lists.ubuntu.com/archives/apparmor/2026-August/014907.html
Fixes: 32e92764d6f8d ("apparmor: grab ns lock and refresh when looking up changehat child profiles")
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Several label helpers only read from their struct aa_label * arguments:
they compare labels, test subset relationships, or check the mediation
bitmask, all via direct field/index access. Mark those parameters
const struct aa_label * to document intent and let the compiler enforce
that the label is not modified.
The converted functions are:
- label_mediates(), label_mediates_safe()
- aa_label_cmp() (and its vec_cmp() helper)
- __aa_label_next_not_in_set(), aa_label_is_subset(),
aa_label_is_unconfined_subset()
- __aa_subj_label_is_cached()
- aa_label_next_confined(), aa_label_next_in_merge()
These all access the label through direct indexing or manual iterators
rather than the label_for_each()/fn_for_each() macros, which are not
const-correct and so gate the majority of the remaining label consumers
(the print, match, and permission-check paths) from being constified.
No functional change.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Assisted-by: Claude:claude-opus-4.8
|
|
Most uses of aa_dfa a read-only walking of the dfa. Have the compiler
enforce this.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
A number of functions take a struct aa_profile * argument that is only
ever read from: they compute DFA matches or apply the profile's mode
flags without modifying the profile, taking a reference on it, or
touching its embedded label. Mark those parameters const struct
aa_profile * to document intent and let the compiler enforce it.
The converted functions are the permission "compute" path plus a few
pure readers:
- aa_apply_modes_to_perms(), aa_profile_match_label()
- AUDIT_MODE()
- aa_label_match() and its match_component()/label_compound_match()/
label_components_match() helpers (label.c)
- match_component()/label_compound_match()/label_components_match()/
label_match()/change_profile_perms()/aa_xattrs_match() (domain.c)
- match_iface()/match_addr_iface()/match_addr_iface_label()/
skb_match_to_sk()/skb_match_to_cmd() (af_inet.c)
- aa_profile_capget(), path_flags(), profile_query_cb()
The remaining aa_profile * parameters cannot be made const: the audit
path stores &profile->label into the owned, refcounted
apparmor_audit_data.subj_label/peer fields, and the domain/lifecycle
paths take references on the profile's embedded label
(aa_get_label()/aa_get_newest_label()/aa_get_profile()) or write
profile fields.
No functional change.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Assisted-by: Claude:claude-opus-4.8
|
|
Several functions take a struct aa_perms * argument that is only ever
read from and never modified through the pointer. Mark those parameters
const struct aa_perms * to document intent and let the compiler enforce
that the permission set is not mutated.
The converted functions are:
- aa_check_perms()
- aa_do_perms()
- do_perms() (af_inet)
- match_label() (af_unix)
- verify_perm()
- aa_perms_accum() / aa_perms_accum_raw() (@addend only)
No functional change.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Assisted-by: Claude:claude-opus-4.8
|
|
In preparation for allowing unconfined to be replaced and mediate
select rules drop use of the _confined variant for iteration in
the mount code.
The RULE_MEDIATES() check will continue to allow unconfined profile
mediation to be skipped unless it is replaced and is given mount
rules.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Move the mount permissions check to use the common backend
aa_check_perms() to check permissions. This will make it so caching,
audit, complain, logic can be handled consistently in a single place.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
AppArmor only mediates non-binary mount data, and should only
audit the mount data if it is non-binary.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Complain messages can be very noisy and fill the logs quickly. Allow
complain (allow) messages to be silenced separate from denied
messages.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
static tables, and structs that are initialized as part of their
data section or during init should be read only to protect against
accidental or malicous changes.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
checking PTR_ERROR() is not correct to just determine if any error
occured, instead use the IS_ERR macro and also output the PTR_ERR
as part of the debug message.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Instead of keeping an external define for the various tables indexed
by an enum, make the size the last entry of the enum so the table
size will get updated correctly with changes to the enum.
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
label replacement can result in the need for locking on two separate
trees. Currently this is done by locking the tree to remove and then
the tree to add to.
For compound labels the race can result in the old label proxy
pointing to the the new label that lost the race and that was not
inserted in to the new tree. This does not break mediation, but it does
result in a task that will not update its profile correctly on future
mediation, and that will leak its refcount due to a circular reference
in its proxy, resulting in a memory leak.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
The full bprm does not need to be passed into xattrs_match, so only
pass in the path, and propagate the change backup the call stack until
bprm is actually needed.
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
If a task is already confined by a stack the unprivileged transition
restriction on unconfined is not correctly, applied. This results in
an escape if two transitions through an unconfined profile can be
executed.
Fix this by pushing the check into the per profile label build. The
check will always be done against unconfined and result in a stack of
just the unconfined component when necessary.
Fixes: 2d9da9b188b8 ("apparmor: allow restricting unprivileged change_profile")
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Packet mediation is going to be added in the future, reserve a class
for it.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
both of these fns are using ifdef CONFIG_NETWORK_SECMARK and related
to AppArmor's secmark based mediation, so move them together.
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Make sure all the network mediation hooks are next to each other in
a logical block. This just makes it easier to read/understand the
network mediation code.
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Make it easier for distros to support the network backwards compat
patch by refactoring the code to minimize the changes needed.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Continue preparing for fine grained inet mediation by setting up the
stacked mediation callback. This lifts address mapping and audit
context setup out of the stacking loop and pushing the mediation down
into the callback fn.
While this patch sets up the structure for fine grained mediation it
does not change mediation and the callback fns only call the default
mediation that will be used when fine grained inet mediation is not
available.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Refactor network mediation, introducing the stub code for the fine
grained inet mediation. This is a preparatory step and does not change
mediation.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Hide the functionality of determinig unix mediation behind its own fn
so it is easier to adjust the test in the future as it has different
requirements than the other socket mediation.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
copy_from_user does not return an error code and the check should be
setting the error code.
Fixes: 8b236f99edf8 ("apparmor: Initial support for compressed policies")
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
aa_vec_unique() null terminates at vec[n - dups] when VEC_FLAG_TERMINATE
is passed. If the components are all distinct no duplicates are dropped,
dups is 0 and the terminator goes to vec[n], so the caller has to provide
room for n + 1 entries.
aa_label_strn_parse() sets up its vector with vec_setup(profile, vec, len,
gfp) and then calls aa_vec_unique(vec, len, VEC_FLAG_TERMINATE), but
vec_setup() does not reserve the terminator entry. Up to LOCAL_VEC_ENTRIES
it uses the local array of LOCAL_VEC_ENTRIES pointers, above that it
allocates exactly len pointers. The terminator therefore lands one entry
past the end of the local array when len is LOCAL_VEC_ENTRIES, and one
entry past the end of the allocation when len is larger.
len comes from the number of "//&" separated components in the label name
and label_count_strn_entries() does not bound it. An unprivileged task
reaches the parse by writing to /proc/self/attr/apparmor/current or through
lsm_set_self_attr(2), both of which go through do_setattr(), and the name
is parsed before the change_profile permission is checked.
The query_label() path behind the securityfs .access file, which is
mode 0666, performs no permission check at all. Every component has to
resolve to a loaded profile, so a system with policy loaded is required.
The other two VEC_FLAG_TERMINATE users work on a label vec that
aa_label_alloc() has already sized with "+ 1 for null terminator entry on
vec". Reserve the same entry in vec_setup() and DEFINE_VEC(). Passing
len + 1 from the caller instead would move len == LOCAL_VEC_ENTRIES out of
the local array and into kzalloc().
Fixes: f1bd904175e8 ("apparmor: add the base fns() for domain labels")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
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>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
The {begin,end}_current_label_crit_section() has the same issue as the
{__begin,__end} version. That is the check to see if the label has
been updated in the end check forces an unnecessary memory barrier.
We can optimize this the same way we do with the {__begin,__end}
variant by passing in a local variable that carries the state
information from the begin check into the end check.
No functional change.
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
AppArmor's begin_current_label_crit_section() is a scary function called
from lots of LSM hooks (in particular VFS/socket-related ones) that checks
if the label referenced by the current creds is marked FLAG_STALE, and if
so, attempts to use aa_replace_current_label() to replace the creds with an
updated version that uses a new label.
The first problem with this is that it would directly lead to UAF of
`struct cred` if anything in the kernel takes a pointer to the current
creds and accesses these past a security hook invocation that replaces
creds, like so:
```
const struct cred *cred = current_cred();
alloc_file_pseudo(...);
uid_t uid = cred->euid;
```
I don't know if anything in the kernel actually does this, but I think it
is very surprising that this pattern could lead to UAF.
The second problem is that things go wrong when aa_replace_current_label()
runs with overridden credentials. aa_replace_current_label() bails out if
`current_cred() != current_real_cred()` (mirroring the check in
proc_pid_attr_write()), but this check can't actually reliably detect
overridden credentials because the overridden creds can be the same as the
objective creds.
So in approximately the following scenario, things go wrong:
1. task begins with <creds A> (as both objective and subjective creds),
with refcount=2
2. task grabs an extra reference on <creds A> for overriding
3. task calls override_creds(<creds A>), which returns a pointer to the old
subjective creds (<creds A>)
4. task enters AppArmor LSM hook
5. AppArmor checks that objective/subjective creds are equal
6. AppArmor replaces both cred pointers with <creds B> and drops 2 refs on
<creds A>
7. task leaves AppArmor LSM hook
8. task calls revert_creds(<creds A>)
9. now task->cred is <creds A> while task->real_cred is <creds B>, but the
task_struct logically holds two references to <creds B>
10. another task drops the extra reference on <creds A> that was used for
overriding, refcount drops to 0
11. now task->real_cred points to freed creds
At this point, any access to current_cred() will be UAF.
I have a test case where I run aa-disable on a profile while a process
using that profile is blocked on splice() from a FUSE passthrough file into
a full pipe; after the profile update, the pipe becomes empty, splice()
resumes, the credentials go out of sync, and a subsequent getuid() syscall
results in a KASAN UAF splat.
To fix this, instead of directly replacing creds, do it via task_work that
will run at the end of the current syscall. (The point in time at which the
cred replacement happens should have no correctness impact; it is just a
performance optimization to avoid unnecessarily touching the refcount of
the new label.)
Note that AppArmor still performs direct cred replacements in the
sb_pivotroot LSM hook after this change, and that direct cred replacements
can still happen in VFS ->write() callbacks via proc_pid_attr_write().
There are two options for what to do with aa_dup_task_ctx(): Either
explicitly reset new->label_replacement_pending after the entire
aa_task_ctx has been copied, or switch to manually copying members over.
I am switching to manually copying members over because that should make
bugs more obvious.
Cc: stable@vger.kernel.org
Fixes: c75afcd153f6 ("AppArmor: contexts used in attaching policy to system objects")
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
SEND_SIG_NOINFO is defined as ((struct kernel_siginfo *) 0), so passing
NULL works, but:
- this works "by accident" and looks as if the caller doesn't understand
the signal sending API.
- more importantly, this hides the usage of SEND_SIG_NOINFO from grep,
and this is really bad.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
decompress_zstd() is used in two cases:
- CONFIG SECURITY_APPARMOR_COMPRESSED_POLICY: which allows for
compressed policy to be loaded
- CONFIG_SECURITY_APPARMOR_EXPORT_BINARY: which allows introspecting
loaded policy, that is stored in compressed form until it is needed.
When neither of these are selected there is no need for decpress_zstd(),
which results in the decompressed_zstd defined but not used message.
Only define decmpress_zstd() if either of those config options are
enabled. The stub routine is not needed because all calling code is
gated by one of those config options.
Fixes: 1c5f27e845e84 ("apparmor: Fix build failure when ZSTD_DECOMPRESS is not enabled")
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
When CONFIG_ZSTD_DECOMPRESS is not enabled, and neither
CONFIG_SECURITY_APPARMOR_EXPORT_BINARY nor
CONFIG_SECURITY_APPARMOR_COMPRESSED_POLICY are enabled.
The build will fail with implicit declaration of function
'decompress_zstd' because there is not an appropriate stub function,
for when the zstd decompression isn't enabled.
In addition fix compress_min, and compress_max to be conditional on
CONFIG_SECURITY_APPARMOR_EXPORT_BINARY, as they are used with the
exported policy.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202608010834.9yIVzhG2-lkp@intel.com/
Fixes: 1c5f27e845e84 ("apparmor: Fix build failure when ZSTD_DECOMPRESS is not enabled")
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
commit
17b5758bf35c ("apparmor: Initial support for compressed policies")
added the ability for apparmor to load compressed policy, unfortunately
it did not add a config option or select CONFIG_ZSTD_DECOMPRESS
which it depends on, leading to the following build failure
apparmorfs.c makes calls into zstd_*() even when
CONFIG_SECURITY_APPARMOR_EXPORT_BINARY is not set, causing
build errors:
/usr/bin/ld.bfd: security/apparmor/apparmorfs.o: in function `policy_update':
apparmorfs.c:(.text+0x1307): undefined reference to `zstd_get_frame_header'
/usr/bin/ld.bfd: apparmorfs.c:(.text+0x1359): undefined reference to `zstd_dctx_workspace_bound'
/usr/bin/ld.bfd: apparmorfs.c:(.text+0x13f7): undefined reference to `zstd_init_dctx'
/usr/bin/ld.bfd: apparmorfs.c:(.text+0x140c): undefined reference to `zstd_decompress_dctx'
/usr/bin/ld.bfd: apparmorfs.c:(.text+0x1411): undefined reference to `zstd_is_error'
Add a new config option to enable compress policy loading as using
the existing CONFIG_SECURITY_APPARMOR_EXPORT_BINARY is in appropriate
as that is about retaining loaded policy so that it can be introspected
at a later date.
Fixes: 17b5758bf35c ("apparmor: Initial support for compressed policies")
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Warning: security/apparmor/apparmorfs.c:501 function parameter 'compressed_data' not described in 'aa_get_data_from_compressed'
Warning: security/apparmor/apparmorfs.c:501 function parameter 'compressed_data' not described in 'aa_get_data_from_compressed'
Warning: security/apparmor/apparmorfs.c:501 function parameter 'compressed_data' not described in 'aa_get_data_from_compressed'
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202607240144.4iqulDF1-lkp@intel.com/
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Should make harder for MITM to redirect to somewhere else.
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Make use of the audit_log_n_untrustedstring() function to simplify the
code in aa_label_xaudit().
Signed-off-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Fix "undefined symbol: decompress_zstd" error caused by decompress_zstd()
being guarded by CONFIG_SECURITY_APPARMOR_EXPORT_BINARY=y.
Reported-by: syzbot+1f14a35d0c73d31555e4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1f14a35d0c73d31555e4
Fixes: 17b5758bf35c ("apparmor: Initial support for compressed policies")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
When policy is compressed it is given a header in userspace. The
parser knows to strip this header, but alternate profile loaders like
aa-load and systemd have not been updated so that they know to strip
the header.
This allows those loaders to just load the binary file without having
to be updated.
Fixes: 17b5758bf35c7 ("apparmor: Initial support for compressed policies")
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
This patch allows policies to be compressed in userspace and be sent to
the kernel through the existing ".load" and ".replace" kernel interfaces.
The benefits of this approach are:
- Save kernel time when loading policies
- Allow userspace to provide a higher level of compression than the one
provided by the kernel (ZSTD_CLEVEL_DEFAULT), thus saving space.
- Allow small embedded systems to only store the compressed version of
policies in userspace, saving memory.
Userspace-compressed policies improve system time by up to ~30% for big
profiles.
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Besides of resolving clangd IDE warnings, self-contained headers will be
less likely to break if the surrounding includes in .c files using them
change.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
While the #ifdef guards prevent the circular include from blowing up,
policy.h does not actually need anything from net.h. Remove, that include
and instead include net.h in the other files that need it.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
The fix for tcp-fast-open ensures that the connect permission is being
mediated correctly but it didn't add an artifact to the feature set to
advertise the fix is available. Add an artifact so that the test suite
can identify if the fix has not been properly applied or a new
unexpected regression has occurred.
Fixes: 4d587cd8a7215 ("apparmor: mediate the implicit connect of TCP fast open sendmsg")
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
sendmsg()/sendto() with MSG_FASTOPEN is a combination of connect(2) and
write(2): it opens the connection in the SYN. apparmor_socket_sendmsg()
only checks AA_MAY_SEND, so a profile that grants send but denies connect
lets a confined task open an outbound TCP/MPTCP connection that connect(2)
would have refused, bypassing connect mediation.
Mediate the implicit connect when MSG_FASTOPEN is set and a destination
is supplied. Add it to apparmor_socket_sendmsg() (not the shared
aa_sock_msg_perm() helper, which recvmsg also uses) and call aa_sk_perm()
directly, mirroring the selinux and tomoyo fixes. sk_is_tcp() does not
cover MPTCP fast open, so the SOCK_STREAM/IPPROTO_MPTCP arm is explicit.
Fixes: cf60af03ca4e ("net-tcp: Fast Open client - sendmsg(MSG_FASTOPEN)")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Fix error reported by kernel test robot
security/apparmor/policy.c:1381:2: error: a label can only be part of
a statement and a declaration is not a statement
All errors (new ones prefixed by >>):
security/apparmor/policy.c: In function 'aa_replace_profiles':
>> security/apparmor/policy.c:1381:2: error: a label can only be part
of a statement and a declaration is not a statement
ssize_t udata_sz = udata->size;
^~~~~
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202606150525.npax8WiH-lkp@intel.com/
Fixes: 7b42f95813dc9 ("apparmor: fix potential UAF in aa_replace_profiles")
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Fix two kernel-doc warnings:
- non-kernel-doc comment marked with '/**' in af_unix.c
- documented symbol name mismatch for aa_get_i_loaddata() in
policy_unpack.h
No functional changes.
Signed-off-by: Rodrigo Zaiden <rodrigoffzz@gmail.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
multi_transaction_new() allocates memory with get_zeroed_page() and uses
it as struct multi_transaction.
The usage of that structure does not require struct page access and it is
better to allocate multi_transaction objects with kzalloc() that provides
better scalability and more debugging possibilities.
Replace use of get_zeroed_page() with kzalloc().
Link: https://lore.kernel.org/all/635405e4-9423-4a25-a6e7-e03c8ea0bcbe@redhat.com
Reviewed-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Fix two spelling errors in comment:
- interated → interacted
- dont → don't
Signed-off-by: Qingshuang Fu <fuqingshuang@kylinos.cn>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
aa_replace_profiles() walks ns->rawdata_list to dedup the incoming
policy blob against entries already attached to existing profiles.
Per the kernel-doc on struct aa_loaddata, list membership does not
hold a reference: profiles hold pcount, and when the last pcount
drops, do_ploaddata_rmfs() is queued on a workqueue that takes
ns->lock and removes the entry. Between dropping the last pcount
and the workqueue running, an entry remains on the list with
pcount == 0.
aa_get_profile_loaddata() is an unconditional kref_get() on
pcount, so when the dedup loop hits such an entry, refcount
hardening reports
refcount_t: addition on 0; use-after-free.
inside aa_replace_profiles(), and the poisoned counter then
trips "saturated" and "underflow" warnings on the subsequent
uses of the same loaddata.
Before commit a0b7091c4de4 ("apparmor: fix race on rawdata
dereference") the dedup path used a get_unless_zero-style helper
on a single counter, so the existing "if (tmp)" guard was
meaningful. The split-refcount refactor introduced
aa_get_profile_loaddata(), which has plain kref_get() semantics,
and the guard quietly became a no-op.
Introduce aa_get_profile_loaddata_not0(), matching the existing
_not0 convention used by aa_get_profile_not0(), and use it for
the rawdata_list dedup lookup so dying entries are skipped.
Reproduced on x86_64 with v7.1-rc5 in QEMU+KVM running Ubuntu
24.04 + stress-ng 0.17.06:
stress-ng --apparmor 1 --klog-check --timeout 60s
Without this patch the three refcount_t warnings fire within a
few seconds. With it the same 60 s run is clean. Coverage is a
smoke-test only; a longer soak with CONFIG_KASAN, CONFIG_KCSAN
and CONFIG_PROVE_LOCKING would be welcome from anyone with the
cycles.
Fixes: a0b7091c4de4 ("apparmor: fix race on rawdata dereference")
Reported-by: Colin Ian King <colin.i.king@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221513
Cc: stable@vger.kernel.org
Signed-off-by: Ruslan Valiyev <linuxoid@gmail.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
cache_hold_inc() prevents the per-CPU cache hold counter from
rising above MAX_HOLD_COUNT, but the comparison is inverted
(> MAX_HOLD_COUNT instead of <), so the counter never rises
above 0.
This breaks the cache mechanism because since the hold counter
is always 0, the global pool is always attempted first before
falling back to the local cache. The decrement also never occurs,
thus the hold counter is effectively dead.
Fix by changing > to < in cache_hold_inc().
Fixes: 0b6a6b72b329 ("apparmor: document the buffer hold, add an overflow guard")
Signed-off-by: Eduardo Vasconcelos <eduardo@eduardovasconcelos.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
Commit 4a134723f9f1 ("apparmor: move check for aa_null file to cover all cases")
intrdouced a small bug, where path_name() may pass a potentially uninitialized
*name to aa_audit_file() if the path->dentry had been replaced with
aa_null.dentry earlier on. This can lead to page fault like one observed on
7.0.2 openSUSE Tumbleweed kernel:
[51692.242756] [ T24690] BUG: unable to handle page fault for address: 0000000f00000003
[51692.242762] [ T24690] #PF: supervisor read access in kernel mode
[51692.242763] [ T24690] #PF: error_code(0x0000) - not-present page
[51692.242765] [ T24690] PGD 0 P4D 0
[51692.242768] [ T24690] Oops: Oops: 0000 [#1] SMP NOPTI
[51692.242772] [ T24690] CPU: 3 UID: 1020 PID: 24690 Comm: snap-confine Tainted: G O 7.0.2-1-default #1 PREEMPT(full) openSUSE Tumbleweed ab90b4c9940707f9cafa19bdad80b2cec52dbe51
[51692.242775] [ T24690] Tainted: [O]=OOT_MODULE
[51692.242777] [ T24690] Hardware name: Framework Laptop 13 (AMD Ryzen 7040Series)/FRANMDCP05, BIOS 03.18 01/08/2026
[51692.242778] [ T24690] RIP: 0010:strlen+0x4/0x30
[51692.242783] [ T24690] Code: f7 75 ec 31 c0 e9 17 9f 00 ff 48 89 f8 e9 0f 9f 00 ff 0f 1f 40 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa <80> 3f 00 74 18 48 89 f8 0f 1f 40 00 48 83 c0 01 80 38 00 75 f7 48
[51692.242785] [ T24690] RSP: 0018:ffffd015eb1e3608 EFLAGS: 00010282
[51692.242787] [ T24690] RAX: 0000000000000000 RBX: ffff89796198a360 RCX: 0000000000000000
[51692.242788] [ T24690] RDX: 00000000000000d1 RSI: 0000000f00000003 RDI: 0000000f00000003
[51692.242790] [ T24690] RBP: ffffffffb7ede090 R08: 00000000000005f5 R09: 0000000000000000
[51692.242791] [ T24690] R10: 0000000000000000 R11: 0000000000000000 R12: ffffd015eb1e3700
[51692.242792] [ T24690] R13: ffff8977a22bc380 R14: ffffffffb7ec5190 R15: ffff8977a0c8aa80
[51692.242794] [ T24690] FS: 0000000000000000(0000) GS:ffff897f640d8000(0000) knlGS:0000000000000000
[51692.242796] [ T24690] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[51692.242797] [ T24690] CR2: 0000000f00000003 CR3: 00000006ad15f000 CR4: 0000000000f50ef0
[51692.242799] [ T24690] PKRU: 55555554
[51692.242800] [ T24690] Call Trace:
[51692.242802] [ T24690] <TASK>
[51692.242804] [ T24690] audit_log_untrustedstring+0x1d/0x40
[51692.242811] [ T24690] common_lsm_audit+0x71/0x1d0
[51692.242816] [ T24690] aa_audit+0x5a/0x170
[51692.242819] [ T24690] aa_audit_file+0x18a/0x1b0
[51692.242825] [ T24690] path_name+0xd2/0x100
[51692.242829] [ T24690] profile_path_perm.part.0+0x58/0xb0
[51692.242832] [ T24690] aa_path_perm+0xef/0x150
[51692.242837] [ T24690] apparmor_file_open+0x153/0x2e0
[51692.242840] [ T24690] security_file_open+0x46/0xd0
[51692.242844] [ T24690] do_dentry_open+0xe9/0x4d0
[51692.242848] [ T24690] vfs_open+0x30/0x100
While here, initialise variables which are passed down to path_name().
Fixes: 4a134723f9f1 ("apparmor: move check for aa_null file to cover all cases")
Signed-off-by: Maciek Borzecki <maciek.borzecki@gmail.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
|
In
commit 4a134723f9f1 ("apparmor: move check for aa_null file to cover all cases")
there was a change to not audit files pointing to
aa_null.dentry because they provide no value, but setting the error
variable instead of returning -EACCES was still causing them to be
audited.
Fixes: 4a134723f9f1 ("apparmor: move check for aa_null file to cover all cases")
Acked-by: David Disseldorp <ddiss@suse.de>
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
|
