Hello!
Thanks for the detailed review again! The FIONREAD implementation that you
suggested works. With FIOQSIZE I ran into some surprises - I believe the check
is a noop - more details below.
On Sat, Aug 26, 2023 at 08:26:30PM +0200, Mickaël Salaün wrote:
> On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote:
> > On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote:
> > > 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.
Just paraphrasing for later reference, because I almost misunderstood it:
FIONREAD should work even when LANDLOCK_ACCESS_FS_IOCTL is *handled*,
which is lingo for "listed in the ruleset_attr.handled_access_fs".
When it is listed there, that means that the Landlock policy does not
grant the LANDLOCK_ACCESS_FS_IOCTL right.
So we treat FIONREAD as blanket-permitted independent of the
LANDLOCK_ACCESS_FS_IOCTL right, under the condition that we have
LANDLOCK_ACCESS_FS_READ_FILE for the file. -- Sounds good to me, will do.
> > > 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.
>
> FIOQSIZE should in fact only be allowed if LANDLOCK_ACCESS_FS_READ_FILE or
> LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled
> and explicitly allowed for the FD. I guess FIOQSIZE is allowed without read
> nor write mode (i.e. O_PATH), so it could be an issue for landlocked
> applications but they can explicitly allow IOCTL for this case. When
> we'll have a LANDLOCK_ACCESS_FS_READ_METADATA (or something similar), we
> should also tie FIOQSIZE to this access right, and we'll be able to
> fully handle all the use cases without fully allowing all other IOCTLs.
I implemented this check for the Landlock access rights in the ioctl hook, but
when testing it I realized that I could not ever get it to fail in practice:
ioctl(2) generally returns EBADF when the file was opened with O_PATH. Early in
the ioctl(2) syscall implementation, it returns EBADF when the struct fd does
not have the ->file attribute set. (This is even true for the commands to
manipulate the Close-on-exec flag, which don't strictly need that. But they
might work through fcntl.)
In my understanding from the open(2) man page, the only ways to open files are
with one of O_RDONLY, O_RDWR, O_WRONLY or O_PATH:
- O_RDONLY: we had LANDLOCK_ACCESS_FS_READ_FILE at the time of open(2).
- O_WRONLY: we had LANDLOCK_ACCESS_FS_WRITE_FILE at the time of open(2).
- O_RDWR: we had both of these two rights at the time of open(2).
- O_PATH: any ioctl(2) attempt returns EBADF early on
So at the time that the ioctl security hook gets called, we already know that
the user must have had one of the LANDLOCK_ACCESS_FS_READ_FILE or
LANDLOCK_ACCESS_FS_WRITE_FILE rights -- checking for it again is not strictly
necessary?
Am I missing something here? (In particular, am I overlooking additional ways
to call open(2) where the read and write rights are not necessary, other than
O_PATH?)
I'd propose this path forward: Let's keep the check for the rights as you
suggested, but I would just keep it as an additional safety net there, for
Landlock's internal consistency, and in case that future Linux versions
introduce new ways to open files. I believe that at the moment, that check is
equivalent to always permitting the FIOQSIZE command in that hook (with the same
logic as for FIOCLEX, FIONCLEX etc).
> > > (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:
>
> We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode
> checks. A kselftest test should check that ENOTTY is returned according
> to the file type and the access rights.
It's not clear to me why we would need to add the same i_mode checks for
S_ISDIR() || S_ISREG() || S_ISLNK() there? If these checks in do_vfs_ioctl()
fail, it returns -ENOTTY. Is that not an appropriate error already?
> > > 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.
Done.
—Günther
