On Fri, Feb 16, 2024 at 03:11:18PM +0100, Mickaël Salaün wrote:
> 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.
Actually, these fifo and socket checks (and related optimizations)
should already be handled with is_nouser_or_private() called by
is_access_to_paths_allowed(). Some new dedicated tests should help
though.
