[PATCH -next 2/5] landlock: add chmod and chown support
xiujianfeng
xiujianfeng at huawei.com
Fri Aug 26 11:14:33 UTC 2022
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 at 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;
}
More information about the Linux-security-module-archive
mailing list