Re: [PATCH -next v2 3/6] landlock: add chmod and chown support

From: xiujianfeng
Date: Thu Sep 01 2022 - 09:11:52 EST


Hi,

在 2022/8/30 0:01, Mickaël Salaün 写道:

On 29/08/2022 03:17, xiujianfeng wrote:

Hi,

在 2022/8/28 3:30, Günther Noack 写道:
Hello!

the mapping between Landlock rights to LSM hooks is now as follows in
your patch set:

* LANDLOCK_ACCESS_FS_CHMOD controls hook_path_chmod
* LANDLOCK_ACCESS_FS_CHGRP controls hook_path_chown
    (this hook can restrict both the chown(2) and chgrp(2) syscalls)

Is this the desired mapping?

The previous discussion I found on the topic was in

[1] https://lore.kernel.org/all/5873455f-fff9-618c-25b1-8b6a4ec94368@xxxxxxxxxxx/
[2] https://lore.kernel.org/all/b1d69dfa-6d93-2034-7854-e2bc4017d20e@xxxxxxxxxxxxxxxx/
[3] https://lore.kernel.org/all/c369c45d-5aa8-3e39-c7d6-b08b165495fd@xxxxxxxxxxx/

In my understanding the main arguments were the ones in [2] and [3].

There were no further responses to [3], so I was under the impression
that we were gravitating towards an approach where the
file-metadata-modification operations were grouped more coarsely?

For example with the approach suggested in [3], which would be to
group the operations coarsely into (a) one Landlock right for
modifying file metadata that is used in security contexts, and (b) one
Landlock right for modifying metadata that was used in non-security
contexts. That would mean that there would be:

(a) LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES to control the
following operations:
    * chmod(2)-variants through hook_path_chmod,
    * chown(2)-variants and chgrp(2)-variants through hook_path_chown,
    * setxattr(2)-variants and removexattr(2)-variants for extended
      attributes that are not "user extended attributes" as described in
      xattr(7) through hook_inode_setxattr and hook_inode_removexattr

(b) LANDLOCK_ACCESS_FS_MODIFY_NON_SECURITY_ATTRIBUTES to control the
following operations:
    * utimes(2) and other operations for setting other non-security
      sensitive attributes, probably through hook_inode_setattr(?)
    * xattr modifications like above, but for the "user extended
      attributes", though hook_inode_setxattr and hook_inode_removexattr

In my mind, this would be a sensible grouping, and it would also help
to decouple the userspace-exposed API from the underlying
implementation, as Casey suggested to do in [2].

Specifically for this patch set, if you want to use this grouping, you
would only need to add one new Landlock right
(LANDLOCK_ACCESS_FS_MODIFY_SECURITY_ATTRIBUTES) as described above
under (a) (and maybe we can find a shorter name for it... :))?

Did I miss any operations here that would be necessary to restrict?

Would that make sense to you? Xiu, what is your opinion on how this
should be grouped? Do you have use cases in mind where a more
fine-grained grouping would be required?

I apologize I may missed that discussion when I prepared v2:(

Yes, agreed, this grouping is more sensible and resonnable. so in this
patchset only one right will be added, and I suppose the first commit
which expand access_mask_t to u32 can be droped.


—Günther

P.S.: Regarding utimes: The hook_inode_setattr hook *also* gets called
on a variety on attribute changes including file ownership, file size
and file mode, so it might potentially interact with a bunch of other
existing Landlock rights. Maybe that is not the right approach. In any
case, it seems like it might require more thinking and it might be
sensible to do that in a separate patch set IMHO.

Thanks for you reminder, that seems it's more complicated to support
utimes, so I think we'd better not support it in this patchset.

The issue with this approach is that it makes it impossible to properly group such access rights. Indeed, to avoid inconsistencies and much more complexity, we cannot extend a Landlock access right once it is defined.

We also need to consider that file ownership and permissions have a default (e.g. umask), which is also a way to set them. How to consistently manage that? What if the application wants to protect its files with chmod 0400?

what do you mean by this? do you mean that we should have a set of default permissions for files created by applications within the sandbox, so that it can update metadata of its own file.


About the naming, I think we can start with:
- LANDLOCK_ACCESS_FS_READ_METADATA (read any file/dir metadata);
- LANDLOCK_ACCESS_FS_WRITE_SAFE_METADATA: change file times, user xattr;

do you mean we should have permission controls on metadata level or operation level? e.g. should we allow update on user xattr but deny update on security xattr? or should we disallow update on any xattr?

- LANDLOCK_ACCESS_FS_WRITE_UNSAFE_METADATA: interpreted by the kernel (could change non-Landlock DAC or MAC, which could be considered as a policy bypass; or other various xattr that might be interpreted by filesystems), this should be denied most of the time.

do you mean FS_WRITE_UNSAFE_METADATA is security-related? and FS_WRITE_SAFE_METADATA is non-security-related?

.