Message-ID: <1373bbe5-16b1-bf0e-5f92-14c31cb94897@huawei.com>
Date:   Fri, 18 Nov 2022 17:03:28 +0800
From:   xiujianfeng <xiujianfeng@...wei.com>
To:     Mickaël Salaün <mic@...ikod.net>,
        Günther Noack <gnoack3000@...il.com>
CC:     <paul@...l-moore.com>, <jmorris@...ei.org>, <serge@...lyn.com>,
        <shuah@...nel.org>, <corbet@....net>,
        <linux-security-module@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
        Christian Brauner <brauner@...nel.org>,
        Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Subject: Re: [PATCH -next v2 3/6] landlock: add chmod and chown support



在 2022/11/14 22:12, Mickaël Salaün 写道:
> 
> On 29/10/2022 10:33, xiujianfeng wrote:
>> Hi,
>>
>> 在 2022/9/2 1:34, Mickaël Salaün 写道:
>>> CCing linux-fsdevel@...r.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.
> 
> I though about differentiating user xattr, atime/mtime, DAC 
> (chown/chmod, posix ACLs), and other xattr, but it would be too complex 
> to get a consistent approach because of indirect consequences (e.g. 
> controlling umask, setegid, settimeofday…). Let's make it simple for now.
> 
> Here is an update on my previous proposal:
> 
> LANDLOCK_ACCESS_FS_READ_METADATA to read any file/dir metadata (i.e. 
> inode attr and xattr). In practice, for most use cases, this access 
> right should be granted whenever LANDLOCK_ACCESS_READ_DIR is allowed.
> 
> LANDLOCK_ACCESS_FS_WRITE_METADATA to *explicitly* write any inode attr 
> or xattr (i.e. chmod, chown, utime, and all xattr). It should be noted 
> that file modification time and access time should always be updated 
> according to the file operation (e.g. write, truncate) even when this 
> access is not explicitly allowed (according to vfs_utimes(), 
> ATTR_TIMES_SET and ATTR_TOUCH should enable to differentiate from 
> implicit time changes).
> 
Thanks, I analyzed the relevant functions and the use of lsm hooks.
so I think what to do will be as follows:

LANDLOCK_ACCESS_FS_WRITE_METADATA controls the following hooks:
1.security_path_chmod
2.security_path_chown
3.security_inode_setattr
4.security_inode_setxattr
5.security_inode_removexattr
6.security_inode_set_acl

LANDLOCK_ACCESS_FS_READ_METADATA controls the following hooks:
1.security_inode_getattr
2.security_inode_get_acl
3.security_inode_getxattr

and the following 7 hooks are using struct dentry * as parameter, should 
be changed to struct path *, and also their callers.

security_inode_setattr
security_inode_setxattr
security_inode_removexattr
security_inode_set_acl
security_inode_getattr
security_inode_get_acl
security_inode_getxattr

Looks like it's a big change.

> 
>>
>> 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?
> 
> I think this can be solved thanks to the ovl_path_real() helper.
> .

Powered by blists - more mailing lists