Message-ID: <8b4aa750-91f3-ca95-bb50-b2023676bc4e@digikod.net>
Date:   Fri, 26 Aug 2022 11:36:47 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     xiujianfeng <xiujianfeng@...wei.com>,
        Günther Noack <gnoack3000@...il.com>
Cc:     paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
        shuah@...nel.org, corbet@....net,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH -next 2/5] landlock: add chmod and chown support


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@...wei.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.

Powered by blists - more mailing lists