Re: [PATCH v5 0/4] landlock: truncate support

["Günther Noack" <gnoack3000@gmail.com>]

On Fri, Sep 02, 2022 at 10:40:57AM +0200, Mickaël Salaün wrote:
> On 02/09/2022 08:16, Günther Noack wrote:
> > On Fri, Sep 02, 2022 at 07:32:49AM +0200, Günther Noack wrote:
> > > On Thu, Sep 01, 2022 at 07:10:38PM +0200, Mickaël Salaün wrote:
> > > > Hmm, I think there is an issue with this series. Landlock only enforces
> > > > restrictions at open time or when dealing with user-supplied file paths
> > > > (relative or absolute).
> > >
> > > Argh, ok. That sounds like a desirable property, although it would
> > > mean reworking the patch set.
> > >
> > > > The use of the path_truncate hook in this series
> > > > doesn't distinguish between file descriptor from before the current sandbox
> > > > or from after being sandboxed. For instance, if a file descriptor is
> > > > received through a unix socket, it is assumed that this is legitimate and no
> > > > Landlock restriction apply on it, which is not the case with this series
> > > > anymore. It is the same for files opened before the process sandbox itself.
> > > >
> > > > To be able to follow the current semantic, I think we should control the
> > > > truncate access at open time (or when dealing with a user-supplied path) but
> > > > not on any file descriptor as it is currently done.
> > >
> > > OK - so let me try to make a constructive proposal. We have previously
> > > identified a few operations where a truncation happens, and I would
> > > propose that the following Landlock rights should be needed for these:
> > >
> > > * truncate() (operating on char *path): Require LL_ACCESS_FS_TRUNCATE
> > > * ftruncate() (operating on fd): No Landlock rights required
> > > * open() for reading with O_TRUNC: Require LL_ACCESS_FS_TRUNCATE
> > > * open() for writing with O_TRUNC: Require LL_ACCESS_FS_WRITE_FILE
> >
> > Thinking about it again, another alternative would be to require
> > TRUNCATE as well when opening a file for writing - it would be
> > logical, because the resulting FD can be truncated. It would also
> > require people to provide the truncate right in order to open files
> > for writing, but this may be the logical thing to do.
>
> Another alternative would be to keep the current semantic but ignore file
> descriptors from not-sandboxed processes. This could be possible by
> following the current file->f_mode logic but using the Landlock's
> file->f_security instead to store if the file descriptor was opened in a
> context allowing it to be truncated: file opened outside of a landlocked
> process, or in a sandbox allowing LANDLOCK_ACCESS_FS_TRUNCATE on the related
> path.

I'm not convinced that it'll be worth distinguishing between a FD
opened for writing and a FD opened for writing+truncation. And whether
the FD is open for writing is already tracked by default and
ftruncate() checks that.

I'm having a hard time constructing a scenario where write() should be
permitted on an FD but ftruncate() should be forbidden. It seems that
write() is the more dangerous operation of the two, with more
potential to modify a file to one's liking, whereas the modifications
possible through TRUNCATE are relatively benign?

The opposite scenario (where ftruncate() is permitted and write() is
forbidden) simply can't exist because an FD must already be writable
in order to use ftruncate(). (see man page)

Additionally, if we recall previous discussions on the truncate patch
sets, there is the very commonly used creat() syscall (a.k.a. open()
with O_CREAT|O_WRONLY|O_TRUNC), which anyway requires the Landlock
truncate right in many cases. So I still think you can't actually use
LANDLOCK_ACCESS_FS_FILE_WRITE without also providing the
LANDLOCK_ACCESS_FS_TRUNCATE right?

In conclusion, I'd be in favor of not tracking the truncate right
separately as a property of an open file descriptor. Does that
rationale sound reasonable?

--