| Message ID | 20220822114701.26975-3-xiujianfeng@huawei.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | landlock: add chmod and chown support | expand |
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. > * > * .. 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) > +{ > + 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) > +{ > + 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.) 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. 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? > + > /* File hooks */ > > static inline access_mask_t get_file_access(const struct file *const file) > @@ -1199,6 +1211,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(path_unlink, hook_path_unlink), > LSM_HOOK_INIT(path_rmdir, hook_path_rmdir), > LSM_HOOK_INIT(path_truncate, hook_path_truncate), > + LSM_HOOK_INIT(path_chmod, hook_path_chmod), > + LSM_HOOK_INIT(path_chown, hook_path_chown), > > LSM_HOOK_INIT(file_open, hook_file_open), > }; > diff --git a/security/landlock/limits.h b/security/landlock/limits.h > index 82288f0e9e5e..08858da7fb4f 100644 > --- a/security/landlock/limits.h > +++ b/security/landlock/limits.h > @@ -18,7 +18,7 @@ > #define LANDLOCK_MAX_NUM_LAYERS 16 > #define LANDLOCK_MAX_NUM_RULES U32_MAX > > -#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_TRUNCATE > +#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_CHOWN > #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1) > #define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS) > > diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c > index f4d6fc7ed17f..469e0e11735c 100644 > --- a/security/landlock/syscalls.c > +++ b/security/landlock/syscalls.c > @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = { > .write = fop_dummy_write, > }; > > -#define LANDLOCK_ABI_VERSION 3 > +#define LANDLOCK_ABI_VERSION 4 ABI version 3 has not made it into a stable kernel yet; I wonder whether it wouldn't be easier to just bundle the truncate, chmod and chown rights as part of ABI version 3 (assuming that the patches make it into a stable release together)? Mickaël, do you have an opinion on this? —Günther --
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. > >> + >> /* File hooks */ >> >> static inline access_mask_t get_file_access(const struct file *const file) >> @@ -1199,6 +1211,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = { >> LSM_HOOK_INIT(path_unlink, hook_path_unlink), >> LSM_HOOK_INIT(path_rmdir, hook_path_rmdir), >> LSM_HOOK_INIT(path_truncate, hook_path_truncate), >> + LSM_HOOK_INIT(path_chmod, hook_path_chmod), >> + LSM_HOOK_INIT(path_chown, hook_path_chown), >> >> LSM_HOOK_INIT(file_open, hook_file_open), >> }; >> diff --git a/security/landlock/limits.h b/security/landlock/limits.h >> index 82288f0e9e5e..08858da7fb4f 100644 >> --- a/security/landlock/limits.h >> +++ b/security/landlock/limits.h >> @@ -18,7 +18,7 @@ >> #define LANDLOCK_MAX_NUM_LAYERS 16 >> #define LANDLOCK_MAX_NUM_RULES U32_MAX >> >> -#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_TRUNCATE >> +#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_CHOWN >> #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1) >> #define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS) >> >> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c >> index f4d6fc7ed17f..469e0e11735c 100644 >> --- a/security/landlock/syscalls.c >> +++ b/security/landlock/syscalls.c >> @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = { >> .write = fop_dummy_write, >> }; >> >> -#define LANDLOCK_ABI_VERSION 3 >> +#define LANDLOCK_ABI_VERSION 4 > > ABI version 3 has not made it into a stable kernel yet; I wonder > whether it wouldn't be easier to just bundle the truncate, chmod and > chown rights as part of ABI version 3 (assuming that the patches make > it into a stable release together)? > > Mickaël, do you have an opinion on this? I'll make sure to only have one ABI version bump per kernel release, but it is OK to bump it for this patch series in case it is not ready for the next merge window. I'll change it if required when merging into my tree. It is easier to change the code to decrease the version, so please keep it as is. ;)
在 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? :) > > >> >>> + >>> /* File hooks */ >>> >>> static inline access_mask_t get_file_access(const struct file >>> *const file) >>> @@ -1199,6 +1211,8 @@ static struct security_hook_list >>> landlock_hooks[] __lsm_ro_after_init = { >>> LSM_HOOK_INIT(path_unlink, hook_path_unlink), >>> LSM_HOOK_INIT(path_rmdir, hook_path_rmdir), >>> LSM_HOOK_INIT(path_truncate, hook_path_truncate), >>> + LSM_HOOK_INIT(path_chmod, hook_path_chmod), >>> + LSM_HOOK_INIT(path_chown, hook_path_chown), >>> >>> LSM_HOOK_INIT(file_open, hook_file_open), >>> }; >>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h >>> index 82288f0e9e5e..08858da7fb4f 100644 >>> --- a/security/landlock/limits.h >>> +++ b/security/landlock/limits.h >>> @@ -18,7 +18,7 @@ >>> #define LANDLOCK_MAX_NUM_LAYERS 16 >>> #define LANDLOCK_MAX_NUM_RULES U32_MAX >>> >>> -#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_TRUNCATE >>> +#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_CHOWN >>> #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << >>> 1) - 1) >>> #define LANDLOCK_NUM_ACCESS_FS >>> __const_hweight64(LANDLOCK_MASK_ACCESS_FS) >>> >>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c >>> index f4d6fc7ed17f..469e0e11735c 100644 >>> --- a/security/landlock/syscalls.c >>> +++ b/security/landlock/syscalls.c >>> @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = { >>> .write = fop_dummy_write, >>> }; >>> >>> -#define LANDLOCK_ABI_VERSION 3 >>> +#define LANDLOCK_ABI_VERSION 4 >> >> ABI version 3 has not made it into a stable kernel yet; I wonder >> whether it wouldn't be easier to just bundle the truncate, chmod and >> chown rights as part of ABI version 3 (assuming that the patches make >> it into a stable release together)? >> >> Mickaël, do you have an opinion on this? > > I'll make sure to only have one ABI version bump per kernel release, but > it is OK to bump it for this patch series in case it is not ready for > the next merge window. I'll change it if required when merging into my > tree. It is easier to change the code to decrease the version, so please > keep it as is. ;) > .
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).
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; }
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.
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: 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)) 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) { 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; 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; }
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; > } >
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. * * .. 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)`, * :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) +{ + 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) +{ + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_CHOWN); +} + /* File hooks */ static inline access_mask_t get_file_access(const struct file *const file) @@ -1199,6 +1211,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(path_unlink, hook_path_unlink), LSM_HOOK_INIT(path_rmdir, hook_path_rmdir), LSM_HOOK_INIT(path_truncate, hook_path_truncate), + LSM_HOOK_INIT(path_chmod, hook_path_chmod), + LSM_HOOK_INIT(path_chown, hook_path_chown), LSM_HOOK_INIT(file_open, hook_file_open), }; diff --git a/security/landlock/limits.h b/security/landlock/limits.h index 82288f0e9e5e..08858da7fb4f 100644 --- a/security/landlock/limits.h +++ b/security/landlock/limits.h @@ -18,7 +18,7 @@ #define LANDLOCK_MAX_NUM_LAYERS 16 #define LANDLOCK_MAX_NUM_RULES U32_MAX -#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_TRUNCATE +#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_CHOWN #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1) #define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS) diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c index f4d6fc7ed17f..469e0e11735c 100644 --- a/security/landlock/syscalls.c +++ b/security/landlock/syscalls.c @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = { .write = fop_dummy_write, }; -#define LANDLOCK_ABI_VERSION 3 +#define LANDLOCK_ABI_VERSION 4 /** * sys_landlock_create_ruleset - Create a new ruleset diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c index 72cdae277b02..9f00582f639c 100644 --- a/tools/testing/selftests/landlock/base_test.c +++ b/tools/testing/selftests/landlock/base_test.c @@ -75,7 +75,7 @@ TEST(abi_version) const struct landlock_ruleset_attr ruleset_attr = { .handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE, }; - ASSERT_EQ(3, landlock_create_ruleset(NULL, 0, + ASSERT_EQ(4, landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION)); ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0, diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index debe2d9ea6cf..5b55b93b5570 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -404,9 +404,11 @@ TEST_F_FORK(layout1, inval) 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) -#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE +#define ACCESS_LAST LANDLOCK_ACCESS_FS_CHOWN #define ACCESS_ALL ( \ ACCESS_FILE | \
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(-)