Messages in this thread | | | Subject | Re: [PATCH -next v2 3/6] landlock: add chmod and chown support | From | xiujianfeng <> | Date | Fri, 18 Nov 2022 17:03:28 +0800 |
| |
在 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@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. > > 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. > .
| |