Messages in this thread | | | Subject | Re: [PATCH -next v2 3/6] landlock: add chmod and chown support | From | xiujianfeng <> | Date | Sat, 29 Oct 2022 16:33:28 +0800 |
| |
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?
| |