Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

[xiujianfeng <xiujianfeng@huawei.com>]

Hi,

在 2022/9/2 1:34, Mickaël Salaün 写道:
> CCing linux-fsdevel@vger.kernel.org
> 
> 
> On 01/09/2022 15:06, xiujianfeng wrote:
>> Hi,
>>
>> 在 2022/8/30 0:01, Mickaël Salaün 写道:
>>>
>>> On 29/08/2022 03:17, xiujianfeng wrote:
>>>>
>>>> Hi,
>>>>
>>>> 在 2022/8/28 3:30, Günther Noack 写道:
>>>>> Hello!
>>>>>
>>>>> the mapping between Landlock rights to LSM hooks is now as follows in
>>>>> your patch set:
>>>>>
>>>>> * LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
>>>>> * LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
>>>>>      (this hook can restrict both the chown(2) and chgrp(2) syscalls)
>>>>>
>>>>> Is this the desired mapping?
>>>>>
>>>>> The previous discussion I found on the topic was in
>>>>>
>>>>> [1]
>>>>> https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@digikod.net/ 
>>>>>
>>>>>
>>>>> [2]
>>>>> https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@schaufler-ca.com/ 
>>>>>
>>>>>
>>>>> [3]
>>>>> https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@digikod.net/ 
>>>>>
>>>>>
>>>>>
>>>>> In my understanding the main arguments were the ones in [2] and [3].
>>>>>
>>>>> There were no further responses to [3], so I was under the impression
>>>>> that we were gravitating towards an approach where the
>>>>> file-metadata-modification operations were grouped more coarsely?
>>>>>
>>>>> For example with the approach suggested in [3], which would be to
>>>>> group the operations coarsely into (a) one Landlock right for
>>>>> modifying file metadata that is used in security contexts, and (b) one
>>>>> Landlock right for modifying metadata that was used in non-security
>>>>> contexts. That would mean that there would be:
>>>>>
>>>>> (a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
>>>>> following operations:
>>>>>      * chmod(2)-variants through hook_path_chmod,
>>>>>      * chown(2)-variants and chgrp(2)-variants through 
>>>>> hook_path_chown,
>>>>>      * setxattr(2)-variants and removexattr(2)-variants for extended
>>>>>        attributes that are not "user extended attributes" as 
>>>>> described in
>>>>>        xattr(7) through hook_inode_setxattr and hook_inode_removexattr
>>>>>
>>>>> (b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
>>>>> following operations:
>>>>>      * utimes(2) and other operations for setting other non-security
>>>>>        sensitive attributes, probably through hook_inode_setattr(?)
>>>>>      * xattr modifications like above, but for the "user extended
>>>>>        attributes", though hook_inode_setxattr and 
>>>>> hook_inode_removexattr
>>>>>
>>>>> In my mind, this would be a sensible grouping, and it would also help
>>>>> to decouple the userspace-exposed API from the underlying
>>>>> implementation, as Casey suggested to do in [2].
>>>>>
>>>>> Specifically for this patch set, if you want to use this grouping, you
>>>>> would only need to add one new Landlock right
>>>>> (LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
>>>>> under (a) (and maybe we can find a shorter name for it... :))?
>>>>>
>>>>> Did I miss any operations here that would be necessary to restrict?
>>>>>
>>>>> Would that make sense to you? Xiu, what is your opinion on how this
>>>>> should be grouped? Do you have use cases in mind where a more
>>>>> fine-grained grouping would be required?
>>>>
>>>> I apologize I may missed that discussion when I prepared v2:(
>>>>
>>>> Yes, agreed, this grouping is more sensible and resonnable. so in this
>>>> patchset only one right will be added, and I suppose the first commit
>>>> which expand access_mask_t to u32 can be droped.
>>>>
>>>>>
>>>>> —Günther
>>>>>
>>>>> P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called
>>>>> on a variety on attribute changes including file ownership, file size
>>>>> and file mode, so it might potentially interact with a bunch of other
>>>>> existing Landlock rights. Maybe that is not the right approach. In any
>>>>> case, it seems like it might require more thinking and it might be
>>>>> sensible to do that in a separate patch set IMHO.
>>>>
>>>> Thanks for you reminder, that seems it's more complicated to support
>>>> utimes, so I think we'd better not support it in this patchset.
>>>
>>> The issue with this approach is that it makes it impossible to properly
>>> group such access rights. Indeed, to avoid inconsistencies and much more
>>> complexity, we cannot extend a Landlock access right once it is defined.
>>>
>>> We also need to consider that file ownership and permissions have a
>>> default (e.g. umask), which is also a way to set them. How to
>>> consistently manage that? What if the application wants to protect its
>>> files with chmod 0400?
>>
>> what do you mean by this? do you mean that we should have a set of
>> default permissions for files created by applications within the
>> sandbox, so that it can update metadata of its own file.
> 
> I mean that we need a consistent access control system, and for this we 
> need to consider all the ways an extended attribute can be set.
> 
> We can either extend the meaning of current access rights (controlled 
> with a ruleset flag for compatibility reasons), or create new access 
> rights. I think it would be better to add new dedicated rights to make 
> it more explicit and flexible.
> 
> I'm not sure about the right approach to properly control file 
> permission. We need to think about it. Do you have some ideas?
> 
> BTW, utimes can be controlled with the inode_setattr() LSM hook. Being 
> able to control arbitrary file time modification could be part of the 
> FS_WRITE_SAFE_METADATA, but modification and access time should always 
> be updated according to the file operation.
> 
> 
>>
>>>
>>> About the naming, I think we can start with:
>>> - LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
>>> - LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user xattr;
>>
>> do you mean we should have permission controls on metadata level or
>> operation level? e.g. should we allow update on user xattr but deny
>> update on security xattr? or should we disallow update on any xattr?
>>
>>> - LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel
>>> (could change non-Landlock DAC or MAC, which could be considered as a
>>> policy bypass; or other various xattr that might be interpreted by
>>> filesystems), this should be denied most of the time.
>>
>> do you mean FS_WRITE_UNSAFE_METADATA is security-related? and
>> FS_WRITE_SAFE_METADATA is non-security-related?
> 
> Yes, FS_WRITE_UNSAFE_METADATA would be for security related 
> xattr/chmod/chown, and FS_WRITE_SAFE_METADATA for non-security xattr. 
> Both are mutually exclusive. This would involve the inode_setattr and 
> inode_setxattr LSM hooks. Looking at the calling sites, it seems 
> possible to replace all inode arguments with paths.
> .

Sorry for the late reply, I have problems with this work, for example,
before:
security_inode_setattr(struct user_namespace *mnt_userns,
                                          struct dentry *dentry,
                                          struct iattr *attr)
after:
security_inode_setattr(struct user_namespace *mnt_userns,
                                          struct path *path,
                                          struct iattr *attr)
then I change the second argument in notify_change() from struct *dentry 
to struct path *, that makes this kind of changes in fs/overlayfs/ 
spread to lots of places because overlayfs basicly uses dentry instead 
of path, the worst case may be here:

ovl_special_inode_operations.set_acl hook calls:
-->
ovl_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry, 
struct posix_acl *acl, int type)
-->
ovl_setattr(struct user_namespace *mnt_userns, struct dentry 
*dentry,struct iattr *attr)
-->
ovl_do_notify_change(struct ovl_fs *ofs, struct dentry *upperdentry, 
struct iattr *attr)

from the top of this callchain, I can not find a path to replace dentry, 
did I miss something? or do you have better idea?