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:

quoted

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/ (local)
V2: https://lore.kernel.org/all/20220712211405.14705-1-gnoack3000@gmail.com/ (local)
V3: https://lore.kernel.org/all/20220804193746.9161-1-gnoack3000@gmail.com/ (local)
V4: https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@gmail.com/ (local)

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

--