Re: [PATCH v10 00/11] landlock: truncate support

[Nathan Chancellor <nathan@kernel.org>]

Hi Günther,

On Tue, Oct 18, 2022 at 08:22:05PM +0200, 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.
> 
> Motivation
> ----------
> 
> 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.
> 
> 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.
> 
> Implementation overview
> -----------------------
> 
> 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.
> 
> Additionally, this patch set introduces a new Landlock security blob
> associated with opened files, to track the available Landlock access
> rights at the time of opening the file. This is in line with Unix's
> general approach of checking the read and write permissions during
> open(), and associating this previously checked authorization with the
> opened file.
> 
> In order to treat truncate(2) and ftruncate(2) calls differently in an
> LSM hook, we split apart the existing security_path_truncate hook into
> security_path_truncate (for truncation by path) and
> security_file_truncate (for truncation of previously opened files).
> 
> We also implement the file_alloc_security hook, in order to override
> the access rights in the file security blob, in cases where the file
> is opened by other means than open(2), but where the opened file still
> supports ftruncate(2). This is also demonstrated in a selftest, using
> memfd_create(2).
> 
> Relationship between "truncate" and "write" rights
> --------------------------------------------------
> 
> 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.
> 
> When the "truncate" right is checked for ftruncate(2)
> -----------------------------------------------------
> 
> Notably, for the purpose of ftruncate(2), the Landlock truncation
> access right is looked up when *opening* the file, not when calling
> ftruncate(). The availability of the truncate right is associated with
> the opened file and is later checked to authorize ftruncate(2)
> operations.
> 
> This is similar to how the write mode gets remembered after a
> open(..., O_WRONLY) to authorize later write() operations.
> 
> These opened file descriptors can also be passed between processes and
> will continue to enforce their truncation properties when these
> processes attempt an ftruncate().
> 
> These patches are based on v6.1-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/
> V5: https://lore.kernel.org/all/20220817203006.21769-1-gnoack3000@gmail.com/
> V6: https://lore.kernel.org/all/20220908195805.128252-1-gnoack3000@gmail.com/
> V7: https://lore.kernel.org/all/20220930160144.141504-1-gnoack3000@gmail.com/
> V8: https://lore.kernel.org/all/20221001154908.49665-1-gnoack3000@gmail.com/
> V9: https://lore.kernel.org/all/20221008100935.73706-1-gnoack3000@gmail.com/
> 
> Changelog:
> 
> V10:
> * Align security blob offsets in security/security.c
>   Bug spotted by Nathan Chancellor. (Thanks!!)

I can confirm v10 works for me on top of next-20221018. Thanks a lot for
digging into it and getting it resolved quickly!

Cheers,
Nathan

>   As suggested by Mickaël Salaün, the bugfix is part of change 4/11.
> * Small wording and formatting fixes in comments
>   Merged from Mickaël Salaün's fixes on his -next branch.
> 
> V9:
> * Implement file_alloc_security hook
>   * Needs to grant all Landlock rights by default for use cases where
>     files are opened by other means than open(2), i.e. memfd_create(2)
>   * Discovered and fixed by Mickaël Salaün on his -next branch
>   * Add a selftest for the memfd_create(2) example
> * file_open_hook: Reorder the logic a bit as discussed in review
> * selftests: Return -errno from recv_fd() and send_fd()
> * Rebase to master branch
> * Reorder __maybe_unused patch before its use
> * Various small formatting and documentation fixes in code,
>   documentation and commit messages
> 
> V8:
> * landlock: Refactor check_access_path_dual() into
>   is_access_to_paths_allowed(), as suggested by Mickaël Salaün on the
>   v7 review. Added this as a separate commit.
> * landlock truncate feature: inline get_path_access()
> * Documentation: update documentation date to October 2022
> * selftests: locally #define __maybe_unused (checkpatch started
>   complaining about it, but the header where it's defined is not
>   usable from selftests)
> 
> V7:
> * security: Create file_truncate hook
>   * Fix the build on kernels without CONFIG_SECURITY_PATH (fixed by
>     Mickaël Salaün)
>   * lsm_hooks.h: Document file_truncate hook
>   * fs/open.c: undo accidental indentation changes
> * landlock: Support file truncation
>   * Use the init_layer_masks() result as input for
>     check_access_path_dual()
>   * Naming
>     * Rename the landlock_file_security.allowed_access field
>       (previously called "rights")
>     * Rename get_path_access_rights() to get_path_access()
>     * Rename get_file_access() to get_required_file_open_access() to
>       avoid confusion with get_path_access()
>     * Use "access_request" for access_mask_t variables, access_req for
>       unsigned long
>   * Documentation:
>     * Fixed some comments according to review
>     * Added comments to get_required_file_open_access() and
>       init_layer_masks()
> * selftests:
>   * remove unused variables
>   * rename fd0,...,fd3 to fd_layer0,...,fd_layer3.
>   * test_ftruncate: define layers on top and inline the helper function
> * New tests (both added as separate commits)
>   * More exhaustive ftruncate test: Add open_and_ftruncate test that
>     exercises ftruncate more thoroughly with fixture variants
>   * FD-passing test: exercise restricted file descriptors getting
>     passed between processes, also using the same fixture variants
> * Documentation: integrate review comments by Mickaël Salaün
>   * do not use contraptions (don't)
>   * use double backquotes in all touched lines
>   * add the read/write open() analogy to the truncation docs
>   * in code example, check for abi<0 explicitly and fix indentation
> 
> V6:
> * LSM hooks: create file_truncate hook in addition to path_truncate.
>   Use it in the existing path_truncate call sites where appropriate.
> * landlock: check LANDLOCK_ACCESS_FS_TRUNCATE right during open(), and
>   associate that right with the opened struct file in a security blob.
>   Introduce get_path_access_rights() helper function.
> * selftests: test ftruncate in a separate test, to exercise that
>   the rights are associated with the file descriptor.
> * Documentation: Rework documentation to reflect new ftruncate() semantics.
> * Applied small fixes by Mickaël Salaün which he added on top of V5, in
>   https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next
>   (I hope I found them all.)
> 
> 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 (11):
>   security: Create file_truncate hook from path_truncate hook
>   landlock: Refactor check_access_path_dual() into
>     is_access_to_paths_allowed()
>   landlock: Document init_layer_masks() helper
>   landlock: Support file truncation
>   selftests/landlock: Test file truncation support
>   selftests/landlock: Test open() and ftruncate() in multiple scenarios
>   selftests/landlock: Locally define __maybe_unused
>   selftests/landlock: Test FD passing from restricted to unrestricted
>     processes
>   selftests/landlock: Test ftruncate on FDs created by memfd_create(2)
>   samples/landlock: Extend sample tool to support
>     LANDLOCK_ACCESS_FS_TRUNCATE
>   landlock: Document Landlock's file truncation support
> 
>  Documentation/userspace-api/landlock.rst     |  67 ++-
>  fs/namei.c                                   |   2 +-
>  fs/open.c                                    |   2 +-
>  include/linux/lsm_hook_defs.h                |   1 +
>  include/linux/lsm_hooks.h                    |  10 +-
>  include/linux/security.h                     |   6 +
>  include/uapi/linux/landlock.h                |  21 +-
>  samples/landlock/sandboxer.c                 |  12 +-
>  security/apparmor/lsm.c                      |   6 +
>  security/landlock/fs.c                       | 206 ++++++--
>  security/landlock/fs.h                       |  24 +
>  security/landlock/limits.h                   |   2 +-
>  security/landlock/setup.c                    |   1 +
>  security/landlock/syscalls.c                 |   2 +-
>  security/security.c                          |  16 +-
>  security/tomoyo/tomoyo.c                     |  13 +
>  tools/testing/selftests/landlock/base_test.c |  38 +-
>  tools/testing/selftests/landlock/common.h    |  85 +++-
>  tools/testing/selftests/landlock/fs_test.c   | 468 ++++++++++++++++++-
>  19 files changed, 862 insertions(+), 120 deletions(-)
> 
> 
> base-commit: 9abf2313adc1ca1b6180c508c25f22f9395cc780
> -- 
> 2.38.0
>