Messages in this thread | | | Date | Fri, 26 Aug 2022 13:32:31 +0200 | From | Mickaël Salaün <> | Subject | Re: [PATCH -next 2/5] landlock: add chmod and chown support |
| |
On 26/08/2022 13:14, xiujianfeng wrote: > Hi, > > 在 2022/8/26 17:36, Mickaël Salaün 写道: >> >> On 26/08/2022 10:36, xiujianfeng wrote: >>> Hi, >>> >>> 在 2022/8/24 19:44, Mickaël Salaün 写道: >>>> >>>> On 23/08/2022 14:50, xiujianfeng wrote: >>>>> >>>>> >>>>> 在 2022/8/23 5:07, Mickaël Salaün 写道: >>>>>> >>>>>> On 22/08/2022 20:25, Günther Noack wrote: >>>>>>> Hi! >>>>>>> >>>>>>> Thanks for sending this patch set! :) >>>>>>> >>>>>>> On Mon, Aug 22, 2022 at 07:46:58PM +0800, Xiu Jianfeng wrote: >>>>>>>> Add two flags LANDLOCK_ACCESS_FS_CHMOD and >>>>>>>> LANDLOCK_ACCESS_FS_CHOWN to >>>>>>>> support restriction to chmod(2) and chown(2) with landlock. >>>>>>>> >>>>>>>> Also change the landlock ABI version from 3 to 4. >>>>>>>> >>>>>>>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> >>>>>>>> --- >>>>>>>> include/uapi/linux/landlock.h | 8 ++++++-- >>>>>>>> security/landlock/fs.c | 16 >>>>>>>> +++++++++++++++- >>>>>>>> security/landlock/limits.h | 2 +- >>>>>>>> security/landlock/syscalls.c | 2 +- >>>>>>>> tools/testing/selftests/landlock/base_test.c | 2 +- >>>>>>>> tools/testing/selftests/landlock/fs_test.c | 6 ++++-- >>>>>>>> 6 files changed, 28 insertions(+), 8 deletions(-) >>>>>>>> >>>>>>>> diff --git a/include/uapi/linux/landlock.h >>>>>>>> b/include/uapi/linux/landlock.h >>>>>>>> index 735b1fe8326e..5ce633c92722 100644 >>>>>>>> --- a/include/uapi/linux/landlock.h >>>>>>>> +++ b/include/uapi/linux/landlock.h >>>>>>>> @@ -141,13 +141,15 @@ struct landlock_path_beneath_attr { >>>>>>>> * directory) parent. Otherwise, such actions are denied with >>>>>>>> errno set to >>>>>>>> * EACCES. The EACCES errno prevails over EXDEV to let user >>>>>>>> space >>>>>>>> * efficiently deal with an unrecoverable error. >>>>>>>> + * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a >>>>>>>> file. >>>>>>>> + * - %LANDLOCK_ACCESS_FS_CHOWN: Change the owner and/or group of a >>>>>>>> file. >>>>>> >>>>>> This section talk about "access rights that only apply to the >>>>>> content of >>>>>> a directory, not the directory itself", which is not correct (see >>>>>> LANDLOCK_ACCESS_FS_READ_DIR). I'd like these access rights to remain >>>>>> here but this kernel patch and the related tests need some changes. >>>>>> >>>>>> What about a LANDLOCK_ACCESS_FS_CHGRP? I'm not sure if we need to >>>>>> differentiate these actions or not, but we need arguments to choose. >>>>>> >>>>>> >>>>>>>> * >>>>>>>> * .. warning:: >>>>>>>> * >>>>>>>> * It is currently not possible to restrict some file-related >>>>>>>> actions >>>>>>>> * accessible through these syscall families: >>>>>>>> :manpage:`chdir(2)`, >>>>>>>> - * :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`, >>>>>>>> - * :manpage:`chown(2)`, :manpage:`setxattr(2)`, >>>>>>>> :manpage:`utime(2)`, >>>>>>>> + * :manpage:`stat(2)`, :manpage:`flock(2)`, >>>>>>>> + * :manpage:`setxattr(2)`, :manpage:`utime(2)`, >>>>>>> >>>>>>> *formatting nit* >>>>>>> We could fill up the full line width here >>>>>>> >>>>>>>> * :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, >>>>>>>> :manpage:`access(2)`. >>>>>>>> * Future Landlock evolutions will enable to restrict them. >>>>>>>> */ >>>>>>>> @@ -167,6 +169,8 @@ struct landlock_path_beneath_attr { >>>>>>>> #define LANDLOCK_ACCESS_FS_MAKE_SYM (1ULL << 12) >>>>>>>> #define LANDLOCK_ACCESS_FS_REFER (1ULL << 13) >>>>>>>> #define LANDLOCK_ACCESS_FS_TRUNCATE (1ULL << 14) >>>>>>>> +#define LANDLOCK_ACCESS_FS_CHMOD (1ULL << 15) >>>>>>>> +#define LANDLOCK_ACCESS_FS_CHOWN (1ULL << 16) >>>>>>>> /* clang-format on */ >>>>>>>> >>>>>>>> #endif /* _UAPI_LINUX_LANDLOCK_H */ >>>>>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c >>>>>>>> index c57f581a9cd5..c25d5f89c8be 100644 >>>>>>>> --- a/security/landlock/fs.c >>>>>>>> +++ b/security/landlock/fs.c >>>>>>>> @@ -147,7 +147,9 @@ static struct landlock_object >>>>>>>> *get_inode_object(struct inode *const inode) >>>>>>>> LANDLOCK_ACCESS_FS_EXECUTE | \ >>>>>>>> LANDLOCK_ACCESS_FS_WRITE_FILE | \ >>>>>>>> LANDLOCK_ACCESS_FS_READ_FILE | \ >>>>>>>> - LANDLOCK_ACCESS_FS_TRUNCATE) >>>>>>>> + LANDLOCK_ACCESS_FS_TRUNCATE | \ >>>>>>>> + LANDLOCK_ACCESS_FS_CHMOD | \ >>>>>>>> + LANDLOCK_ACCESS_FS_CHOWN) >>>>>>>> /* clang-format on */ >>>>>>>> >>>>>>>> /* >>>>>>>> @@ -1146,6 +1148,16 @@ static int hook_path_truncate(const struct >>>>>>>> path *const path) >>>>>>>> return current_check_access_path(path, >>>>>>>> LANDLOCK_ACCESS_FS_TRUNCATE); >>>>>>>> } >>>>>>>> >>>>>>>> +static int hook_path_chmod(const struct path *const dir, umode_t >>>>>>>> mode) >>>>>> >>>>>> This is not a "dir" but a "path". >>>>>> >>>>>> >>>>>>>> +{ >>>>>>>> + return current_check_access_path(dir, >>>>>>>> LANDLOCK_ACCESS_FS_CHMOD); >>>>>>>> +} >>>>>>>> + >>>>>>>> +static int hook_path_chown(const struct path *const dir, kuid_t >>>>>>>> uid, >>>>>>>> kgid_t gid) >>>>>> >>>>>> Same here. >>>>>> >>>>>> >>>>>>>> +{ >>>>>>>> + return current_check_access_path(dir, >>>>>>>> LANDLOCK_ACCESS_FS_CHOWN); >>>>>>>> +} >>>>>>> >>>>>>> One implication of this approach is that the chown+chmod right on a >>>>>>> directory's contents are always going together with the same >>>>>>> rights on >>>>>>> the directory itself. >>>>>>> >>>>>>> For example, if you grant chmod+chown access rights for "datadir/", >>>>>>> the command "chmod 0600 datadir/file1" will work, but so will the >>>>>>> command "chmod 0600 datadir". But the approach of checking just the >>>>>>> parent directory's rights is also inflexible if you think through the >>>>>>> kinds of rights you can grant with it. (It would also not be possible >>>>>>> to grant chmod+chown on individual files.) >>>>>> >>>>>> Good point. For an initial chmod/chown/chgrp access right, I'd >>>>>> prefer to >>>>>> be able to set these access rights on a directory but only for its >>>>>> content, not the directory itself. I think it is much safer and should >>>>>> be enough for the majority of use cases, but let me know if I'm >>>>>> missing >>>>>> something. I'm not sure being able to change the root directory access >>>>>> rights may be a good idea anyway (even for containers). ;) >>>>>> >>>>>> A path_beneath rule enables to identify a file hierarchy (i.e. the >>>>>> content of a directory), not to make modifications visible outside of >>>>>> the directory identifying the hierarchy (hence the "parent_fd" field), >>>>>> which would be the case with the current chmod/chown access rights. >>>>>> >>>>>> >>>>>>> >>>>>>> Do you have any thoughts on how to resolve this if this flexibility >>>>>>> might be needed? >>>>>>> >>>>>>> I wonder whether the right way to resolve this would be to give users >>>>>>> a way to make that distinction at the level of landlock_add_rule(), >>>>>>> with an API like this (note the additional flag): >>>>>>> >>>>>>> err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, >>>>>>> &path_beneath, >>>>>>> LANDLOCK_STRICTLY_BENEATH); >>>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>>>> >>>>>>> Multiple calls of landlock_add_rule() on the same file are already >>>>>>> today joining the requested access rights, so it would be possible to >>>>>>> mix-and-match "strict beneath" with "beneath" rights on the same >>>>>>> directory, and it would work in the same way for other access rights >>>>>>> as well. >>>>>> >>>>>> This kind of option is interesting. For now, some access rights are >>>>>> kind >>>>>> of "doubled" to enable to differentiate between a file and a directory >>>>>> (i.e. READ_DIR/READ_FILE, REMOVE_DIR/REMOVE_FILE, WRITE_FILE/MAKE_*) >>>>>> when it may be useful, but this is different. >>>>>> >>>>>> I think this "strictly beneath" behavior should be the default, >>>>>> which is >>>>>> currently the case. >>>>>> >>>>>> >>>>>>> >>>>>>> To be clear: I'm proposing this approach not because I think it >>>>>>> should >>>>>>> be part of this patch set, but because it would be good to have a way >>>>>>> forward if that kind of flexibility is needed in the future. >>>>>>> >>>>>>> Does that seem reasonable? >>>>>> >>>>>> This is the kind of questions that made such access rights not >>>>>> appropriate for the initial version of Landlock. But we should talk >>>>>> about that now. >>>>> >>>>> Hi Günther and Mickaël, >>>>> >>>>> Thanks for your comments, so I think the conclusion here is that we >>>>> have >>>>> to make sure that in this patchset chown/chmod access rights can be set >>>>> on a directory only for its content, not the directory itself, right? >>>>> any good idea about how to implement this? :) >>>> >>>> In such hook code, you need to get the parent directory of the path >>>> argument. This require to use and refactor the >>>> check_access_path_dual/jump_up part in a dedicated helper (and take care >>>> of all the corner cases). >>>> . >>> >>> Sorry, I don't quite understand what you mean, but I have another idea, >>> how about this? >>> >>> static int hook_path_chown(const struct path *const path, kuid_t uid, >>> kgid_t gid) >>> { >>> int ret; >>> struct dentry *parent_dentry; >>> struct path eff_path; >>> >>> eff_path = *path; >>> path_get(&eff_path); >>> if (d_is_dir(eff_path.dentry)) { >>> parent_dentry = dget_parent(eff_path.dentry); >>> dput(eff_path.dentry); >>> eff_path.dentry = parent_dentry; >>> } >>> ret = current_check_access_path(&eff_path, >>> LANDLOCK_ACCESS_FS_CHGRP); >>> path_put(&eff_path); >>> >>> return ret; >>> } >> >> This is close but it ignores mount points (e.g. path being used multiple >> time as a mount point). This is why we need to use follow_up(), hence my >> previous comment. This is the kind of corner case that require tests. >> >> This helper could look like this: >> enum walk_result walk_to_visible_parent(struct path *path) >> It could then return either WALK_CONTINUE, WALK_DENIED, or WALK_ALLOWED. >> . > > Thanks, It's more clear now, except the return type, I think void type > like follows maybe ok:
The enum return type is required to use this helper in check_access_path_dual(), and to handles the same cases (e.g. internal mount point).
> > static void walk_to_visible_parent(struct path *path) > { > struct dentry *parent_dentry; > > path_get(path); > /* don't need to follow_up if not dir */ > if (!d_is_dir(path->dentry))
This check should be in hook_path_chown(), to know if it makes sense to call walk_to_visible_parent().
> return; > > jump_up: > if (path->dentry == path->mnt->mnt_root) { > if (follow_up(path)) { > /* Ignores hidden mount points. */ > goto jump_up; > } else { > /*Stops at the real root. */ > return; > } > } > parent_dentry = dget_parent(path->dentry); > dput(path->dentry); > path->dentry = parent_dentry; > } > > static void walk_to_visible_parent_end(struct path *path)
This function is not useful, we could just explicitly call path_put() and document that requirement. To make it easier to understand and more consistent, we should not call path_get() in walk_to_visible_parent() but before to make it explicit. Something like this:
if (d_is_dir(path->dentry)) { path_get(path); switch (walk_to_visible_parent(path)) { … } path_put(path); … }
> { > path_put(path); > } > > static int hook_path_chown(const struct path *const path, kuid_t uid, > kgid_t gid) > { > int ret; > struct path eff_path;
All Landlock hooks must first check that a process is tied to a domain and return immediately if it is not the case.
> > eff_path = *path; > walk_to_visible_parent(&eff_path); > ret = current_check_access_path(&eff_path, > LANDLOCK_ACCESS_FS_CHGRP); > walk_to_visible_parent_end(&eff_path); > > return ret; > } >
| |