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

[John Johansen]

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
> 
> 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 <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;
>