On Sat, Feb 10, 2024 at 12:18:06PM +0100, Günther Noack wrote:
> On Fri, Feb 09, 2024 at 06:06:05PM +0100, Günther Noack wrote:
> > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > index 73997e63734f..84efea3f7c0f 100644
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > @@ -1333,7 +1520,9 @@ static int hook_file_open(struct file *const file)
> > {
> > layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> > access_mask_t open_access_request, full_access_request, allowed_access;
> > - const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
> > + const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE |
> > + LANDLOCK_ACCESS_FS_IOCTL |
> > + IOCTL_GROUPS;
> > const struct landlock_ruleset *const dom = get_current_fs_domain();
> >
> > if (!dom)
> > @@ -1375,6 +1564,16 @@ static int hook_file_open(struct file *const file)
> > }
> > }
> >
> > + /*
> > + * Named pipes should be treated just like anonymous pipes.
> > + * Therefore, we permit all IOCTLs on them.
> > + */
> > + if (S_ISFIFO(file_inode(file)->i_mode)) {
> > + allowed_access |= LANDLOCK_ACCESS_FS_IOCTL |
> > + LANDLOCK_ACCESS_FS_IOCTL_RW |
> > + LANDLOCK_ACCESS_FS_IOCTL_RW_FILE;
Why not LANDLOCK_ACCESS_FS_IOCTL | IOCTL_GROUPS instead?
> > + }
> > +
>
> Hello Mickaël, this "if" is a change I'd like to draw your attention
> to -- this special case was necessary so that all IOCTLs are permitted
> on named pipes. (There is also a test for it in another commit.)
>
> Open questions here are:
>
> - I'm a bit on the edge whether it's worth it to have these special
> cases here. After all, users can very easily just permit all
> IOCTLs through the ruleset if needed, and it might simplify the
> mental model that we have to explain in the documentation
It might simplify the kernel implementation a bit but it would make the
Landlock security policies more complex, and could encourage people to
allow all IOCTLs on a directory because this directory might contain
(dynamically created) named pipes.
I suggest to extend this check with S_ISFIFO(mode) || S_ISSOCK(mode).
A comment should explain that LANDLOCK_ACCESS_FS_* rights are not meant
to restrict IPCs.
>
> - I've put the special case into the file open hook, under the
> assumption that it would simplify the Landlock audit support to
> have the correct rights on the struct file. The implementation
> could alternatively also be done in the ioctl hook. Let me know
> which one makes more sense to you.
I like your approach, thanks! Also, in theory this approach should be
better for performance reasons, even if it should not be visible in
practice. Anyway, keeping a consistent set of access rights is
definitely useful for observability.
I'm wondering if we should do the same mode check for
LANDLOCK_ACCESS_FS_TRUNCATE too... It would not be visible to user space
anyway because the LSM hooks are called after the file mode checks for
truncate(2) and ftruncate(2). But because we need this kind of check for
IOCTL, it might be a good idea to make it common to all optional_access
values, at least to document what is really handled. Adding dedicated
truncate and ftruncate tests (before this commit) would guarantee that
the returned error codes are unchanged.
Moving this check before the is_access_to_paths_allowed() call would
enable to avoid looking for always-allowed access rights by removing
them from the full_access_request. This could help improve performance
when opening named pipe because no optional_access would be requested.
A new helper similar to get_required_file_open_access() could help.
>
> BTW, named UNIX domain sockets can apparently not be opened with open() and
> therefore they don't hit the LSM file_open hook. (It is done with the BSD
> socket API instead.)
What about /proc/*/fd/* ? We can test with open_proc_fd() to make sure
our assumptions are correct.
