On Fri, Nov 17, 2023 at 09:44:31PM +0100, Mickaël Salaün wrote:
> On Fri, Nov 17, 2023 at 03:44:20PM +0100, Günther Noack wrote:
> > On Thu, Nov 16, 2023 at 04:49:09PM -0500, Mickaël Salaün wrote:
> > > On Fri, Nov 03, 2023 at 04:57:10PM +0100, Günther Noack wrote:
> > > And patch the landlock_create_ruleset() helper with that:
> > >
> > > - if (!fs_access_mask && !net_access_mask)
> > > + if (WARN_ON_ONCE(!fs_access_mask) && !net_access_mask)
> > > return ERR_PTR(-ENOMSG);
> >
> > Why would you want to warn on the case where fs_access_mask is zero?
>
> Because in my suggestion the real check is moved/copied to
> landlock_expand_fs_access(), which is called before, and it should then
> not be possible to have this case here.
Oh, I see, I misread that code. I guess it does not apply to the version that
we ended up with.
> > > > * When LANDLOCK_ACCESS_FS_IOCTL is granted on a file hierarchy,
> > > > should this grant the permission to use *any* IOCTL? (Right now,
> > > > it is any IOCTL except for the ones covered by the IOCTL groups,
> > > > and it's a bit weird that the scope of LANDLOCK_ACCESS_FS_IOCTL
> > > > becomes smaller when other access rights are also handled.
> > >
> > > Are you suggesting to handle differently this right if it is applied to
> > > a directory?
> >
> > No - this applies to files as well. I am suggesting that granting
> > LANDLOCK_ACCESS_FS_IOCTL on a file or file hierarchy should always give access
> > to *all* ioctls, both the ones in the synthetic groups and the remaining ones.
> >
> > Let me spell out the scenario:
> >
> > Steps to reproduce:
> > - handle: LANDLOCK_ACCESS_FS_IOCTL | LANDLOCK_ACCESS_FS_READ_FILE
> > - permit: LANDLOCK_ACCESS_FS_IOCTL
> > on file f
> > - open file f (for write-only)
> > - attempt to use ioctl(fd, FIOQSIZE, ...)
> >
> > With this patch set:
> > - ioctl(fd, FIOQSIZE, ...) fails,
> > because FIOQSIZE is part of IOCTL_CMD_G1
> > and because LANDLOCK_ACCESS_FS_READ_FILE is handled,
> > IOCTL_CMD_G1 is only unlocked through LANDLOCK_ACCESS_FS_READ_FILE
>
> Correct, and it looks consistent to me.
>
> >
> > Alternative proposal:
> > - ioctl(fd, FIOQSIZE, ...) should maybe work,
> > because LANDLOCK_ACCESS_FS_IOCTL is permitted on f
> >
> > Implementation-wise, this would mean to add
> >
> > expand_ioctl(handled, access, LANDLOCK_ACCESS_FS_IOCTL, ioctl_groups)
> >
> > to expand_all_ioctl().
> >
> > I feel that this alternative might be less surprising, because granting the
> > IOCTL right would grant all the things that were restricted when handling the
> > IOCTL right, and it would be more "symmetric".
> >
> > What do you think?
>
> I though that we discussed about that and we agree that it was the way
> to go. Cf. the table of handled/allowed/not-allowed.
We can go with the current implementation as well, I don't feel very strongly
about it.
> Why would LANDLOCK_ACCESS_FS_IOCTL grant access to FIOQSIZE in the case
> of a directory but not a file? These would be two different semantics.
If the ruleset were enforced in that proposal, as in the example above, it would
not distinguish whether the affected filesystem paths are files or directories.
If LANDLOCK_ACCESS_FS_IOCTL is handled, the semantics would be:
* If you permit LANDLOCK_ACCESS_FS_READ_FILE on a directory or file,
it would become possible to use these ioctl commands on the affected files
which are relevant and harmless for reading files. (As before)
* If you permit LANDLOCK_ACCESS_FS_IOCTL on a directory or file,
it would become possible to use *all* ioctl commands on the affected files.
(That is the difference. In the current implementation, this only affects
the ioctl commands which are *not* in the synthetic groups. In the
alternative proposal, it would affect *all* ioctl commands.
I think this might be simpler to reason about, because the set of ioctl
commands which are affected by permitting(!) LANDLOCK_ACCESS_FS_IOCTL would
always be the same (namely, all ioctl commands), and it would not be
dependent on whether other access rights are handled.)
I don't think it is at odds with the backwards-compatibility concerns which we
previously discussed. The synthetic groups still exist, it's just the
"permitting LANDLOCK_ACCESS_FS_IOCTL on a file or directory" which affects a
different set of IOCTL commands.
> > > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should
> > > be OK. But maybe we should rename this right to something like
> > > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles
> > > IOCTLs that are not handled by other access rights?
> >
> > Hmm, I'm not convinced this is a good name. It makes sense in the context of
> > allowing "all the other ioctls" for a file or file hierarchy, but when setting
> > LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls,
> > so "default" doesn't seem appropriate to me.
>
> It should turn off all IOCTLs that are not handled by another access
> right. The handled access rights should be expanded the same way as the
> allowed access rights.
If you handle LANDLOCK_ACCESS_FS_IOCTL, and you don't permit anything on files
or directories, all IOCTL commands will be forbidden, independent of what else
is handled.
The opposite is not true:
If you handle LANDLOCK_ACCESS_FS_READ_FILE, and you don't handle
LANDLOCK_ACCESS_FS_IOCTL, all IOCTL commands will happily work.
So if you see it through that lens, you could say that it is only the
LANDLOCK_ACCESS_FS_IOCTL bit in the "handled" mask which forbids any IOCTL
commands at all.
I hope this makes sense. It's not my intent to open this
backwards-compatibility can of worms from scratch... :)
—Günther
