Re: [PATCH v3 0/5] Landlock: IOCTL support

["Mickaël Salaün" <mic@digikod.net>]

On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote:
> Hi!
> 
> On Fri, Aug 18, 2023 at 03:39:19PM +0200, Mickaël Salaün wrote:
> > On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote:
> > > These patches add simple ioctl(2) support to Landlock.
> > 
> > [...]
> > 
> > > How we arrived at the list of always-permitted IOCTL commands
> > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 
> > > To decide which IOCTL commands should be blanket-permitted I went through the
> > > list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually
> > > to understand what they are about.  The following list is my conclusion from
> > > that.
> > > 
> > > We should always allow the following IOCTL commands:
> > > 
> > >  * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the
> > >    close-on-exec flag
> > >  * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO
> > >    and async flags
> > >  * FIONREAD - get the number of bytes available for reading (the implementation
> > >    is defined per file type)
> > 
> > I think we should treat FIOQSIZE like FIONREAD, i.e. check for
> > LANDLOCK_ACCESS_FS_READ_FILE as explain in my previous message.
> > Tests should then rely on something else.
> 
> OK, I rewrote the tests to use FS_IOC_GETFLAGS.
> 
> Some thoughts on these two IOCTLs:
> 
> FIONREAD gives the number of bytes that are ready to read.  This IOCTL seems
> only useful when the file is open for reading.  However, do you think that we
> should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b)
> f->f_mode & FMODE_READ?  (The difference is that in case (a), FIONREAD will work
> if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE
> right for that file.  In case (b), it would only work if you also opened the
> file for reading.)

I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled
and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD.

f->f_mode & FMODE_READ would make sense but it should have been handled
by the kernel; Landlock should not try to fix this inconsistency.

These are good test cases though.

It should be noted that SELinux considers FIONREAD as tied to metadata
reading (FILE__GETATTR). It think it makes more sense to tie it to the
read right (LANDLOCK_ACCESS_FS_READ_FILE) because it might be
(legitimately) required to properly read a file and FIONREAD is tied to
the content of a file, not file attributes.

> 
> FIOQSIZE seems like it would be useful for both reading *and* writing? -- The
> reading case is obvious, but for writers it's also useful if you want to seek
> around in the file, and make sure that the position that you seek to already
> exists.  (I'm not sure whether that use case is relevant in practical
> applications though.) -- Why would FIOQSIZE only be useful for readers?

Good point! The use case you define for writing is interesting. However,
would it make sense to seek at a specific offset without being able to
know/read the content? I guest not in theory, but in practice we might
want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is
they only require to write (at a specific offset), or to deal with write
errors. Anyway, I guess that this information can be inferred by trying
to seek at a specific offset.  The only limitation that this approach
would bring is that it seems that we can seek into an FD even without
read nor write right, and there is no specific (LSM) access control for
this operation (and nobody seems to care about being able to read the
size of a symlink once opened). If this is correct, I think we should
indeed always allow FIOQSIZE. Being able to open a file requires
LANDLOCK_ACCESS_FS_READ or WRITE anyway.  It would be interesting to
check and test with O_PATH though.

> 
> (In fact, it seems to me almost like FIOQSIZE might rather be missing a security
> hook check for one of the "getting file attribute" hooks?)
> 
> So basically, the implementation that I currently ended up with is:
> 

Before checking these commands, we first need to check that the original
domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new
bit and replace the file's allowed_access field (see
landlock_add_fs_access_mask() and similar helpers in the network patch
series that does a similar thing but for ruleset's handle access
rights), but here is the idea:

if (!landlock_file_handles_ioctl(file))
	return 0;

> switch (cmd) {
	/*
	 * Allows file descriptor and file description operations (see
	 * fcntl commands).
	 */
>   case FIOCLEX:
>   case FIONCLEX:
>   case FIONBIO:
>   case FIOASYNC:

>   case FIOQSIZE:
>     return 0;
>   case FIONREAD:
>     if (file->f_mode & FMODE_READ)

We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of
FMODE_READ.

>       return 0;
> }
> 
> (with some comments in the source code, of course...)
> 
> Does that look reasonable to you?
> 
> —Günther
> 
> -- 
> Sent using Mutt 🐕 Woof Woof