[PATCH 1/2] apparmor: shift ouid when mediating hard links in userns

[Gabriel Totev]

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

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()

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
policy namespaces? Perhaps they already are and I can't tell? (not sure
what this would look like or how uids would be affected)

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.

If there's anything I can do to help with this effort, please let me know!

> > 
> > Signed-off-by: Gabriel Totev <gabriel.totev at zetier.com>
> > ---
> >   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;