Re: [PATCH] Smack modifications for: security: Allow all LSMs to provide xattrs for inode_init_security hook

[Casey Schaufler <casey@schaufler-ca.com>]

On 5/9/2023 4:44 PM, Mengchi Cheng wrote:
> On Mon, 2023-05-08 12:29:42 +0000, Roberto Sassu wrote:
>> On Thu, 2023-04-20 at 10:48 +0200, Roberto Sassu wrote:
>>> On Wed, 2023-04-19 at 12:25 -0700, Mengchi Cheng wrote:
>>>>> I got some errors during xattr removal, so not sure if my patch was
>>>>> working properly or not (it happened also without it, didn't
>>>>> investigate more).
>>>>>
>>>>> However, I saw another discussion related to transmute:
>>>>>
>>>>> https://lore.kernel.org/linux-security-module/20230419002338.566487-1-mengcc@amazon.com/
>>>>>
>>>>> I add the people in CC.
>>>>>
>>>>> The steps described were so easy to understand and executed, I tried
>>>>> without and with overlayfs.
>>>>>
>>>>> Without:
>>>>>
>>>>> # echo "_ system rwxatl" > /sys/fs/smackfs/load2
>>>>> # mkdir /data
>>>>> # chsmack -a "system" /data
>>>>> # chsmack -t /data
>>>>> # mkdir -p /data/dir1/dir2
>>>>> # chsmack /data/dir1
>>>>> /data/dir1 access="system" transmute="TRUE"
>>>>> # chsmack /data/dir1/dir2
>>>>> /data/dir1/dir2 access="system" transmute="TRUE"
>>>>>
>>>>> It seems to work, right?
>>>>>
>>>>> With overlay fs it didn't work, same result as the one Mengchi
>>>>> reported. Since Mengchi's solution was to set SMK_INODE_CHANGED, and I
>>>>> want to get rid of it, I thought to investigate more.
>>>>>
>>>>> Looking at smack_dentry_create_files_as(), I see that the label of the
>>>>> process is overwritten with the label of the transmuting directory.
>>>>>
>>>>> That causes smack_inode_init_security() to lookup the transmuting rule
>>>>> on the overridden credential, and not on the original one.
>>>>>
>>>>> In the example above, it means that, when overlayfs is creating the new
>>>>> inode, the label of the process is system, not _. So no transmute
>>>>> permission, and also the xattr will not be added, as observed by
>>>>> Mengchi.
>>>>>
>>>>> Hopefully I undertood the code, so in this particular case we would not
>>>>> need to override the label of the process in smack_dentry_create_files_
>>>>> as().
>>>>>
>>>>> If you see smack_inode_init_security():
>>>>>
>>>>> 	struct smack_known *skp = smk_of_current();
>>>>> 	struct smack_known *isp = smk_of_inode(inode);
>>>>> 	struct smack_known *dsp = smk_of_inode(dir);
>>>>>
>>>>> [...]
>>>>>
>>>>> 		if (may > 0 && ((may & MAY_TRANSMUTE) != 0) &&
>>>>> 		    smk_inode_transmutable(dir)) {
>>>>> 			isp = dsp;
>>>>> [...]
>>>>>
>>>>> 		xattr->value = kstrdup(isp->smk_known, GFP_NOFS);
>>>>>
>>>>> This code is telling, if there is a transmute rule, and the directory
>>>>> is transmuting, set the label of the new inode to the label of the
>>>>> directory. That should be already the result that we wanted to obtain.
>>>>>
>>>>> The current code should have been doing it by overriding the label of
>>>>> the process in smack_dentry_create_files_as() with the label of the
>>>>> parent directory, and letting the inode being created with the
>>>>> overridden label of the process. The transmute xattr is not set due to
>>>>> the problem described above.
>>>>>
>>>>> So, as a quick test, I kept this patch with the change to xattr2->name, 
>>>>> and skipped the label override in smack_dentry_create_files_as(). It
>>>>> worked, I get the same result as without overlayfs. Wondering if the
>>>>> process label override is necessary in other cases.
>>>> If I understand correctly, removing the if block below is what you suggested.
>>> Yes, more or less is what I did.
>>>
>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>>> index cfcbb748da25..a867288e9de9 100644
>>>> --- a/security/smack/smack_lsm.c
>>>> +++ b/security/smack/smack_lsm.c
>>>> @@ -4769,8 +4769,8 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
>>>>                  * providing access is transmuting use the containing
>>>>                  * directory label instead of the process label.
>>>>                  */
>>>> -               if (may > 0 && (may & MAY_TRANSMUTE))
>>>> -                       ntsp->smk_task = isp->smk_inode;
>>>> +//             if (may > 0 && (may & MAY_TRANSMUTE))
>>>> +//                     ntsp->smk_task = isp->smk_inode;
>>>>         }
>>>>         return 0;
>>>>  }
>>>>
>>>> This way will have issue in the following situation on the vanila kernel.
>>>> data in the lowerdir has "_" label before overlay and dir1 is already
>>>> created in the lowerdir.
>>>> # chsmack /data
>>>> /data access="_"
>>>> # chsmack /data/dir1
>>>> /data/dir1 access="system" transmute="TRUE"
>>>> Apply overlay on data directory and set the smack rule in the same way.
>>>> data has the same smack label.
>>>> # chsmack /data
>>>> /data access="system" transmute="TRUE"
>>> I'm using an older kernel, but I get _ instead of system.
>>>
>>>> After that, remove dir1 and mkdir dir1 again. dir1 did not get the correct
>>>> label.
>>>> # rm -r /data/dir1
>>>> # mkdir -p /data/dir1
>>>> # chsmack /data/dir1
>>>> /data/dir1 access="_"
>>> Unfortunately, it cannot work:
>>>
>>> Thread 3 hit Breakpoint 1, smack_inode_init_security (...) at security/smack/smack_lsm.c:959
>>> 959	{
>>> (gdb) p dir->i_ino
>>> $12 = 9169116
>>> (gdb) p dsp
>>> $13 = (struct smack_known *) 0xffffffff831fc0a0 <smack_known_floor>
>>>
>>>
>>> ls -i /home/root/data_work/
>>> 9169116 work
>>>
>>> So, transmuting is decided on the working directory.
>> Actually, after studying the code better, this is
>> what security_dentry_create_files_as() is useful for.
>>
>>  * Compute a context for a dentry as the inode is not yet available and set
>>  * that context in passed in creds so that new files are created using that
>>  * context. Context is calculated using the passed in creds and not the creds
>>  * of the caller.
>>
>> And Smack is doing:
>>
>> 		if (may > 0 && (may & MAY_TRANSMUTE)) {
>> 			ntsp->smk_task = isp->smk_inode;
>>
>> The new inode will be created with the label of the current task, that
>> was replaced with the label of the parent directory (see above) in smac
>> k_dentry_create_files_as().
>>
>> I think the reason why Mengchi was not obtaining the desired label when
>> replacing /data/dir1 was because /data is incorrectly labeled.
>>
>> To ensure that /data has label 'system' and transmute is true, I added
>> smackfstransmute=system to the mount options.
>>
>> However, at the beginning, it seemed that it didn't work:
>>
>> # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data
>> # chsmack /data
>> /data access="system"
>>
>> I found that the reason for this is that smack_inode_getsecurity()
>> retrieves metadata from the inode only for SMACK64, and the rest from
>> xattrs (which would not work for mount options). I just made a patch to
>> handle SMACK64TRANSMUTE too.
>>
>> With the patch applied, I correctly get:
>>
>> # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data
>> # chsmack /data
>> /data access="system" transmute="TRUE"
>>
>> With the root inode correctly labeled, I get:
>>
>> # mount -t overlay overlay -o lowerdir=/data,upperdir=/home/root/data,workdir=/home/root/data_work,smackfstransmute=system /data
>> # rm -Rf /data/dir1
>> # mkdir /data/dir1
>> # chsmack /data/dir1
>> /data/dir1 access="system"
>>
>> This is partially correct, transmute="TRUE" is missing.
>>
>> Judging from smk_task, we cannot determine in smack_inode_init_security
>> () if transmuting was successful in smack_dentry_create_files_as(). We
>> need an extra information.
>>
>> Mengchi's solution was to add the new inode as parameter
>> to security_dentry_create_files_as(), so that SMK_INODE_CHANGED can be
>> set in smk_flags, and SMACK64TRANSMUTE is set in smack_d_instantiate().
>>
>> One concern could be that preallocating the inode maybe is overlayfs-
>> specific? A comment also says that we might not use that one:
> security_dentry_create_files_as() is only referred in the fs/overlayfs/dir.c.
> inode is for sure preallocated in this case.
> I am glad you find a way without modifying security api. I did some tests
> with your patch on our system. It works as expected. I am ok to drop mine.

Excellent. Thank you for verifying the patches. I have a few tests of my
own to complete, but I expect to move it into next shortly.

>
> Best,
> Mengchi
>
>> 	err = ovl_create_or_link(dentry, inode, &attr, false);
>> 	/* Did we end up using the preallocated inode? */
>> 	if (inode != d_inode(dentry))
>>
>> We could achieve the same goal without adding a new parameter to security_dentry_create_files_as() and, instead, by adding a new field
>> in the task_smack structure, smk_transmuted, that is set to smk_task
>> when transmuting is successful.
>>
>> Then, if smk_task == smk_transmuted, smack_inode_init_security() would
>> set SMK_INODE_CHANGED. On top of that, I would instead just provide the
>> second xattr SMACK64TRANSMUTE, in addition to SMACK64.
>>
>> Will send the patches for upstream first, and then switch to providing 
>> SMACK64TRANSMUTE in smack_inode_init_security(), in a new version of
>> the EVM patch set.
>>
>> Thanks
>>
>> Roberto
>>
>>> If I do:
>>>
>>> # chsmack -a system -t /home/root/data_work/work/
>>> # mkdir /data/dir1
>>> # chsmack /data/dir1
>>> /data/dir1 access="system" transmute="TRUE"
>>>
>>> I obtain the expected result. However, this problem is due to how overlayfs works:
>>>
>>> static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>>> 				    struct ovl_cattr *cattr)
>>> {
>>>
>>> [...]
>>>
>>> 	newdentry = ovl_create_temp(ofs, workdir, cattr);
>>> 	err = PTR_ERR(newdentry);
>>> 	if (IS_ERR(newdentry))
>>> 		goto out_dput;
>>>
>>>
>>> The good news seems to be that, once you set the label to the correct
>>> directory, transmuting works with the changes I proposed.
>>>
>>> Roberto
>>>
>>>> Since I am not very familiar your change. Could you help check with your
>>>> patch will this issue also happen? 
>>>>
>>>>
>>>> Best,
>>>> Mengchi
>>>>
>>>>>  
>>>>> Roberto
>>