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@xxxxxxxxx/ > V2: https://lore.kernel.org/all/20220712211405.14705-1-gnoack3000@xxxxxxxxx/ > V3: https://lore.kernel.org/all/20220804193746.9161-1-gnoack3000@xxxxxxxxx/ > V4: https://lore.kernel.org/all/20220814192603.7387-1-gnoack3000@xxxxxxxxx/ > V5: https://lore.kernel.org/all/20220817203006.21769-1-gnoack3000@xxxxxxxxx/ > V6: https://lore.kernel.org/all/20220908195805.128252-1-gnoack3000@xxxxxxxxx/ > V7: https://lore.kernel.org/all/20220930160144.141504-1-gnoack3000@xxxxxxxxx/ > V8: https://lore.kernel.org/all/20221001154908.49665-1-gnoack3000@xxxxxxxxx/ > V9: https://lore.kernel.org/all/20221008100935.73706-1-gnoack3000@xxxxxxxxx/ > > 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 >
