commit 8e135b8aee upstream.
AppArmor was putting the reference to i_private data on its end after
removing the original entry from the file system. However the inode
can aand does live beyond that point and it is possible that some of
the fs call back functions will be invoked after the reference has
been put, which results in a race between freeing the data and
accessing it through the fs.
While the rawdata/loaddata is the most likely candidate to fail the
race, as it has the fewest references. If properly crafted it might be
possible to trigger a race for the other types stored in i_private.
Fix this by moving the put of i_private referenced data to the correct
place which is during inode eviction.
Fixes: c961ee5f21 ("apparmor: convert from securityfs to apparmorfs for policy ns files")
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>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit a0b7091c4d upstream.
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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 6601e13e82 upstream.
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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Pull AppArmor updates from John Johansen:
"This adds a single feature, switch the hash used to check policy from
sha1 to sha256
There are fixes for two memory leaks, and refcount bug and a potential
crash when a profile name is empty. Along with a couple minor code
cleanups.
Summary:
Features
- switch policy hash from sha1 to sha256
Bug Fixes
- Fix refcount leak in task_kill
- Fix leak of pdb objects and trans_table
- avoid crash when parse profie name is empty
Cleanups
- add static to stack_msg and nulldfa
- more kernel-doc cleanups"
* tag 'apparmor-pr-2024-01-18' of git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor:
apparmor: Fix memory leak in unpack_profile()
apparmor: avoid crash when parsed profile name is empty
apparmor: fix possible memory leak in unpack_trans_table
apparmor: free the allocated pdb objects
apparmor: Fix ref count leak in task_kill
apparmor: cleanup network hook comments
apparmor: add missing params to aa_may_ptrace kernel-doc comments
apparmor: declare nulldfa as static
apparmor: declare stack_msg as static
apparmor: switch SECURITY_APPARMOR_HASH from sha1 to sha256
Pull misc filesystem updates from Al Viro:
"Misc cleanups (the part that hadn't been picked by individual fs
trees)"
* tag 'pull-misc' of git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs:
apparmorfs: don't duplicate kfree_link()
orangefs: saner arguments passing in readdir guts
ocfs2_find_match(): there's no such thing as NULL or negative ->d_parent
reiserfs_add_entry(): get rid of pointless namelen checks
__ocfs2_add_entry(), ocfs2_prepare_dir_for_insert(): namelen checks
ext4_add_entry(): ->d_name.len is never 0
befs: d_obtain_alias(ERR_PTR(...)) will do the right thing
affs: d_obtain_alias(ERR_PTR(...)) will do the right thing
/proc/sys: use d_splice_alias() calling conventions to simplify failure exits
hostfs: use d_splice_alias() calling conventions to simplify failure exits
udf_fiiter_add_entry(): check for zero ->d_name.len is bogus...
udf: d_obtain_alias(ERR_PTR(...)) will do the right thing...
udf: d_splice_alias() will do the right thing on ERR_PTR() inode
nfsd: kill stale comment about simple_fill_super() requirements
bfs_add_entry(): get rid of pointless ->d_name.len checks
nilfs2: d_obtain_alias(ERR_PTR(...)) will do the right thing...
zonefs: d_splice_alias() will do the right thing on ERR_PTR() inode
Prevent move_mount from applying the attach_disconnected flag
to move_mount(). This prevents detached mounts from appearing
as / when applying mount mediation, which is not only incorrect
but could result in bad policy being generated.
Basic mount rules like
allow mount,
allow mount options=(move) -> /target/,
will allow detached mounts, allowing older policy to continue
to function. New policy gains the ability to specify `detached` as
a source option
allow mount detached -> /target/,
In addition make sure support of move_mount is advertised as
a feature to userspace so that applications that generate policy
can respond to the addition.
Note: this fixes mediation of move_mount when a detached mount is used,
it does not fix the broader regression of apparmor mediation of
mounts under the new mount api.
Link: https://lore.kernel.org/all/68c166b8-5b4d-4612-8042-1dee3334385b@leemhuis.info/T/#mb35fdde37f999f08f0b02d58dc1bf4e6b65b8da2
Fixes: 157a3537d6 ("apparmor: Fix regression in mount mediation")
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
sha1 is insecure and has colisions, thus it is not useful for even
lightweight policy hash checks. Switch to sha256, which on modern
hardware is fast enough.
Separately as per NIST Policy on Hash Functions, sha1 usage must be
withdrawn by 2030. This config option currently is one of many that
holds up sha1 usage.
Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Pull apparmor updates from John Johansen:
"This adds initial support for mediating io_uring and userns creation.
Adds a new restriction that tightens the use of change_profile, and a
couple of optimizations to reduce performance bottle necks that have
been found when retrieving the current task's secid and allocating
work buffers.
The majority of the patch set continues cleaning up and simplifying
the code (fixing comments, removing now dead functions, and macros
etc). Finally there are 4 bug fixes, with the regression fix having
had a couple months of testing.
Features:
- optimize retrieving current task secid
- add base io_uring mediation
- add base userns mediation
- improve buffer allocation
- allow restricting unprivilege change_profile
Cleanups:
- Fix kernel doc comments
- remove unused declarations
- remove unused functions
- remove unneeded #ifdef
- remove unused macros
- mark fns static
- cleanup fn with unused return values
- cleanup audit data
- pass cred through to audit data
- refcount the pdb instead of using duplicates
- make SK_CTX macro an inline fn
- some comment cleanups
Bug fixes:
- fix regression in mount mediation
- fix invalid refenece
- use passed in gfp flags
- advertise avaiability of extended perms and disconnected.path"
* tag 'apparmor-pr-2023-11-03' of git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor: (39 commits)
apparmor: Fix some kernel-doc comments
apparmor: Fix one kernel-doc comment
apparmor: Fix some kernel-doc comments
apparmor: mark new functions static
apparmor: Fix regression in mount mediation
apparmor: cache buffers on percpu list if there is lock contention
apparmor: add io_uring mediation
apparmor: add user namespace creation mediation
apparmor: allow restricting unprivileged change_profile
apparmor: advertise disconnected.path is available
apparmor: refcount the pdb
apparmor: provide separate audit messages for file and policy checks
apparmor: pass cred through to audit info.
apparmor: rename audit_data->label to audit_data->subj_label
apparmor: combine common_audit_data and apparmor_audit_data
apparmor: rename SK_CTX() to aa_sock and make it an inline fn
apparmor: Optimize retrieving current task secid
apparmor: remove unused functions in policy_ns.c/.h
apparmor: remove unneeded #ifdef in decompress_zstd()
apparmor: fix invalid reference on profile->disconnected
...
Unprivileged user namespace creation is often used as a first step
in privilege escalation attacks. Instead of disabling it at the
sysrq level, which blocks its legitimate use as for setting up a sandbox,
allow control on a per domain basis.
This allows an admin to quickly lock down a system while also still
allowing legitimate use.
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.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>
While disconnected.path has been available for a while it was never
properly advertised as a feature. Fix this so that userspace doesn't
need special casing to handle it.
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>
The whole function is guarded by CONFIG_SECURITY_APPARMOR_EXPORT_BINARY,
so the #ifdef here is redundant, remove it.
Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Userspace won't load policy using extended perms unless it knows the
kernel can handle them. Advertise that extended perms are supported in
the feature set.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Reviewed-by: Jon Tourville <jontourville@me.com>
Convert to struct mnt_idmap.
Last cycle we merged the necessary infrastructure in
256c8aed2b ("fs: introduce dedicated idmap type for mounts").
This is just the conversion to struct mnt_idmap.
Currently we still pass around the plain namespace that was attached to a
mount. This is in general pretty convenient but it makes it easy to
conflate namespaces that are relevant on the filesystem with namespaces
that are relevent on the mount level. Especially for non-vfs developers
without detailed knowledge in this area this can be a potential source for
bugs.
Once the conversion to struct mnt_idmap is done all helpers down to the
really low-level helpers will take a struct mnt_idmap argument instead of
two namespace arguments. This way it becomes impossible to conflate the two
eliminating the possibility of any bugs. All of the vfs and all filesystems
only operate on struct mnt_idmap.
Acked-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
The rawdata readback has a few of problems. First if compression is
enabled when the data is read then the compressed data is read out
instead decompressing the data. Second if compression of the data
fails, the code does not handle holding onto the raw_data in
uncompressed form. Third if the compression is enabled/disabled after
the rawdata was loaded, the check against the global control of
whether to use compression does not reflect what was already done to
the data.
Fix these by always storing the compressed size, along with the
original data size even if compression fails or is not used. And use
this to detect whether the rawdata is actually compressed.
Fixes: 52ccc20c652b ("apparmor: use zstd compression for profile data")
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Jon Tourville <jon.tourville@canonical.com>
Unfortunately the switch to using zstd compression did not properly
ifdef all the code that uses zstd_ symbols. So that if exporting of
binary policy is disabled in the config the compile will fail with the
following errors
security/apparmor/lsm.c:1545: undefined reference to `zstd_min_clevel'
aarch64-linux-ld: security/apparmor/lsm.c:1545: undefined reference to `zstd_max_clevel'
Reported-by: kernel test robot <lkp@intel.com>
Fixes: 52ccc20c652b ("apparmor: use zstd compression for profile data")
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Jon Tourville <jon.tourville@canonical.com>
The decompress ctx was not properly initialized when reading raw
profile data back to userspace.
Reported-by: kernel test robot <lkp@intel.com>
Fixes: 52ccc20c652b ("apparmor: use zstd compression for profile data")
Signed-off-by: John Johansen <john.johansen@canonical.com>
Convert profile->rules to a list as the next step towards supporting
multiple rulesets in a profile. For this step only support a single
list entry item. The logic for iterating the list will come as a
separate step.
Signed-off-by: John Johansen <john.johansen@canonical.com>
In preparation for moving from a single set of rules and a single
attachment to multiple rulesets and attachments separate from the
profile refactor attachment information and ruleset info into their
own structures.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Convert from an unsigned int to a state_t for state position. This is
a step in prepping for the state position carrying some additional
flags, and a limited form of backtracking to support variables.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Remap polidydb dfa accept table from embedded perms to an index, and
then move the perm lookup to use the accept entry as an index into the
perm table. This is done so that the perm table can be separated from
the dfa, allowing dfa accept to index to share expanded permission
sets.
Signed-off-by: John Johansen <john.johansen@canonical.com>
continue permission unification by converting xmatch to use the
policydb struct that is used by the other profile dfas.
Signed-off-by: John Johansen <john.johansen@canonical.com>
file_rules and policydb are almost the same and will need the same
features in the future so combine them.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Rather than computing policydb permissions for each access
permissions can be computed once on profile load and stored for lookup.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Rather than computing file permissions for each file access, file
permissions can be computed once on profile load and stored for lookup.
Signed-off-by: Mike Salvatore <mike.salvatore@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Create two new files in apparmor's sysfs:
/sys/kernel/security/apparmor/raw_data_compression_level_min
/sys/kernel/security/apparmor/raw_data_compression_level_max
These correspond to the minimum and maximum zstd compression levels
that can be assigned to the apparmor module parameter
raw_data_compression_level.
Signed-off-by: Jon Tourville <jon.tourville@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Change the algorithm used by apparmor to compress profile data from
zlib to zstd, using the new zstd API introduced in 5.16.
Zstd provides a larger range of compression levels than zlib and
significantly better performance at the default level (for a relatively
small increase in compressed size).
The apparmor module parameter raw_data_compression_level is now clamped
to the minimum and maximum compression levels reported by the zstd
library. A compression level of 0 retains the previous behavior of
disabling policy compression instead of using zstd's behavior, which is
to use the default compression level.
Signed-off-by: Jon Tourville <jon.tourville@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
In multi_transaction_new(), the variable t is not freed or passed out
on the failure of copy_from_user(t->data, buf, size), which could lead
to a memleak.
Fix this bug by adding a put_multi_transaction(t) in the error path.
Fixes: 1dea3b41e8 ("apparmor: speed up transactional queries")
Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
AppArmor split out task oriented controls to their own logical file
a while ago. Ptrace mediation is better grouped with task than
ipc, so move it.
Signed-off-by: John Johansen <john.johansen@canonical.com>
When copy_from_user failed, the memory is freed by kvfree. however the
management struct and data blob are allocated independently, so only
kvfree(data) cause a memleak issue here. Use aa_put_loaddata(data) to
fix this issue.
Fixes: a6a52579e5 ("apparmor: split load data into management struct and data blob")
Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
IF CONFIG_SECURITY_APPARMOR_EXPORT_BINARY is disabled, there remains
some unneed references to zlib, and can result in undefined symbol
references if ZLIB_INFLATE or ZLIB_DEFLATE are not defined.
Reported-by: kernel test robot <lkp@intel.com>
Fixes: abfb9c0725f2 ("apparmor: make export of raw binary profile to userspace optional")
Signed-off-by: John Johansen <john.johansen@canonical.com>
Don't use /** for non-kernel-doc comments and change function name
aa_mangle_name to mangle_name in kernel-doc comment to Remove some
warnings found by running scripts/kernel-doc, which is caused by
using 'make W=1'.
security/apparmor/apparmorfs.c:1503: warning: Cannot understand *
on line 1503 - I thought it was a doc line
security/apparmor/apparmorfs.c:1530: warning: Cannot understand *
on line 1530 - I thought it was a doc line
security/apparmor/apparmorfs.c:1892: warning: Cannot understand *
on line 1892 - I thought it was a doc line
security/apparmor/apparmorfs.c:108: warning: expecting prototype for
aa_mangle_name(). Prototype was for mangle_name() instead
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Currently if sha1 hashing of policy is disabled a sha1 hash symlink
to the non-existent file is created. There is now reason to create
the symlink in this case so don't do it.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Embedded systems have limited space and don't need the introspection
or checkpoint restore capability provided by exporting the raw
profile binary data so make it so make it a config option.
This will reduce run time memory use and also speed up policy loads.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Pull apparmor updates from John Johansen:
"Features
- use per file locks for transactional queries
- update policy management capability checks to work with LSM stacking
Bug Fixes:
- check/put label on apparmor_sk_clone_security()
- fix error check on update of label hname
- fix introspection of of task mode for unconfined tasks
Cleanups:
- avoid -Wempty-body warning
- remove duplicated 'Returns:' comments
- fix doc warning
- remove unneeded one-line hook wrappers
- use struct_size() helper in kzalloc()
- fix zero-length compiler warning in AA_BUG()
- file.h: delete duplicated word
- delete repeated words in comments
- remove repeated declaration"
* tag 'apparmor-pr-2021-11-10' of git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor:
apparmor: remove duplicated 'Returns:' comments
apparmor: remove unneeded one-line hook wrappers
apparmor: Use struct_size() helper in kzalloc()
apparmor: fix zero-length compiler warning in AA_BUG()
apparmor: use per file locks for transactional queries
apparmor: fix doc warning
apparmor: Remove the repeated declaration
apparmor: avoid -Wempty-body warning
apparmor: Fix internal policy capable check for policy management
apparmor: fix error check
security: apparmor: delete repeated words in comments
security: apparmor: file.h: delete duplicated word
apparmor: switch to apparmor to internal capable check for policy management
apparmor: update policy capable checks to use a label
apparmor: fix introspection of of task mode for unconfined tasks
apparmor: check/put label on apparmor_sk_clone_security()
As made mention of in commit 1dea3b41e8 ("apparmor: speed up
transactional queries"), a single lock is currently used to synchronize
transactional queries. We can, use the lock allocated for each file by
VFS instead.
Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Fix gcc W=1 warning:
security/apparmor/apparmorfs.c:2125: warning: Function parameter or member 'p' not described in '__next_profile'
Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Previously the policy capable checks assumed they were using the
current task. Make them take the task label so the query can be
made against an arbitrary task.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Extend some inode methods with an additional user namespace argument. A
filesystem that is aware of idmapped mounts will receive the user
namespace the mount has been marked with. This can be used for
additional permission checking and also to enable filesystems to
translate between uids and gids if they need to. We have implemented all
relevant helpers in earlier patches.
As requested we simply extend the exisiting inode method instead of
introducing new ones. This is a little more code churn but it's mostly
mechanical and doesnt't leave us with additional inode methods.
Link: https://lore.kernel.org/r/20210121131959.646623-25-christian.brauner@ubuntu.com
Cc: Christoph Hellwig <hch@lst.de>
Cc: David Howells <dhowells@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
