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

[John Johansen]

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