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

[Casey Schaufler]

On 4/19/2023 6:46 AM, Roberto Sassu wrote:
> On Tue, 2023-04-18 at 09:02 -0700, Casey Schaufler wrote:
>> On 4/18/2023 12:05 AM, Roberto Sassu wrote:
>>> On Mon, 2023-04-17 at 09:41 -0700, Casey Schaufler wrote:
>>>> On 4/13/2023 12:11 AM, Roberto Sassu wrote:
>>>>> On Wed, 2023-04-12 at 13:29 -0700, Casey Schaufler wrote:
>>>>>> On 4/12/2023 12:22 AM, Roberto Sassu wrote:
>>>>>>> On Tue, 2023-04-11 at 10:54 -0700, Casey Schaufler wrote:
>>>>>>>> On 4/11/2023 10:23 AM, Roberto Sassu wrote:
>>>>>>>>> From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
>>>>>>>>>
>>>>>>>>> Very very quick modification. Not tested.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
>>>>>>>>> ---
>>>>>>>>>  security/smack/smack.h     |  2 +-
>>>>>>>>>  security/smack/smack_lsm.c | 42 ++++++++++++++++++++------------------
>>>>>>>>>  2 files changed, 23 insertions(+), 21 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/security/smack/smack.h b/security/smack/smack.h
>>>>>>>>> index e2239be7bd6..f00c8498c60 100644
>>>>>>>>> --- a/security/smack/smack.h
>>>>>>>>> +++ b/security/smack/smack.h
>>>>>>>>> @@ -127,7 +127,7 @@ struct task_smack {
>>>>>>>>>  
>>>>>>>>>  #define	SMK_INODE_INSTANT	0x01	/* inode is instantiated */
>>>>>>>>>  #define	SMK_INODE_TRANSMUTE	0x02	/* directory is transmuting */
>>>>>>>>> -#define	SMK_INODE_CHANGED	0x04	/* smack was transmuted */
>>>>>>>>> +#define	SMK_INODE_CHANGED	0x04	/* smack was transmuted (unused) */
>>>>>>>> See below ...
>>>>>>>>
>>>>>>>>>  #define	SMK_INODE_IMPURE	0x08	/* involved in an impure transaction */
>>>>>>>>>  
>>>>>>>>>  /*
>>>>>>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>>>>>>>> index 8392983334b..b43820bdbd0 100644
>>>>>>>>> --- a/security/smack/smack_lsm.c
>>>>>>>>> +++ b/security/smack/smack_lsm.c
>>>>>>>>> @@ -54,12 +54,12 @@
>>>>>>>>>  
>>>>>>>>>  /*
>>>>>>>>>   * Smack uses multiple xattrs.
>>>>>>>>> - * SMACK64 - for access control, SMACK64EXEC - label for the program,
>>>>>>>>> - * SMACK64MMAP - controls library loading,
>>>>>>>>> + * SMACK64 - for access control,
>>>>>>>>>   * SMACK64TRANSMUTE - label initialization,
>>>>>>>>> - * Not saved on files - SMACK64IPIN and SMACK64IPOUT
>>>>>>>>> + * Not saved on files - SMACK64IPIN and SMACK64IPOUT,
>>>>>>>>> + * Must be set explicitly - SMACK64EXEC and SMACK64MMAP
>>>>>>>>>   */
>>>>>>>>> -#define SMACK_INODE_INIT_XATTRS 4
>>>>>>>>> +#define SMACK_INODE_INIT_XATTRS 2
>>>>>>>>>  
>>>>>>>>>  #ifdef SMACK_IPV6_PORT_LABELING
>>>>>>>>>  static DEFINE_MUTEX(smack_ipv6_lock);
>>>>>>>>> @@ -957,11 +957,11 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>>>>>>>>>  				     const struct qstr *qstr,
>>>>>>>>>  				     struct xattr *xattrs, int *xattr_count)
>>>>>>>>>  {
>>>>>>>>> -	struct inode_smack *issp = smack_inode(inode);
>>>>>>>>>  	struct smack_known *skp = smk_of_current();
>>>>>>>>>  	struct smack_known *isp = smk_of_inode(inode);
>>>>>>>>>  	struct smack_known *dsp = smk_of_inode(dir);
>>>>>>>>>  	struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count);
>>>>>>>>> +	struct xattr *xattr2;
>>>>>>>> I'm going to channel Paul and suggest this be xattr_transmute instead of xattr2.
>>>>>>>> It also looks like it could move to be declared in the if clause.
>>>>>>>>
>>>>>>>>>  	int may;
>>>>>>>>>  
>>>>>>>>>  	if (xattr) {
>>>>>>>>> @@ -979,7 +979,17 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>>>>>>>>>  		if (may > 0 && ((may & MAY_TRANSMUTE) != 0) &&
>>>>>>>>>  		    smk_inode_transmutable(dir)) {
>>>>>>>>>  			isp = dsp;
>>>>>>>>> -			issp->smk_flags |= SMK_INODE_CHANGED;
>>>>>>>> I think you need to keep this. More below.
>>>>>>>>
>>>>>>>>> +			xattr2 = lsm_get_xattr_slot(xattrs, xattr_count);
>>>>>>>>> +			if (xattr2) {
>>>>>>>>> +				xattr2->value = kmemdup(TRANS_TRUE,
>>>>>>>>> +							TRANS_TRUE_SIZE,
>>>>>>>>> +							GFP_NOFS);
>>>>>>>>> +				if (xattr2->value == NULL)
>>>>>>>>> +					return -ENOMEM;
>>>>>>>>> +
>>>>>>>>> +				xattr2->value_len = TRANS_TRUE_SIZE;
>>>>>>>>> +				xattr2->name = XATTR_NAME_SMACKTRANSMUTE;
>>>>>>>>> +			}
>>>>>>>>>  		}
>>>>>>>>>  
>>>>>>>>>  		xattr->value = kstrdup(isp->smk_known, GFP_NOFS);
>>>>>>>>> @@ -3512,20 +3522,12 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>>>>>>>>>  			 * If there is a transmute attribute on the
>>>>>>>>>  			 * directory mark the inode.
>>>>>>>>>  			 */
>>>>>>>>> -			if (isp->smk_flags & SMK_INODE_CHANGED) {
>>>>>>>>> -				isp->smk_flags &= ~SMK_INODE_CHANGED;
>>>>>>>>> -				rc = __vfs_setxattr(&nop_mnt_idmap, dp, inode,
>>>>>>>>> -					XATTR_NAME_SMACKTRANSMUTE,
>>>>>>>>> -					TRANS_TRUE, TRANS_TRUE_SIZE,
>>>>>>>>> -					0);
>>>>>>>>> -			} else {
>>>>>>>>> -				rc = __vfs_getxattr(dp, inode,
>>>>>>>>> -					XATTR_NAME_SMACKTRANSMUTE, trattr,
>>>>>>>>> -					TRANS_TRUE_SIZE);
>>>>>>>>> -				if (rc >= 0 && strncmp(trattr, TRANS_TRUE,
>>>>>>>>> -						       TRANS_TRUE_SIZE) != 0)
>>>>>>>>> -					rc = -EINVAL;
>>>>>>>>> -			}
>>>>>>>>> +			rc = __vfs_getxattr(dp, inode,
>>>>>>>>> +					    XATTR_NAME_SMACKTRANSMUTE, trattr,
>>>>>>>>> +					    TRANS_TRUE_SIZE);
>>>>>>>>> +			if (rc >= 0 && strncmp(trattr, TRANS_TRUE,
>>>>>>>>> +					       TRANS_TRUE_SIZE) != 0)
>>>>>>>>> +				rc = -EINVAL;
>>>>>>>> Where is the SMACK64_TRANSMUTE attribute going to get set on the file?
>>>>>>>> It's not going to get set in smack_init_inode_security(). The inode will
>>>>>>> Isn't that the purpose of the inode_init_security hook?
>>>>>> No. It initializes the in-memory inode. 
>>>>> I hope I'm not mistaken here...
>>>>>
>>>>> I make a small example. Filesystems call
>>>>> security_inode_init_security(). Ext4 does:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n54
>>>>>
>>>>> security_inode_init_security() allocates new_xattrs. Each LSM fills
>>>>> new_xattrs. At the end of the loop, if there is at least one xattr
>>>>> filled, the initxattrs() callback passed by the caller of
>>>>> security_inode_init_security() is called.
>>>>>
>>>>> The ext4 initxattrs() callback is:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ext4/xattr_security.c?h=v6.3-rc6#n35
>>>>>
>>>>> which scans the xattr array and, for each xattr,
>>>>> calls ext4_xattr_set_handle().
>>>>>
>>>>> Maybe I'm overlooking it, but ext4_xattr_set_handle() is setting xattrs
>>>>> on the disk. Am I wrong?
>>>> Yes, you're wrong. I tried your change, and the SMACK64_TRANSMUTE isn't
>>>> set on the sub-directory when it's created. The __vfs_setxattr() call really
>>>> is necessary. 
>>> Could you please also check if there is any change with this fix:
>>>
>>> Replace:
>>>
>>> 	xattr2->name = XATTR_NAME_SMACKTRANSMUTE;
>>>
>>> with:
>>>
>>> 	xattr2->name = XATTR_SMACK_TRANSMUTE;
>>>
>>> Thanks
>> Unless I'm missing something really obvious there's no way that any
>> of the evm/ima changes would obviate the need for the __vfs_setxattr() call.
>> It's real easy to verify correct behavior, see the attached script.
>> (you'll want to change the "notroot" value to a user on your system)
> 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).

The script is demonstrating that those xattrs don't exist when they
shouldn't, si all is good there.

>
> However, I saw another discussion related to transmute:
>
> https://lore.kernel.org/linux-security-module/20230419002338.566487-1-mengcc@xxxxxxxxxx/
>
> 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.

OK, I see that. Looks like the original implementation was poorly
thought out/tested.

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

That would explain the observed behavior.

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

It's possible. It's been a long time since I've looked at this.
I'm tempted to take a change to make overlayfs work upstream and
then worry about the ima changes. There seems to be a lot more
going on with the ima changes than is obvious from what's in the
Smack code.

>
> Roberto
>
>>> Roberto
>>>
>>>>> Thanks
>>>>>
>>>>> Roberto
>>>>>
>>>>>>> After all LSMs provide one or multiple xattrs, xattrs are going to be
>>>>>>> written to the disk with the initxattr() callback of filesystems.
>>>>>>>
>>>>>>> There is a small mistake above (XATTR_SMACK_TRANSMUTE instead
>>>>>>> of XATTR_NAME_SMACKTRANSMUTE, as we are providing just the suffix).
>>>>>> but I'm pretty sure the __vfs_setxattr() call is necessary to get
>>>>>> the attribute written out. With your change the in-memory inode will
>>>>>> get the attribute, but if you reboot it won't be on the directory.
>>>>>>
>>>>>>> 95 Passed, 0 Failed, 100% Success rate
>>>>>>>
>>>>>>> There was a test failing in dir-transmute.sh, before I fixed the xattr
>>>>>>> name.
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Roberto
>>>>>>>
>>>>>>>> know it's transmuting, but it won't get to disk without the __vfs_setxattr()
>>>>>>>> here in smack_d_instantiate(). Now, it's been a long time since that code
>>>>>>>> was written, so I could be wrong, but I'm pretty sure about that.
>>>>>>>>
>>>>>>>> I think that you should be fine with the changes in smack_init_inode_security(),
>>>>>>>> and leaving smack_d_instantiate() untouched. 
>>>>>>>>
>>>>>>>>>  			if (rc >= 0)
>>>>>>>>>  				transflag = SMK_INODE_TRANSMUTE;
>>>>>>>>>  		}