There is a race condition that leads to a use-after-free situation:
because the rawdata inodes are not refcounted, an attacker can start
open()ing one of the rawdata files, and at the same time remove the
last reference to this rawdata (by removing the corresponding profile,
for example), which frees its struct aa_loaddata; as a result, when
seq_rawdata_open() is reached, i_private is a dangling pointer and
freed memory is accessed.
The rawdata inodes weren't refcounted to avoid a circular refcount and
were supposed to be held by the profile rawdata reference. However
during profile removal there is a window where the vfs and profile
destruction race, resulting in the use after free.
Fix this by moving to a double refcount scheme. Where the profile
refcount on rawdata is used to break the circular dependency. Allowing
for freeing of the rawdata once all inode references to the rawdata
are put.
Fixes: 5d5182cae4 ("apparmor: move to per loaddata files, instead of replicating in profiles")
Reported-by: Qualys Security Advisory <qsa@qualys.com>
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Reviewed-by: Maxime Bélair <maxime.belair@canonical.com>
Reviewed-by: Cengiz Can <cengiz.can@canonical.com>
Tested-by: Salvatore Bonaccorso <carnil@debian.org>
Signed-off-by: John Johansen <john.johansen@canonical.com>
An unprivileged local user can load, replace, and remove profiles by
opening the apparmorfs interfaces, via a confused deputy attack, by
passing the opened fd to a privileged process, and getting the
privileged process to write to the interface.
This does require a privileged target that can be manipulated to do
the write for the unprivileged process, but once such access is
achieved full policy management is possible and all the possible
implications that implies: removing confinement, DoS of system or
target applications by denying all execution, by-passing the
unprivileged user namespace restriction, to exploiting kernel bugs for
a local privilege escalation.
The policy management interface can not have its permissions simply
changed from 0666 to 0600 because non-root processes need to be able
to load policy to different policy namespaces.
Instead ensure the task writing the interface has privileges that
are a subset of the task that opened the interface. This is already
done via policy for confined processes, but unconfined can delegate
access to the opened fd, by-passing the usual policy check.
Fixes: b7fd2c0340 ("apparmor: add per policy ns .load, .replace, .remove interface files")
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: John Johansen <john.johansen@canonical.com>
if ns_name is NULL after
1071 error = aa_unpack(udata, &lh, &ns_name);
and if ent->ns_name contains an ns_name in
1089 } else if (ent->ns_name) {
then ns_name is assigned the ent->ns_name
1095 ns_name = ent->ns_name;
however ent->ns_name is freed at
1262 aa_load_ent_free(ent);
and then again when freeing ns_name at
1270 kfree(ns_name);
Fix this by NULLing out ent->ns_name after it is transferred to ns_name
Fixes: 145a0ef21c ("apparmor: fix blob compression when ns is forced on a policy load
")
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: John Johansen <john.johansen@canonical.com>
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: c88d4c7b04 ("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>
This is the result of running the Coccinelle script from
scripts/coccinelle/api/kmalloc_objs.cocci. The script is designed to
avoid scalar types (which need careful case-by-case checking), and
instead replace kmalloc-family calls that allocate struct or union
object instances:
Single allocations: kmalloc(sizeof(TYPE), ...)
are replaced with: kmalloc_obj(TYPE, ...)
Array allocations: kmalloc_array(COUNT, sizeof(TYPE), ...)
are replaced with: kmalloc_objs(TYPE, COUNT, ...)
Flex array allocations: kmalloc(struct_size(PTR, FAM, COUNT), ...)
are replaced with: kmalloc_flex(*PTR, FAM, COUNT, ...)
(where TYPE may also be *VAR)
The resulting allocations no longer return "void *", instead returning
"TYPE *".
Signed-off-by: Kees Cook <kees@kernel.org>
aa_free_data() and free_attachment() don't guard against having
a NULL parameter passed to them. Fix this.
Reviewed-by: Ryan Lee <ryan.lee@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Add support for the per permission tag index for a given permission
set. This will be used by both meta-data tagging, to allow annotating
accept states with context and debug information. As well as by rule
tainting and triggers to specify the taint or trigger to be applied.
Since these are low frequency ancillary data items they are stored
in a tighter packed format to that allows for sharing and reuse of the
strings between permissions and accept states. Reducing the amount of
kernel memory use at the cost of having to go through a couple if
index based indirections.
The tags are just strings that has no meaning with out context. When
used as meta-data for auditing and debugging its entirely information
for userspace, but triggers, and tainting can be used to affect the
domain. However they all exist in the same packed data set and can
be shared between different uses.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The strtable is currently limited to a single entry string on unpack
even though domain has the concept of multiple entries within it. Make
this a reality as it will be used for tags and more advanced domain
transitions.
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Replace unbounded sprintf() calls with snprintf() to prevent potential
buffer overflows in aa_new_learning_profile(). While the current code
works correctly, snprintf() is safer and follows secure coding best
practices. No functional changes.
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
Signed-off-by: John Johansen <john.johansen@canonical.com>
The set of rules on a profile is not dynamically extended, instead
if a new ruleset is needed a new version of the profile is created.
This allows us to use a vector of rules instead of a list, slightly
reducing memory usage and simplifying the code.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Same as aa_get_newest_cred_label_condref().
This avoids a bunch of work overall and allows the compiler to note when no
clean up is necessary, allowing for tail calls.
This in particular happens in apparmor_file_permission(), which manages to
tail call aa_file_perm() 105 bytes in (vs a regular call 112 bytes in
followed by branches to figure out if clean up is needed).
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Previously apparmor has only sent SIGKILL but there are cases where
it can be useful to send a different signal. Allow the profile
to optionally specify a different value.
Signed-off-by: John Johansen <john.johansen@canonical.com>
In order to speed up the mediated check, precompute and store the
result as a bit per class type. This will not only allow us to
speed up the mediation check but is also a step to removing the
unconfined special cases as the unconfined check can be replaced
with the generic label_mediates() check.
Note: label check does not currently work for capabilities and resources
which need to have their mediation updated first.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Make it so apparmor debug output can be controlled by class flags
as well as the debug flag on labels. This provides much finer
control at what is being output so apparmor doesn't flood the
logs with information that is not needed, making it hard to find
what is important.
Signed-off-by: John Johansen <john.johansen@canonical.com>
aa_label_audit, aa_label_find, aa_label_seq_print and aa_update_label_name
were added by commit
f1bd904175 ("apparmor: add the base fns() for domain labels")
but never used.
aa_profile_label_perm was added by commit
637f688dc3 ("apparmor: switch from profiles to using labels on contexts")
but never used.
aa_secid_update was added by commit
c092921219 ("apparmor: add support for mapping secids and using secctxes")
but never used.
aa_split_fqname has been unused since commit
3664268f19 ("apparmor: add namespace lookup fns()")
aa_lookup_profile has been unused since commit
93c98a484c ("apparmor: move exec domain mediation to using labels")
aa_audit_perms_cb was only used by aa_profile_label_perm (see above).
All of these commits are from around 2017.
Remove them.
Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Since kvfree() already checks if its argument is NULL, an additional
check before calling kvfree() is unnecessary and can be removed.
Remove it and the following Coccinelle/coccicheck warning reported by
ifnullfree.cocci:
WARNING: NULL check before some freeing functions is not needed
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
Signed-off-by: John Johansen <john.johansen@canonical.com>
attach->xmatch was not set when allocating a null profile, which is used in
complain mode to allocate a learning profile. This was causing downstream
failures in find_attach, which expected a valid xmatch but did not find
one under a certain sequence of profile transitions in complain mode.
This patch ensures the xmatch is set up properly for null profiles.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Inside unpack_profile() data->data is allocated using kvmemdup() so it
should be freed with the corresponding kvfree_sensitive().
Also add missing data->data release for rhashtable insertion failure path
in unpack_profile().
Found by Linux Verification Center (linuxtesting.org).
Fixes: e025be0f26 ("apparmor: support querying extended trusted helper extra data")
Cc: stable@vger.kernel.org
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Fix some kernel-doc comments to silence the warnings:
security/apparmor/policy.c:117: warning: Function parameter or member 'kref' not described in 'aa_pdb_free_kref'
security/apparmor/policy.c:117: warning: Excess function parameter 'kr' description in 'aa_pdb_free_kref'
security/apparmor/policy.c:882: warning: Function parameter or member 'subj_cred' not described in 'aa_may_manage_policy'
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=7037
Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
unprivileged unconfined can use change_profile to alter the confinement
set by the mac admin.
Allow restricting unprivileged unconfined by still allowing change_profile
but stacking the change against unconfined. This allows unconfined to
still apply system policy but allows the task to enter the new confinement.
If unprivileged unconfined is required a sysctl is provided to switch
to the previous behavior.
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
With the move to permission tables the dfa is no longer a stand
alone entity when used, needing a minimum of a permission table.
However it still could be shared among different pdbs each using
a different permission table.
Instead of duping the permission table when sharing a pdb, add a
refcount to the pdb so it can be easily shared.
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
The cred is needed to properly audit some messages, and will be needed
in the future for uid conditional mediation. So pass it through to
where the apparmor_audit_data struct gets defined.
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
John Johansen
Massimiliano PellizzerandJohn Johansen
Kees Cook
Thorsten BlumandJohn Johansen
Mateusz GuzikandJohn Johansen
Tanya AgarwalandJohn Johansen
Dr. David Alan GilbertandJohn Johansen
Ryan LeeandJohn Johansen
Fedor PchelkinandJohn Johansen
Yang LiandJohn Johansen