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:

quoted

On Fri, Sep 02, 2022 at 07:32:49AM +0200, Günther Noack wrote:

quoted

On Thu, Sep 01, 2022 at 07:10:38PM +0200, Mickaël Salaün wrote:

quoted

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.

quoted

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?

--