On 5/24/25 16:16, John Johansen wrote:
On 5/19/25 14:56, Gabriel Totev wrote:
On Sat May 17, 2025 at 4:40 AM EDT, John Johansen wrote:
On 4/16/25 15:42, Gabriel Totev wrote:
When using AppArmor profiles inside an unprivileged container,
the link operation observes an unshifted ouid.
(tested with LXD and Incus)
For example, root inside container and uid 1000000 outside, with
`owner /root/link l,` profile entry for ln:
/root$ touch chain && ln chain link
==> dmesg
apparmor="DENIED" operation="link" class="file"
namespace="root//lxd-feet_<var-snap-lxd-common-lxd>" profile="linkit"
name="/root/link" pid=1655 comm="ln" requested_mask="l" denied_mask="l"
fsuid=1000000 ouid=0 [<== should be 1000000] target="/root/chain"
Fix by mapping inode uid of old_dentry in aa_path_link() rather than
using it directly, similarly to how it's mapped in __file_path_perm()
later in the file.
so unfortunately this isn't correct. Yes some mapping needs to be
done but it needs to be relative to different policy namespaces. I
need to spend some time on this
So I think what I am going to do, is pull this in and work on a fix on
top of it. This would at least get us consistent, and mostly right for
the most common use cases and the others are already have difficulties
wrt namespaces atm.
Not sure I understand... I based this patch and its sibling on similar
code for making path_cond structs from the lsm.c functions:
- apparmor_path_rename()
- apparmor_file_open()
- common_perm_cond()
- common_perm_rm()
yes, unfortunately those patches didn't go through me, and are also bad
or more correctly bad dependent on the policy.
Are hard links (and Unix sockets) different in terms of figuring out the
correct uid? Or should all these functions be updated to be relative to
no
policy namespaces? Perhaps they already are and I can't tell? (not sure
what this would look like or how uids would be affected)
yep everything needs to be updated to properly deal with policy namespaces
and stacking.
I'm by no means an AppArmor expert but I'd like to understand and if
possible help get this fixed as it prevent Snaps from running correctly
in LXD/Incus containers. I've built and tested with these patches and it
seems to work: Snaps now don't attract spurious denials and the ouid
from my example above gets the correct value of 1000000 rather than 0.
However, I can't be totally sure I'm not introducing any regressions or
vulnerabilities.
so it depends on the policy being applied and at what level of the policy
stack. There is some long term work that needs to be done here. Its a
bit of a hack but in the short term, I think we can tag each profile with
the correct user and mount namespace and use that for the mappings here.
That should work well enough to address the current bug. I need to spend
some time to do some work on the namespace side, for a longer term fix
If there's anything I can do to help with this effort, please let me know!
Signed-off-by: Gabriel Totev <[email protected]>
---
security/apparmor/file.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/security/apparmor/file.c b/security/apparmor/file.c
index 5c984792cbf0..ecd36199337c 100644
--- a/security/apparmor/file.c
+++ b/security/apparmor/file.c
@@ -430,9 +430,11 @@ int aa_path_link(const struct cred *subj_cred,
{
struct path link = { .mnt = new_dir->mnt, .dentry = new_dentry };
struct path target = { .mnt = new_dir->mnt, .dentry = old_dentry };
+ struct inode *inode = d_backing_inode(old_dentry);
+ vfsuid_t vfsuid = i_uid_into_vfsuid(mnt_idmap(target.mnt), inode);
struct path_cond cond = {
- d_backing_inode(old_dentry)->i_uid,
- d_backing_inode(old_dentry)->i_mode
+ .uid = vfsuid_into_kuid(vfsuid),
+ .mode = inode->i_mode,
};
char *buffer = NULL, *buffer2 = NULL;
struct aa_profile *profile;
