[PATCH v8 4/9] landlock: Add IOCTL access right

[Mickaël Salaün]

On Tue, Jan 30, 2024 at 07:13:25PM +0100, Günther Noack wrote:
> Hello!
> 
> On Thu, Dec 14, 2023 at 03:28:10PM +0100, Mickaël Salaün wrote:
> > Christian, what do you think about the following IOCTL groups?
> > 
> > On Thu, Dec 14, 2023 at 11:14:10AM +0100, Mickaël Salaün wrote:
> > > On Thu, Dec 14, 2023 at 10:26:49AM +0100, Mickaël Salaün wrote:
> > > > On Fri, Dec 08, 2023 at 04:51:16PM +0100, Günther Noack wrote:

> > > > > +	switch (cmd) {
> > > > > +	case FIOCLEX:
> > > > > +	case FIONCLEX:
> > > > > +	case FIONBIO:
> > > > > +	case FIOASYNC:
> > > > > +		/*
> > > > > +		 * FIOCLEX, FIONCLEX, FIONBIO and FIOASYNC manipulate the FD's
> > > > > +		 * close-on-exec and the file's buffered-IO and async flags.
> > > > > +		 * These operations are also available through fcntl(2),
> > > > > +		 * and are unconditionally permitted in Landlock.
> > > > > +		 */
> > > > > +		return 0;
> > 
> > Could you please add comments for the following IOCTL commands
> > explaining why they make sense for the related file/dir read/write
> > mapping? We discussed about that in the ML but it would be much easier
> > to put that doc here for future changes, and for reviewers to understand
> > the rationale. Some of this doc is already in the cover letter.
> 
> Done, I'm adding documentation inline here.
> 
> > 
> > To make this easier to follow, what about renaming the IOCTL groups to
> > something like this:
> > * LANDLOCK_ACCESS_FS_IOCTL_GROUP1:
> >   LANDLOCK_ACCESS_FS_IOCTL_GET_SIZE

Well, this looks better:
LANDLOCK_ACCESS_FS_IOCTL_RW

We could think that it includes LANDLOCK_ACCESS_FS_MAKE_* though (which
is not the case), but it looks like the least worst...  These synthetic
access rights are not public anyway.

> > * LANDLOCK_ACCESS_FS_IOCTL_GROUP2:
> >   LANDLOCK_ACCESS_FS_IOCTL_GET_INNER

LANDLOCK_ACCESS_FS_IOCTL_RW_FILE

> > * LANDLOCK_ACCESS_FS_IOCTL_GROUP3:
> >   LANDLOCK_ACCESS_FS_IOCTL_READ_FILE

LANDLOCK_ACCESS_FS_IOCTL_R_FILE

> > * LANDLOCK_ACCESS_FS_IOCTL_GROUP4:
> >   LANDLOCK_ACCESS_FS_IOCTL_WRITE_FILE

LANDLOCK_ACCESS_FS_IOCTL_W_FILE

> 
> Agreed that better names are in order here.
> I renamed them as you suggested.
> 
> In principle, it would have been nice to name them after the access rights which
> enable them, but LANDLOCK_ACCESS_FS_IOCTL_READ_DIR_OR_READ_FILE_OR_WRITE_FILE is
> a bit too long for my taste. o_O
> 
> 
> > > > > +	case FIOQSIZE:
> > > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP1;
> > > > > +	case FS_IOC_FIEMAP:
> > > > > +	case FIBMAP:
> > > > > +	case FIGETBSZ:
> > 
> > Does it make sense to not include FIGETBSZ in
> > LANDLOCK_ACCESS_FS_IOCTL_GROUP1? I think it's OK like this as previously

I guess I meant "Does it make sense *to include* FIGETBSZ in
LANDLOCK_ACCESS_FS_IOCTL_GROUP1?" Which means to allow it for the
write_file, read_file and read_dir access rights:
LANDLOCK_ACCESS_FS_IOCTL_RW.

> > explained but I'd like to get confirmation:
> > https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net
> 
> It seems that the more standardized way to get file system block sizes is to use
> POSIX' statvfs(3) interface, whose functionality is provided through the
> statfs(2) syscall.  These functions have the usual path-based and fd-based
> variants.  Landlock does not currently restrict statfs(2) at all, but there is
> an existing LSM security hook for it.
> 
> We should probably introduce an access right to restrict statfs(2) in the
> future, because this otherwise lets callers probe for the existence of files.  I
> filed https://github.com/landlock-lsm/linux/issues/18 for it.

According to the struct statfs fields, most of them seems to be useful
for file writes (e.g. f_bsize) and file creations (e.g. f_ffree) but
potentially for file read too (e.g. f_bsize). I'm not sure how statfs is
used in practice though.

> 
> I am not sure how to group this best.  It seems like a very harmless thing to
> allow.  (What is to be learned from the filesystem blocksize anyway?)  If we are
> unsure about it, we could do the following though:

I agree that it seems to be harmless.

When we'll be able to control statfs(2), following the same logic,
LANDLOCK_ACCESS_FS_IOCTL_RW should allows to use it.  To said it another
way, implementing the statfs LSM hook doesn't seem to be useful once we
get the ability to restrict path walks:
https://github.com/landlock-lsm/linux/issues/9
Until then, there are other ways to probe for file existence anyway.

> 
>  - disallow FIGETBSZ unless LANDLOCK_ACCESS_FS_IOCTL ("misc") is granted
>  - allow FIGETBSZ together with a future access right which controls statfs(2)
> 
> In that case, the use of FIGETBSZ would be nicely separable from regular read
> access for files, and it would be associated with the same right.

If this FIGETBSZ is legitimately used by applications to optimize their
FS interactions, then we should not mask it under FS_IOCTL because this
would result of applications allowing FS_IOCTL everywhere, which is not
what we want.

> 
> (We could also potentially group FS_IOC_FIEMAP and FIBMAP in the same way.
> These ones give information about file extents and a file's block numbers.  (You
> can check whether your file is stored in a continuous area on disk.))
> 
> This would simplify the story somewhat for the IOCTLs that we need to
> immediately give access to.
> 
> What do you think?

It would help to trace a lot of generic applications and see which IOCTL
command are used in practice, we might discover new ones.

> 
> 
> > > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP2;
> > > > > +	case FIONREAD:
> > > > > +	case FIDEDUPERANGE:
> > > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP3;
> > > > > +	case FICLONE:
> > > > > +	case FICLONERANGE:
> > 
> > The FICLONE* commands seems to already check read/write permissions with
> > generic_file_rw_checks(). Always allowing them should then be OK (and
> > the current tests should still pass), but we can still keep them here to
> > make the required access right explicit and test with and without
> > Landlock restrictions to make sure this is consistent with the VFS
> > access checks. See
> > https://lore.kernel.org/r/20230904.aiWae8eineo4@digikod.net
> > If this is correct, a new test should check that Landlock restrictions
> > are the same as the VFS checks and then don't impact such IOCTLs.
> 
> Noted.  I'll look into it.
> 
> (My understanding of FICLONE, FIDEDUPRANGE and FICLONERANGE is that they let
> files share the same underlying storage, on a per-range basis ("reflink").  The
> IOCTL man pages for these do not explain that as explicitly, but the key point
> is that the two resulting files still behave like a regular copy, because this
> feature exists on COW file systems only.  So that reinforces the approach of
> using READ_FILE and WRITE_FILE access rights for these IOCTL commands (because
> it behaves just as if we had called read() on one file and written the results
> to the other file with write()).)
> 
> 
> > > > > +	case FS_IOC_RESVSP:
> > > > > +	case FS_IOC_RESVSP64:
> > > > > +	case FS_IOC_UNRESVSP:
> > > > > +	case FS_IOC_UNRESVSP64:
> > > > > +	case FS_IOC_ZERO_RANGE:
> > > > > +		return LANDLOCK_ACCESS_FS_IOCTL_GROUP4;
> > > > > +	default:
> > > > > +		/*
> > > > > +		 * Other commands are guarded by the catch-all access right.
> > > > > +		 */
> > > > > +		return LANDLOCK_ACCESS_FS_IOCTL;
> > > > > +	}
> > > > > +}
> 
> > We previously talked about allowing all IOCTLs on unix sockets and named
> > pipes: https://lore.kernel.org/r/ZP7lxmXklksadvz+@google.com
> 
> Thanks for the reminder, I missed that.  Putting it on the TODO list.
> 
> 
> > I think the remaining issue with this grouping is that if the VFS
> > implementation returns -ENOIOCTLCMD, then the IOCTL command can be
> > forwarded to the device driver (for character or block devices).
> > For instance, FIONREAD on a character device could translate to unknown
> > action (on this device), which should then be considered dangerous and
> > denied unless explicitly allowed with LANDLOCK_ACCESS_FS_IOCTL (but not
> > any IOCTL_GROUP*).
> >
> > For instance, FIONREAD on /dev/null should return -ENOTTY, which should
> > then also be the case if LANDLOCK_ACCESS_FS_IOCTL is allowed (even if
> > LANDLOCK_ACCESS_FS_READ_FILE is denied). This is also the case for
> > file_ioctl()'s commands.
> > 
> > One solution to implement this logic would be to add an additional check
> > in hook_file_ioctl() for specific file types (!S_ISREG or socket or pipe
> > exceptions) and IOCTL commands.
> 
> In my view this seems OK, because we are primarily protecting access to
> resources (files), and only secondarily reducing the exposed kernel attack
> surface.

Correct, but of course block and character devices need to be handled.
seccomp-bpf is the main tool to protect the kernel in this case.

> 
> I agree there is a certain risk associated with calling ioctl(fd, FIONREAD, ...)
> on a buggy device driver.  But then again, that risk is comparable to the risk
> of calling read(fd, &buf, buflen) on the same buggy device driver.  So the
> LANDLOCK_ACCESS_FS_READ_FILE right grants access to both.  Users who are
> concerned about the security of specific device drivers can enforce a policy
> where only the necessary device files can be opened.

I'm thinking about the case where the FIONREAD value is used by a device
with a completely different semantic. This should be a bad practice but
this could happen, right Christian?

> 
> Does that make sense?
> 
> (Otherwise, if it makes you feel better, we can also change it so that these
> IOCTL commands require LANDLOCK_ACCESS_FS_IOCTL if they are used on non-S_ISREG
> files.  But it would complicate the IOCTL logic a bit, which we are exposing to
> users.)

I think I'd prefer this approach. I'm not sure how special files (but
still S_ISREG) are handled though, nor if this could be an issue.

> 
> 
> > Christian, is it correct to say that device drivers are not "required"
> > to follow the same semantic as the VFS's IOCTLs and that (for whatever
> > reason) collisions may occur? I guess this is not the case for
> > filesystems, which should implement similar semantic for the same
> > IOCTLs.
> 
> Christian, friendly ping! :)  Do you have opinions on this?
> 
> If the Landlock LSM makes decisions based on the IOCTL command numbers, do we
> have to assume that underlying device drivers might expose different
> functionality under the same IOCTL command numbers?
> 
> Thanks,
> —Günther
>