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

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

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

The rationale goes as follows:

* ftruncate() is already adequately protected by the
  LL_ACCESS_FS_WRITE_FILE right. ftruncate is only permitted on fds
  that are open for writing.
* truncate() is not Landlock-restrictable in Landlock ABI v1,
  so needs to be covered by the new truncate right.
* open() for reading with O_TRUNC is also not Landlock-restrictable in
  Landlock ABI v1, so needs to be covered by the new truncate right.
* open() for writing with O_TRUNC is also not Landlock-restrictable in
  Landlock ABI v1. BUT: A caller who can open the file for writing
  will also be able to ftruncate it - so it doesn't really make sense
  to ask for a different Landlock right here.

Does that approach make sense to you?

I think in terms of changs required for it, it sounds like it would
require a change to the path_truncate LSM hook to distinguish the
cases above.

Do you want a new patch on top of the existing one, or should I rather
create a new version of the old truncate patch set?

--Günther

> On 17/08/2022 22:30, Günther Noack wrote:
> > The goal of these patches is to work towards a more complete coverage
> > of file system operations that are restrictable with Landlock.
> >
> > The known set of currently unsupported file system operations in
> > Landlock is described at [1]. Out of the operations listed there,
> > truncate is the only one that modifies file contents, so these patches
> > should make it possible to prevent the direct modification of file
> > contents with Landlock.
> >
> > The patch introduces the truncation restriction feature as an
> > additional bit in the access_mask_t bitmap, in line with the existing
> > supported operations.
> >
> > The truncation flag covers both the truncate(2) and ftruncate(2)
> > families of syscalls, as well as open(2) with the O_TRUNC flag.
> > This includes usages of creat() in the case where existing regular
> > files are overwritten.
> >
> > Apart from Landlock, file truncation can also be restricted using
> > seccomp-bpf, but it is more difficult to use (requires BPF, requires
> > keeping up-to-date syscall lists) and it is not configurable by file
> > hierarchy, as Landlock is. The simplicity and flexibility of the
> > Landlock approach makes it worthwhile adding.
> >
> > While it's possible to use the "write file" and "truncate" rights
> > independent of each other, it simplifies the mental model for
> > userspace callers to always use them together.
> >
> > Specifically, the following behaviours might be surprising for users
> > when using these independently:
> >
> >   * The commonly creat() syscall requires the truncate right when
> >     overwriting existing files, as it is equivalent to open(2) with
> >     O_TRUNC|O_CREAT|O_WRONLY.
> >   * The "write file" right is not always required to truncate a file,
> >     even through the open(2) syscall (when using O_RDONLY|O_TRUNC).
> >
> > Nevertheless, keeping the two flags separate is the correct approach
> > to guarantee backwards compatibility for existing Landlock users.
> >
> > These patches are based on version 6.0-rc1.
> >
> > Best regards,
> > Günther
> >
> > [1] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags
> >
> > Past discussions:
> > V1: https://lore.kernel.org/all/20220707200612.132705-1-gnoack3000@gmail.com/
> > V2: https://lore.kernel.org/all/20220712211405.14705-1-gnoack3000@gmail.com/
> > V3: https://lore.kernel.org/all/20220804193746.9161-1-gnoack3000@gmail.com/
> > V4: https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@gmail.com/
> >
> > Changelog:
> >
> > V5:
> > * Documentation
> >    * Fix wording in userspace-api headers and in landlock.rst.
> >    * Move the truncation limitation section one to the bottom.
> >    * Move all .rst changes into the documentation commit.
> > * selftests
> >    * Remove _metadata argument from helpers where it became unnecessary.
> >    * Open writable file descriptors at the top of both tests, before Landlock
> >      is enabled, to exercise ftruncate() independently from open().
> >    * Simplify test_ftruncate and decouple it from exercising open().
> >    * test_creat(): Return errno on close() failure (it does not conflict).
> >    * Fix /* comment style */
> >    * Reorder blocks of EXPECT_EQ checks to be consistent within a test.
> >    * Add missing |O_TRUNC to a check in one test.
> >    * Put the truncate_unhandled test before the other.
> >
> > V4:
> >   * Documentation
> >     * Clarify wording and syntax as discussed in review.
> >     * Use a less confusing error message in the example.
> >   * selftests:
> >     * Stop using ASSERT_EQ in test helpers, return EBADFD instead.
> >       (This is an intentionally uncommon error code, so that the source
> >       of the error is clear and the test can distinguish test setup
> >       failures from failures in the actual system call under test.)
> >   * samples/Documentation:
> >     * Use additional clarifying comments in the kernel backwards
> >       compatibility logic.
> >
> > V3:
> >   * selftests:
> >     * Explicitly test ftruncate with readonly file descriptors
> >       (returns EINVAL).
> >     * Extract test_ftruncate, test_truncate, test_creat helpers,
> >       which simplified the previously mixed usage of EXPECT/ASSERT.
> >     * Test creat() behaviour as part of the big truncation test.
> >     * Stop testing the truncate64(2) and ftruncate64(2) syscalls.
> >       This simplifies the tests a bit. The kernel implementations are the
> >       same as for truncate(2) and ftruncate(2), so there is little benefit
> >       from testing them exhaustively. (We aren't testing all open(2)
> >       variants either.)
> >   * samples/landlock/sandboxer.c:
> >     * Use switch() to implement best effort mode.
> >   * Documentation:
> >     * Give more background on surprising truncation behaviour.
> >     * Use switch() in the example too, to stay in-line with the sample tool.
> >     * Small fixes in header file to address previous comments.
> > * misc:
> >    * Fix some typos and const usages.
> >
> > V2:
> >   * Documentation: Mention the truncation flag where needed.
> >   * Documentation: Point out connection between truncation and file writing.
> >   * samples: Add file truncation to the landlock/sandboxer.c sample tool.
> >   * selftests: Exercise open(2) with O_TRUNC and creat(2) exhaustively.
> >   * selftests: Exercise truncation syscalls when the truncate right
> >     is not handled by Landlock.
> >
> > Günther Noack (4):
> >    landlock: Support file truncation
> >    selftests/landlock: Selftests for file truncation support
> >    samples/landlock: Extend sample tool to support
> >      LANDLOCK_ACCESS_FS_TRUNCATE
> >    landlock: Document Landlock's file truncation support
> >
> >   Documentation/userspace-api/landlock.rst     |  52 +++-
> >   include/uapi/linux/landlock.h                |  17 +-
> >   samples/landlock/sandboxer.c                 |  23 +-
> >   security/landlock/fs.c                       |   9 +-
> >   security/landlock/limits.h                   |   2 +-
> >   security/landlock/syscalls.c                 |   2 +-
> >   tools/testing/selftests/landlock/base_test.c |   2 +-
> >   tools/testing/selftests/landlock/fs_test.c   | 257 ++++++++++++++++++-
> >   8 files changed, 336 insertions(+), 28 deletions(-)
> >
> >
> > base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
> > --
> > 2.37.2

--