| Message ID | 20230814172816.3907299-1-gnoack@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Landlock: IOCTL support | expand |
On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote: > Hello! > > These patches add simple ioctl(2) support to Landlock. > > Objective > ~~~~~~~~~ > > Make ioctl(2) requests restrictable with Landlock, > in a way that is useful for real-world applications. > > Proposed approach > ~~~~~~~~~~~~~~~~~ > > Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use > of ioctl(2) on file descriptors. > > We attach the LANDLOCK_ACCESS_FS_IOCTL right to opened file > descriptors, as we already do for LANDLOCK_ACCESS_FS_TRUNCATE. > > We make an exception for the common and known-harmless IOCTL commands FIOCLEX, > FIONCLEX, FIONBIO, FIOASYNC and FIONREAD. These IOCTL commands are always > permitted. The functionality of the first four is already available through > fcntl(2), and FIONREAD only returns the number of ready-to-read bytes. > > I believe that this approach works for the majority of use cases, and > offers a good trade-off between Landlock API and implementation > complexity and flexibility when the feature is used. > > Current limitations > ~~~~~~~~~~~~~~~~~~~ > > With this patch set, ioctl(2) requests can *not* be filtered based on > file type, device number (dev_t) or on the ioctl(2) request number. > > On the initial RFC patch set [1], we have reached consensus to start > with this simpler coarse-grained approach, and build additional IOCTL > restriction capabilities on top in subsequent steps. > > [1] https://lore.kernel.org/linux-security-module/d4f1395c-d2d4-1860-3a02-2a0c023dd761@digikod.net/ > > Notable implications of this approach > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > * Existing inherited file descriptors stay unaffected > when a program enables Landlock. > > This means in particular that in common scenarios, > the terminal's IOCTLs (ioctl_tty(2)) continue to work. > > * ioctl(2) continues to be available for file descriptors acquired > through means other than open(2). Example: Network sockets, > memfd_create(2), file descriptors that are already open before the > Landlock ruleset is enabled. > > Examples > ~~~~~~~~ > > Starting a sandboxed shell from $HOME with samples/landlock/sandboxer: > > LL_FS_RO=/ LL_FS_RW=. ./sandboxer /bin/bash > > The LANDLOCK_ACCESS_FS_IOCTL right is part of the "read-write" rights > here, so we expect that newly opened files outside of $HOME don't work > with ioctl(2). > > * "stty" works: It probes terminal properties > > * "stty </dev/tty" fails: /dev/tty can be reopened, but the IOCTL is > denied. > > * "eject" fails: ioctls to use CD-ROM drive are denied. > > * "ls /dev" works: It uses ioctl to get the terminal size for > columnar layout > > * The text editors "vim" and "mg" work. (GNU Emacs fails because it > attempts to reopen /dev/tty.) > > How we arrived at the list of always-permitted IOCTL commands > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > To decide which IOCTL commands should be blanket-permitted I went through the > list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually > to understand what they are about. The following list is my conclusion from > that. > > We should always allow the following IOCTL commands: > > * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the > close-on-exec flag I agree that FIOCLEX and FIONCLEX should always be allowed. > * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO > and async flags About FIONBIO and FIOASYNC, I think it's OK because it is already allowed thanks to fcntl. We could combine these commands with LANDLOCK_ACCESS_FS_{READ,WRITE}_FILE but it may not be related to only regular files. I found that there is an inconsistency between the fcntl's SETFL and the FIONBIO/FIOASYNC IOCTLs. The first one call filp->f_op->check_flags() while the second one doesn't. This should enable to bypass such check_flags() checks. This is unrelated to Landlock or this patch series though, and it should be OK because only NFS seems to implement check_flags() and only O_DIRECT|O_APPEND are checked. Cc Christian > * FIONREAD - get the number of bytes available for reading (the implementation > is defined per file type) About FIONREAD, I'm convinced we should only allow this command according to LANDLOCK_ACCESS_FS_READ. As for the VFS implementation, it should also depend on the file being a regular file (otherwise any driver could implement another semantic). To make it forward compatible (with the same semantic), I think we should handle specific IOCTLs this way: if the complementary access right (e.g. LANDLOCK_ACCESS_FS_READ_FILE) is not handled by the ruleset (which is not the same as allowed by a rule), then the related IOCTLs (e.g. FIONREAD) are denied, otherwise the related IOCTLs are only allowed if the complementary access is explicitly allowed for this FD. This way of delegating enables to extend the access control to IOCTL commands while preserving the access (rights) semantic. This will enable for instance to restrict FS_IOC_GETFLAGS according to a potential future LANDLOCK_ACCESS_FS_READ_METADATA, while keeping the same IOCTL restriction semantic. We could also follow this same semantic for synthetic access rights grouping IOCTLs commands. > > The first four are also available through fcntl with the F_SETFD and F_SETFL > commands. > > The following commands mentioned in fs/ioctl.c should be guarded by the > LANDLOCK_ACCESS_FS_IOCTL access right, the same as the other ioctl commands, > because they are nontrivial: > > * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file > system. Requires CAP_SYS_ADMIN. > * FICLONE, FICLONERANGE, FIDEDUPRANGE - making files share physical storage > between multiple files. These only work on some file systems, by design. > * Commands that read file system internals: > * FS_IOC_FIEMAP - get information about file extent mapping > (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt) > * FIBMAP - get a file's file system block number > * FIGETBSZ - get file system blocksize > * Accessing file attributes: > * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2)) > * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes > * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64, > FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS preallocation > syscalls which predate fallocate(2). > > Related Work > ~~~~~~~~~~~~ > > OpenBSD's pledge(2) [2] restricts ioctl(2) independent of the file > descriptor which is used. The implementers maintain multiple > allow-lists of predefined ioctl(2) operations required for different > application domains such as "audio", "bpf", "tty" and "inet". > > OpenBSD does not guarantee ABI backwards compatibility to the same > extent as Linux does, so it's easier for them to update these lists in > later versions. It might not be a feasible approach for Linux though. > > [2] https://man.openbsd.org/OpenBSD-7.3/pledge.2 > > Changes > ~~~~~~~ > > V3: > * always permit the IOCTL commands FIOCLEX, FIONCLEX, FIONBIO, FIOASYNC and > FIONREAD, independent of LANDLOCK_ACCESS_FS_IOCTL > * increment ABI version in the same commit where the feature is introduced > * testing changes > * use FIOQSIZE instead of TTY IOCTL commands > (FIOQSIZE works with regular files, directories and memfds) > * run the memfd test with both Landlock enabled and disabled > * add a test for the always-permitted IOCTL commands > > V2: > * rebased on mic-next > * added documentation > * exercise ioctl(2) in the memfd test > * test: Use layout0 for the test > > --- > > V1: https://lore.kernel.org/linux-security-module/20230502171755.9788-1-gnoack3000@gmail.com/ > V2: https://lore.kernel.org/linux-security-module/20230623144329.136541-1-gnoack@google.com/ > > Günther Noack (5): > landlock: Add ioctl access right > selftests/landlock: Test ioctl support > selftests/landlock: Test ioctl with memfds > samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL > landlock: Document ioctl support > > Documentation/userspace-api/landlock.rst | 74 ++++++++--- > include/uapi/linux/landlock.h | 31 +++-- > samples/landlock/sandboxer.c | 12 +- > security/landlock/fs.c | 38 +++++- > 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 | 133 +++++++++++++++++-- > 8 files changed, 249 insertions(+), 45 deletions(-) > > > base-commit: 35ca4239929737bdc021ee923f97ebe7aff8fcc4 > -- > 2.41.0.694.ge786442a9b-goog >
On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote: > Hello! > > These patches add simple ioctl(2) support to Landlock. > [...] > How we arrived at the list of always-permitted IOCTL commands > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > To decide which IOCTL commands should be blanket-permitted I went through the > list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually > to understand what they are about. The following list is my conclusion from > that. > > We should always allow the following IOCTL commands: > > * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the > close-on-exec flag > * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO > and async flags > * FIONREAD - get the number of bytes available for reading (the implementation > is defined per file type) I think we should treat FIOQSIZE like FIONREAD, i.e. check for LANDLOCK_ACCESS_FS_READ_FILE as explain in my previous message. Tests should then rely on something else. [...] > Changes > ~~~~~~~ > > V3: > * always permit the IOCTL commands FIOCLEX, FIONCLEX, FIONBIO, FIOASYNC and > FIONREAD, independent of LANDLOCK_ACCESS_FS_IOCTL > * increment ABI version in the same commit where the feature is introduced > * testing changes > * use FIOQSIZE instead of TTY IOCTL commands > (FIOQSIZE works with regular files, directories and memfds) > * run the memfd test with both Landlock enabled and disabled > * add a test for the always-permitted IOCTL commands
Here is a proposal to restrict TTY with Landlock, complementary to this patch series (which should land before any other IOCTL-related features). CCing folks part of TIOCSTI discussions, as a complementary approach to https://lore.kernel.org/all/ZN+X6o3cDWcLoviq@google.com/ On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote: > Hello! > > These patches add simple ioctl(2) support to Landlock. > > Objective > ~~~~~~~~~ > > Make ioctl(2) requests restrictable with Landlock, > in a way that is useful for real-world applications. > > Proposed approach > ~~~~~~~~~~~~~~~~~ > > Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use > of ioctl(2) on file descriptors. > > We attach the LANDLOCK_ACCESS_FS_IOCTL right to opened file > descriptors, as we already do for LANDLOCK_ACCESS_FS_TRUNCATE. > > We make an exception for the common and known-harmless IOCTL commands FIOCLEX, > FIONCLEX, FIONBIO, FIOASYNC and FIONREAD. These IOCTL commands are always > permitted. The functionality of the first four is already available through > fcntl(2), and FIONREAD only returns the number of ready-to-read bytes. > > I believe that this approach works for the majority of use cases, and > offers a good trade-off between Landlock API and implementation > complexity and flexibility when the feature is used. > > Current limitations > ~~~~~~~~~~~~~~~~~~~ > > With this patch set, ioctl(2) requests can *not* be filtered based on > file type, device number (dev_t) or on the ioctl(2) request number. > > On the initial RFC patch set [1], we have reached consensus to start > with this simpler coarse-grained approach, and build additional IOCTL > restriction capabilities on top in subsequent steps. > > [1] https://lore.kernel.org/linux-security-module/d4f1395c-d2d4-1860-3a02-2a0c023dd761@digikod.net/ > > Notable implications of this approach > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > * Existing inherited file descriptors stay unaffected > when a program enables Landlock. > > This means in particular that in common scenarios, > the terminal's IOCTLs (ioctl_tty(2)) continue to work. > > * ioctl(2) continues to be available for file descriptors acquired > through means other than open(2). Example: Network sockets, > memfd_create(2), file descriptors that are already open before the > Landlock ruleset is enabled. Digging through all potential malicious use of TTYs and because TTYs are part of process management (e.g. signaling) and mediated by the kernel, I changed my mind and I think we should investigate on protecting shared TTY thanks to Landlock, but not without context and not only against TIOCSTI and TIOCLINUX IOCTLs. TIOCSTI is abused since a few decades [1] [2], and as Günther said, it is still the case [3] as with TIOCLINUX [4] [5]. Since Linux 6.2, CONFIG_LEGACY_TIOCSTI (and the corresponding sysctl knob) can be set to deny all use of TIOCSTI. This kernel configuration is a good step forward but it may not be enabled everywhere because it is a system-wide restriction. Moreover, it is not a sandboxing feature which means developers cannot safely protect users from their applications without impacting the whole system. Making Landlock able to protect against this kind of attack and other TTY-based ones (e.g. snoop keystrokes [6]) is definitely something worth it. The behavior change should only affect a TTY which is shared (same session or not) with a set of processes containing at least one sandboxed process. The simplest and more generic solution I came up with is to tie the TTY (e.g. PTY slave) with the Landlock domain (if any) of the *first process* to be a session leader with this TTY. For all sandboxed processes, if the TTY's domain is more privileged than the process's domain, then any TIOCSTI should be denied. For the snooping protection, I think we could enforce that only the (current) session leader can read the TTY. Same goes for writing to the TTY (but this should already be covered). Basically, all IOCTLs that enable, one way or another, to fool a user should be restricted as TIOCSTI. This includes copy/paste requests (TIOCLINUX subcommands), but also potentially font change (e.g. PIO_FONT), keyboard mapping change, all CAP_SYS_TTY_CONFIG-checked IOCTLs, and probably more. The goal is not to protect against potentially annoying features such as keyboard light changes though, but really to protect integrity and confidentiality of data going through the TTY. The goal is to enforce Landlock security boundaries across TTY's clients. In a nutshell, if a process is sandboxed, only allow read, write and most IOCTL requests if the TTY's domain would be ptracable (see security/landlock/ptrace.c), otherwise deny such action. I think this algorithm would fit well: * if the current process is not sandboxed, then allow * else if the TTY's domain is the same or a child of the current process's domain, then allow (including the TIOCSTI IOCTL) * else if the current process is the session leader of this TTY, then allow read/write/non-TIOCSTI-IOCTLs * else deny The challenge would be to make these checks efficient, especially for the read and write syscalls. When setting the session leader, we could update the TTY's domain with the highest-privileged one, or the NULL domain (i.e. the root/unsandboxed). However, this would mean that previous TIOCSTI requests could have been allowed and could now impact the current (higher privileged) session leader. I think this cannot be properly mitigated solely at the access control level. I'd prefer to properly document this limitation for which I don't see any valid use case. We should test if this theory works in practice with real-world applications though. The question is: are they any programs that pass a TTY FD to a (potentially malicious but sandboxed) process, and *then* switch for the first time to a session leader with this TTY? We might also not want to return EPERM for all kind of requests but EIO instead. Because of compatibility reasons, and different use cases, these restrictions should only be enforced between Landlock domains that opt-in for this feature thanks to a new ruleset's flag, something like LANDLOCK_RULESET_SCOPE_TTY. So all mentions of Landlock domains should in fact only refer to TTY-restricted-domains. As for the upcoming Landlock network restrictions, these TTY restrictions should be independent from any FS-related actions (e.g. mount). BTW, the TIOCSTI would be useful to test (cf. kselftest) this kind of restrictions. What do you think? [1] https://isopenbsdsecu.re/mitigations/tiocsti/ [2] https://jdebp.uk/FGA/TIOCSTI-is-a-kernel-problem.html [3] https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=TIOCSTI [4] https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=TIOCLINUX [5] https://lore.kernel.org/all/ZN+X6o3cDWcLoviq@google.com/ [6] https://gist.github.com/thejh/e163071dfe4c96a9f9b589b7a2c24fc6
Hi! On Fri, Aug 18, 2023 at 03:39:19PM +0200, Mickaël Salaün wrote: > On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote: > > These patches add simple ioctl(2) support to Landlock. > > [...] > > > How we arrived at the list of always-permitted IOCTL commands > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > To decide which IOCTL commands should be blanket-permitted I went through the > > list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually > > to understand what they are about. The following list is my conclusion from > > that. > > > > We should always allow the following IOCTL commands: > > > > * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the > > close-on-exec flag > > * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO > > and async flags > > * FIONREAD - get the number of bytes available for reading (the implementation > > is defined per file type) > > I think we should treat FIOQSIZE like FIONREAD, i.e. check for > LANDLOCK_ACCESS_FS_READ_FILE as explain in my previous message. > Tests should then rely on something else. OK, I rewrote the tests to use FS_IOC_GETFLAGS. Some thoughts on these two IOCTLs: FIONREAD gives the number of bytes that are ready to read. This IOCTL seems only useful when the file is open for reading. However, do you think that we should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b) f->f_mode & FMODE_READ? (The difference is that in case (a), FIONREAD will work if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE right for that file. In case (b), it would only work if you also opened the file for reading.) FIOQSIZE seems like it would be useful for both reading *and* writing? -- The reading case is obvious, but for writers it's also useful if you want to seek around in the file, and make sure that the position that you seek to already exists. (I'm not sure whether that use case is relevant in practical applications though.) -- Why would FIOQSIZE only be useful for readers? (In fact, it seems to me almost like FIOQSIZE might rather be missing a security hook check for one of the "getting file attribute" hooks?) So basically, the implementation that I currently ended up with is: switch (cmd) { case FIOCLEX: case FIONCLEX: case FIONBIO: case FIOASYNC: case FIOQSIZE: return 0; case FIONREAD: if (file->f_mode & FMODE_READ) return 0; } (with some comments in the source code, of course...) Does that look reasonable to you? —Günther
On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote: > Hi! > > On Fri, Aug 18, 2023 at 03:39:19PM +0200, Mickaël Salaün wrote: > > On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote: > > > These patches add simple ioctl(2) support to Landlock. > > > > [...] > > > > > How we arrived at the list of always-permitted IOCTL commands > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > To decide which IOCTL commands should be blanket-permitted I went through the > > > list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually > > > to understand what they are about. The following list is my conclusion from > > > that. > > > > > > We should always allow the following IOCTL commands: > > > > > > * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the > > > close-on-exec flag > > > * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO > > > and async flags > > > * FIONREAD - get the number of bytes available for reading (the implementation > > > is defined per file type) > > > > I think we should treat FIOQSIZE like FIONREAD, i.e. check for > > LANDLOCK_ACCESS_FS_READ_FILE as explain in my previous message. > > Tests should then rely on something else. > > OK, I rewrote the tests to use FS_IOC_GETFLAGS. > > Some thoughts on these two IOCTLs: > > FIONREAD gives the number of bytes that are ready to read. This IOCTL seems > only useful when the file is open for reading. However, do you think that we > should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b) > f->f_mode & FMODE_READ? (The difference is that in case (a), FIONREAD will work > if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE > right for that file. In case (b), it would only work if you also opened the > file for reading.) I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD. f->f_mode & FMODE_READ would make sense but it should have been handled by the kernel; Landlock should not try to fix this inconsistency. These are good test cases though. It should be noted that SELinux considers FIONREAD as tied to metadata reading (FILE__GETATTR). It think it makes more sense to tie it to the read right (LANDLOCK_ACCESS_FS_READ_FILE) because it might be (legitimately) required to properly read a file and FIONREAD is tied to the content of a file, not file attributes. > > FIOQSIZE seems like it would be useful for both reading *and* writing? -- The > reading case is obvious, but for writers it's also useful if you want to seek > around in the file, and make sure that the position that you seek to already > exists. (I'm not sure whether that use case is relevant in practical > applications though.) -- Why would FIOQSIZE only be useful for readers? Good point! The use case you define for writing is interesting. However, would it make sense to seek at a specific offset without being able to know/read the content? I guest not in theory, but in practice we might want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is they only require to write (at a specific offset), or to deal with write errors. Anyway, I guess that this information can be inferred by trying to seek at a specific offset. The only limitation that this approach would bring is that it seems that we can seek into an FD even without read nor write right, and there is no specific (LSM) access control for this operation (and nobody seems to care about being able to read the size of a symlink once opened). If this is correct, I think we should indeed always allow FIOQSIZE. Being able to open a file requires LANDLOCK_ACCESS_FS_READ or WRITE anyway. It would be interesting to check and test with O_PATH though. > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security > hook check for one of the "getting file attribute" hooks?) > > So basically, the implementation that I currently ended up with is: > Before checking these commands, we first need to check that the original domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new bit and replace the file's allowed_access field (see landlock_add_fs_access_mask() and similar helpers in the network patch series that does a similar thing but for ruleset's handle access rights), but here is the idea: if (!landlock_file_handles_ioctl(file)) return 0; > switch (cmd) { /* * Allows file descriptor and file description operations (see * fcntl commands). */ > case FIOCLEX: > case FIONCLEX: > case FIONBIO: > case FIOASYNC: > case FIOQSIZE: > return 0; > case FIONREAD: > if (file->f_mode & FMODE_READ) We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of FMODE_READ. > return 0; > } > > (with some comments in the source code, of course...) > > Does that look reasonable to you? > > —Günther > > -- > Sent using Mutt
On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote: > On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote: > > Hi! > > > > On Fri, Aug 18, 2023 at 03:39:19PM +0200, Mickaël Salaün wrote: > > > On Mon, Aug 14, 2023 at 07:28:11PM +0200, Günther Noack wrote: > > > > These patches add simple ioctl(2) support to Landlock. > > > > > > [...] > > > > > > > How we arrived at the list of always-permitted IOCTL commands > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > To decide which IOCTL commands should be blanket-permitted I went through the > > > > list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually > > > > to understand what they are about. The following list is my conclusion from > > > > that. > > > > > > > > We should always allow the following IOCTL commands: > > > > > > > > * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the > > > > close-on-exec flag > > > > * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO > > > > and async flags > > > > * FIONREAD - get the number of bytes available for reading (the implementation > > > > is defined per file type) > > > > > > I think we should treat FIOQSIZE like FIONREAD, i.e. check for > > > LANDLOCK_ACCESS_FS_READ_FILE as explain in my previous message. > > > Tests should then rely on something else. > > > > OK, I rewrote the tests to use FS_IOC_GETFLAGS. > > > > Some thoughts on these two IOCTLs: > > > > FIONREAD gives the number of bytes that are ready to read. This IOCTL seems > > only useful when the file is open for reading. However, do you think that we > > should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b) > > f->f_mode & FMODE_READ? (The difference is that in case (a), FIONREAD will work > > if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE > > right for that file. In case (b), it would only work if you also opened the > > file for reading.) > > I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled > and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD. > > f->f_mode & FMODE_READ would make sense but it should have been handled > by the kernel; Landlock should not try to fix this inconsistency. > > These are good test cases though. > > It should be noted that SELinux considers FIONREAD as tied to metadata > reading (FILE__GETATTR). It think it makes more sense to tie it to the > read right (LANDLOCK_ACCESS_FS_READ_FILE) because it might be > (legitimately) required to properly read a file and FIONREAD is tied to > the content of a file, not file attributes. > > > > > FIOQSIZE seems like it would be useful for both reading *and* writing? -- The > > reading case is obvious, but for writers it's also useful if you want to seek > > around in the file, and make sure that the position that you seek to already > > exists. (I'm not sure whether that use case is relevant in practical > > applications though.) -- Why would FIOQSIZE only be useful for readers? > > Good point! The use case you define for writing is interesting. However, > would it make sense to seek at a specific offset without being able to > know/read the content? I guest not in theory, but in practice we might > want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is > they only require to write (at a specific offset), or to deal with write > errors. Anyway, I guess that this information can be inferred by trying > to seek at a specific offset. The only limitation that this approach > would bring is that it seems that we can seek into an FD even without > read nor write right, and there is no specific (LSM) access control for > this operation (and nobody seems to care about being able to read the > size of a symlink once opened). If this is correct, I think we should > indeed always allow FIOQSIZE. Being able to open a file requires > LANDLOCK_ACCESS_FS_READ or WRITE anyway. It would be interesting to > check and test with O_PATH though. FIOQSIZE should in fact only be allowed if LANDLOCK_ACCESS_FS_READ_FILE or LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled and explicitly allowed for the FD. I guess FIOQSIZE is allowed without read nor write mode (i.e. O_PATH), so it could be an issue for landlocked applications but they can explicitly allow IOCTL for this case. When we'll have a LANDLOCK_ACCESS_FS_READ_METADATA (or something similar), we should also tie FIOQSIZE to this access right, and we'll be able to fully handle all the use cases without fully allowing all other IOCTLs. > > > > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security > > hook check for one of the "getting file attribute" hooks?) > > > > So basically, the implementation that I currently ended up with is: > > > > Before checking these commands, we first need to check that the original > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new > bit and replace the file's allowed_access field (see > landlock_add_fs_access_mask() and similar helpers in the network patch > series that does a similar thing but for ruleset's handle access > rights), but here is the idea: > > if (!landlock_file_handles_ioctl(file)) > return 0; > > > switch (cmd) { > /* > * Allows file descriptor and file description operations (see > * fcntl commands). > */ > > case FIOCLEX: > > case FIONCLEX: > > case FIONBIO: > > case FIOASYNC: > > > case FIOQSIZE: We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode checks. A kselftest test should check that ENOTTY is returned according to the file type and the access rights. > > return 0; > > case FIONREAD: > > if (file->f_mode & FMODE_READ) > > We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of > FMODE_READ. > > > return 0; > > } > > > > (with some comments in the source code, of course...) > > > > Does that look reasonable to you? > > > > —Günther > > > > -- > > Sent using Mutt
Hello! Thanks for the detailed review again! The FIONREAD implementation that you suggested works. With FIOQSIZE I ran into some surprises - I believe the check is a noop - more details below. On Sat, Aug 26, 2023 at 08:26:30PM +0200, Mickaël Salaün wrote: > On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote: > > On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote: > > > FIONREAD gives the number of bytes that are ready to read. This IOCTL seems > > > only useful when the file is open for reading. However, do you think that we > > > should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b) > > > f->f_mode & FMODE_READ? (The difference is that in case (a), FIONREAD will work > > > if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE > > > right for that file. In case (b), it would only work if you also opened the > > > file for reading.) > > > > I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled > > and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD. Just paraphrasing for later reference, because I almost misunderstood it: FIONREAD should work even when LANDLOCK_ACCESS_FS_IOCTL is *handled*, which is lingo for "listed in the ruleset_attr.handled_access_fs". When it is listed there, that means that the Landlock policy does not grant the LANDLOCK_ACCESS_FS_IOCTL right. So we treat FIONREAD as blanket-permitted independent of the LANDLOCK_ACCESS_FS_IOCTL right, under the condition that we have LANDLOCK_ACCESS_FS_READ_FILE for the file. -- Sounds good to me, will do. > > > FIOQSIZE seems like it would be useful for both reading *and* writing? -- The > > > reading case is obvious, but for writers it's also useful if you want to seek > > > around in the file, and make sure that the position that you seek to already > > > exists. (I'm not sure whether that use case is relevant in practical > > > applications though.) -- Why would FIOQSIZE only be useful for readers? > > > > Good point! The use case you define for writing is interesting. However, > > would it make sense to seek at a specific offset without being able to > > know/read the content? I guest not in theory, but in practice we might > > want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is > > they only require to write (at a specific offset), or to deal with write > > errors. Anyway, I guess that this information can be inferred by trying > > to seek at a specific offset. The only limitation that this approach > > would bring is that it seems that we can seek into an FD even without > > read nor write right, and there is no specific (LSM) access control for > > this operation (and nobody seems to care about being able to read the > > size of a symlink once opened). If this is correct, I think we should > > indeed always allow FIOQSIZE. Being able to open a file requires > > LANDLOCK_ACCESS_FS_READ or WRITE anyway. It would be interesting to > > check and test with O_PATH though. > > FIOQSIZE should in fact only be allowed if LANDLOCK_ACCESS_FS_READ_FILE or > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled > and explicitly allowed for the FD. I guess FIOQSIZE is allowed without read > nor write mode (i.e. O_PATH), so it could be an issue for landlocked > applications but they can explicitly allow IOCTL for this case. When > we'll have a LANDLOCK_ACCESS_FS_READ_METADATA (or something similar), we > should also tie FIOQSIZE to this access right, and we'll be able to > fully handle all the use cases without fully allowing all other IOCTLs. I implemented this check for the Landlock access rights in the ioctl hook, but when testing it I realized that I could not ever get it to fail in practice: ioctl(2) generally returns EBADF when the file was opened with O_PATH. Early in the ioctl(2) syscall implementation, it returns EBADF when the struct fd does not have the ->file attribute set. (This is even true for the commands to manipulate the Close-on-exec flag, which don't strictly need that. But they might work through fcntl.) In my understanding from the open(2) man page, the only ways to open files are with one of O_RDONLY, O_RDWR, O_WRONLY or O_PATH: - O_RDONLY: we had LANDLOCK_ACCESS_FS_READ_FILE at the time of open(2). - O_WRONLY: we had LANDLOCK_ACCESS_FS_WRITE_FILE at the time of open(2). - O_RDWR: we had both of these two rights at the time of open(2). - O_PATH: any ioctl(2) attempt returns EBADF early on So at the time that the ioctl security hook gets called, we already know that the user must have had one of the LANDLOCK_ACCESS_FS_READ_FILE or LANDLOCK_ACCESS_FS_WRITE_FILE rights -- checking for it again is not strictly necessary? Am I missing something here? (In particular, am I overlooking additional ways to call open(2) where the read and write rights are not necessary, other than O_PATH?) I'd propose this path forward: Let's keep the check for the rights as you suggested, but I would just keep it as an additional safety net there, for Landlock's internal consistency, and in case that future Linux versions introduce new ways to open files. I believe that at the moment, that check is equivalent to always permitting the FIOQSIZE command in that hook (with the same logic as for FIOCLEX, FIONCLEX etc). > > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security > > > hook check for one of the "getting file attribute" hooks?) > > > > > > So basically, the implementation that I currently ended up with is: > > > > > > > Before checking these commands, we first need to check that the original > > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new > > bit and replace the file's allowed_access field (see > > landlock_add_fs_access_mask() and similar helpers in the network patch > > series that does a similar thing but for ruleset's handle access > > rights), but here is the idea: > > > > if (!landlock_file_handles_ioctl(file)) > > return 0; > > > > > switch (cmd) { > > /* > > * Allows file descriptor and file description operations (see > > * fcntl commands). > > */ > > > case FIOCLEX: > > > case FIONCLEX: > > > case FIONBIO: > > > case FIOASYNC: > > > > > case FIOQSIZE: > > We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode > checks. A kselftest test should check that ENOTTY is returned according > to the file type and the access rights. It's not clear to me why we would need to add the same i_mode checks for S_ISDIR() || S_ISREG() || S_ISLNK() there? If these checks in do_vfs_ioctl() fail, it returns -ENOTTY. Is that not an appropriate error already? > > > return 0; > > > case FIONREAD: > > > if (file->f_mode & FMODE_READ) > > > > We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of > > FMODE_READ. Done. —Günther
On Sat, Sep 02, 2023 at 01:53:57PM +0200, Günther Noack wrote: > Hello! > > Thanks for the detailed review again! The FIONREAD implementation that you > suggested works. With FIOQSIZE I ran into some surprises - I believe the check > is a noop - more details below. > > On Sat, Aug 26, 2023 at 08:26:30PM +0200, Mickaël Salaün wrote: > > On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote: > > > On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote: > > > > FIONREAD gives the number of bytes that are ready to read. This IOCTL seems > > > > only useful when the file is open for reading. However, do you think that we > > > > should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b) > > > > f->f_mode & FMODE_READ? (The difference is that in case (a), FIONREAD will work > > > > if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE > > > > right for that file. In case (b), it would only work if you also opened the > > > > file for reading.) > > > > > > I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled > > > and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD. > > Just paraphrasing for later reference, because I almost misunderstood it: > > FIONREAD should work even when LANDLOCK_ACCESS_FS_IOCTL is *handled*, > which is lingo for "listed in the ruleset_attr.handled_access_fs". > When it is listed there, that means that the Landlock policy does not > grant the LANDLOCK_ACCESS_FS_IOCTL right. > > So we treat FIONREAD as blanket-permitted independent of the > LANDLOCK_ACCESS_FS_IOCTL right, under the condition that we have > LANDLOCK_ACCESS_FS_READ_FILE for the file. -- Sounds good to me, will do. We are almost on the same line but here is the explicit algorithm: if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) { allow FIONREAD } else { if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) { if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) { allow FIONREAD } else { deny FIONREAD } } else { if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) { allow FIONREAD } else { deny FIONREAD } } } The notation "FD's ruleset" refers to the Landlock domain that was being evaluated when the FD was being opened, not necessarely the current process's domain. The same logic should apply for all IOCTL delegations, and the tests should check this behavior. We may want to create a new helper to ease this IOCTL delegation and future ones. BTW, FIONREAD requested on another FS would call vfs_ioctl() twice. This should probably be fixed. Any though Christian? > > > > > > FIOQSIZE seems like it would be useful for both reading *and* writing? -- The > > > > reading case is obvious, but for writers it's also useful if you want to seek > > > > around in the file, and make sure that the position that you seek to already > > > > exists. (I'm not sure whether that use case is relevant in practical > > > > applications though.) -- Why would FIOQSIZE only be useful for readers? > > > > > > Good point! The use case you define for writing is interesting. However, > > > would it make sense to seek at a specific offset without being able to > > > know/read the content? I guest not in theory, but in practice we might > > > want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is > > > they only require to write (at a specific offset), or to deal with write > > > errors. Anyway, I guess that this information can be inferred by trying > > > to seek at a specific offset. The only limitation that this approach > > > would bring is that it seems that we can seek into an FD even without > > > read nor write right, and there is no specific (LSM) access control for > > > this operation (and nobody seems to care about being able to read the > > > size of a symlink once opened). If this is correct, I think we should > > > indeed always allow FIOQSIZE. Being able to open a file requires > > > LANDLOCK_ACCESS_FS_READ or WRITE anyway. It would be interesting to > > > check and test with O_PATH though. > > > > FIOQSIZE should in fact only be allowed if LANDLOCK_ACCESS_FS_READ_FILE or > > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled > > and explicitly allowed for the FD. I guess FIOQSIZE is allowed without read > > nor write mode (i.e. O_PATH), so it could be an issue for landlocked > > applications but they can explicitly allow IOCTL for this case. When > > we'll have a LANDLOCK_ACCESS_FS_READ_METADATA (or something similar), we > > should also tie FIOQSIZE to this access right, and we'll be able to > > fully handle all the use cases without fully allowing all other IOCTLs. > > I implemented this check for the Landlock access rights in the ioctl hook, but > when testing it I realized that I could not ever get it to fail in practice: > > ioctl(2) generally returns EBADF when the file was opened with O_PATH. Early in > the ioctl(2) syscall implementation, it returns EBADF when the struct fd does > not have the ->file attribute set. (This is even true for the commands to > manipulate the Close-on-exec flag, which don't strictly need that. But they > might work through fcntl.) Yes, this is expected, but I'd like to keep these tests to guarantee this behavior with all future kernel versions as well. > > In my understanding from the open(2) man page, the only ways to open files are > with one of O_RDONLY, O_RDWR, O_WRONLY or O_PATH: > > - O_RDONLY: we had LANDLOCK_ACCESS_FS_READ_FILE at the time of open(2). > - O_WRONLY: we had LANDLOCK_ACCESS_FS_WRITE_FILE at the time of open(2). > - O_RDWR: we had both of these two rights at the time of open(2). > - O_PATH: any ioctl(2) attempt returns EBADF early on > > So at the time that the ioctl security hook gets called, we already know that > the user must have had one of the LANDLOCK_ACCESS_FS_READ_FILE or > LANDLOCK_ACCESS_FS_WRITE_FILE rights -- checking for it again is not strictly > necessary? This is not the case if LANDLOCK_ACCESS_FS_{READ,WRITE}_FILE are not handled by the ruleset, see the subtle difference between not handled and explicitly allowed. Here is the corresponding explicit algorithm: if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) { allow FIOQSIZE } else { if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) { if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) { allow FIOQSIZE } else { if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) { if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) { allow FIOQSIZE } else { deny FIOQSIZE } } else { if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) { allow FIOQSIZE } else { deny FIOQSIZE } } } } else { if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) { if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) { allow FIOQSIZE } else { deny FIOQSIZE } } else { if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) { allow FIOQSIZE } else { deny FIOQSIZE } } } } > > Am I missing something here? (In particular, am I overlooking additional ways > to call open(2) where the read and write rights are not necessary, other than > O_PATH?) You're correct about the file mode and IOCTL returning EBADF for O_PATH, but you didn't take into account the fact that (for whatever reason) rulesets may not handle read/write file access rights. > > I'd propose this path forward: Let's keep the check for the rights as you > suggested, but I would just keep it as an additional safety net there, for > Landlock's internal consistency, and in case that future Linux versions > introduce new ways to open files. Yes, you're correct, this is the right approach. Even if the kernel doesn't need additional checks for now, we should still give guarantees/promises that we can keep (i.e. part of Landlock's code), and be consistent with Landlock's internals and code documentation. > I believe that at the moment, that check is > equivalent to always permitting the FIOQSIZE command in that hook (with the same > logic as for FIOCLEX, FIONCLEX etc). Not if LANDLOCK_ACCESS_FS_READ_FILE or LANDLOCK_ACCESS_FS_WRITE_FILE are not allowed. > > > > > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security > > > > hook check for one of the "getting file attribute" hooks?) > > > > > > > > So basically, the implementation that I currently ended up with is: > > > > > > > > > > Before checking these commands, we first need to check that the original > > > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new > > > bit and replace the file's allowed_access field (see > > > landlock_add_fs_access_mask() and similar helpers in the network patch > > > series that does a similar thing but for ruleset's handle access > > > rights), but here is the idea: > > > > > > if (!landlock_file_handles_ioctl(file)) > > > return 0; > > > > > > > switch (cmd) { > > > /* > > > * Allows file descriptor and file description operations (see > > > * fcntl commands). > > > */ > > > > case FIOCLEX: > > > > case FIONCLEX: > > > > case FIONBIO: > > > > case FIOASYNC: > > > > > > > case FIOQSIZE: > > > > We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode > > checks. A kselftest test should check that ENOTTY is returned according > > to the file type and the access rights. > > It's not clear to me why we would need to add the same i_mode checks for > S_ISDIR() || S_ISREG() || S_ISLNK() there? If these checks in do_vfs_ioctl() > fail, it returns -ENOTTY. Is that not an appropriate error already? The LSM IOCTL hook is called before do_vfs_ioctl(), and I think that Landlock should not change the error codes according to the error types (i.e. return ENOTTY when the inode is something else than a directory, regular file, or symlink). Indeed, I think it's valuable to not blindly return EACCES when the IOCTL doesn't make sense for this file type. This should help user space handle meaningful error messages, inconsistent requests, and fallback mechanisms. Tests should check these return codes, see the network patch series (and especially the latest reviews and changes) that takes care of this kind of error codes compatibility. We could also return 0 (i.e. accept the request) and rely on the do_vfs_ioctl() check to return ENOTTY, but this is unnecessary risky from an access control point of view. Let's directly return ENOTTY as a safeguard (which BTW also avoids some useless CPU instructions) and test this behavior. I think an access control mechanism, and especially Landlock, should be as discreet as possible, and help developers quickly debug issues (and avoid going through the access control layer if it doesn't make sense). I think error ordering like this could be useful but I'd like to get other point of views. > > > > > > return 0; > > > > case FIONREAD: > > > > if (file->f_mode & FMODE_READ) > > > > > > We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of > > > FMODE_READ. > > Done. We could also mimic the do_vfs_ioctl() checks according to the file being a regular file or not, but I think the FIONREAD semantic is well defined and delegating this command to underlying VFS implementations should not be an issue and be controlled the same way. It seems that only the pipe FS also implements this IOCTL and we should treat it the same way as for regular files. We should keep an eye on new implementations of this IOCTL though, but I guess the limit of our review stops at the FUSE boundary. We should also allow all IOCTLs implemented by pipefifo_fops. They are unrelated to the underlying filesystem (and then don't store data) but they can still be found on any of them, they can only have an impact on the related IPC (not directly system-wide like char/block devices), and these kind of files may be swapped from a FD unrelated to the filesystem to a named pipe according to application configuration (e.g. pipe redirection). IOCTLs on unix named socket should also be allowed, but anyway, they cannot be opened with open(2) (ENXIO is returned), so socket FDs should never get any restriction because of a path_beneath rule, so we can simply ignore this case (but still document and test it). Thinking more about the IOCTL control, I think we should help as much as possible developers to not require the LANDLOCK_ACCESS_FS_IOCTL right because that would mask their intent and it would grant a lot of potentially-undefined accesses. Delegating at least most VFS IOCTLs (i.e. standardized/generic IOCTLs) to regular Landlock access rights should then be encouraged. Even if new VFS IOCTLs should be scarce and new VFS syscalls should be encouraged instead, we will still be able to delegate future VFS IOCTLs to existing Landlock access rights according to their semantic. Taking the list of VFS IOCTLs you extracted, here is my updated point of view: > * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file > system. Requires CAP_SYS_ADMIN. These acts on an entire filesystem, not a file, and this is an administrative task, so we can rely on LANDLOCK_ACCESS_FS_IOCTL to control these ones for now. > * FICLONE, FICLONERANGE, FIDEDUPRANGE - making files share physical storage > between multiple files. These only work on some file systems, by design. For these IOCTLs, the kernel already check file permission with the remap_verify_area() and generic_file_rw_checks() calls. We should then follow the same logic. However, we should not check if the FD has the read/write Landlock access rights in the IOCTL hook (to only check once, and avoid TOCTOU), but only check if they are handled. The effective checks will be done by the VFS code, and we can then keep the current error ordering. FICLONE, FICLONERANGE should delegate to LANDLOCK_ACCESS_FS_WRITE_FILE. The other FD, extracted from IOCTL argument, is checked against the read permission, but this should only be part of a comment (in our IOCTL hook implementation). FIDEDUPRANGE should delegate to LANDLOCK_ACCESS_FS_READ_FILE > * Commands that read file system internals: > * FS_IOC_FIEMAP - get information about file extent mapping > (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt) For FS_IOCT_FIEMAP, there is no file permission check. This should be delegated to LANDLOCK_ACCESS_FS_READ_FILE or LANDLOCK_ACCESS_FS_WRITE_FILE for the same reasons as FIOQSIZE. I'm not sure if we'll have to care about FIEMAP_FLAG_XATTR, but that should not be an issue for now. > * FIBMAP - get a file's file system block number There is no file permission check for FIBMAP (only the process's capabilities). I think the Landlock checks should be the same as for FS_IOCT_FIEMAP. > * FIGETBSZ - get file system blocksize I guess this also enables to optimize file storage. It would probably make sense to delegate this one to LANDLOCK_ACCESS_FS_WRITE_FILE, or follow the FIOQSIZE logic? > * Accessing file attributes: > * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2)) These should be delegated to a future LANDLOCK_ACCESS_FS_READ_METADATA, so only LANDLOCK_ACCESS_FS_IOCTL for now. > * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes These should be delegated to a future LANDLOCK_ACCESS_FS_WRITE_METADATA, so only LANDLOCK_ACCESS_FS_IOCTL for now. > * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64, > FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS preallocation > syscalls which predate fallocate(2). ioctl_preallocate()-related IOCTLs should also be controlled like FIONREAD but according to LANDLOCK_ACCESS_FS_WRITE_FILE because there is in fact already a check with the vfs_fallocate/security_file_permission call. We should also check an IOCTL from an unrestricted special filesystems, e.g. NS_GET_NSTYPE. What do you think?
Hello! On Mon, Sep 04, 2023 at 08:08:29PM +0200, Micka�l Sala�n wrote: > On Sat, Sep 02, 2023 at 01:53:57PM +0200, G�nther Noack wrote: > > On Sat, Aug 26, 2023 at 08:26:30PM +0200, Micka�l Sala�n wrote: > > > On Fri, Aug 25, 2023 at 06:50:29PM +0200, Micka�l Sala�n wrote: > > > > On Fri, Aug 25, 2023 at 05:03:43PM +0200, G�nther Noack wrote: > > > > > FIONREAD gives the number of bytes that are ready to read. This IOCTL seems > > > > > only useful when the file is open for reading. However, do you think that we > > > > > should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b) > > > > > f->f_mode & FMODE_READ? (The difference is that in case (a), FIONREAD will work > > > > > if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE > > > > > right for that file. In case (b), it would only work if you also opened the > > > > > file for reading.) > > > > > > > > I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled > > > > and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD. > > > > Just paraphrasing for later reference, because I almost misunderstood it: > > > > FIONREAD should work even when LANDLOCK_ACCESS_FS_IOCTL is *handled*, > > which is lingo for "listed in the ruleset_attr.handled_access_fs". > > When it is listed there, that means that the Landlock policy does not > > grant the LANDLOCK_ACCESS_FS_IOCTL right. > > > > So we treat FIONREAD as blanket-permitted independent of the > > LANDLOCK_ACCESS_FS_IOCTL right, under the condition that we have > > LANDLOCK_ACCESS_FS_READ_FILE for the file. -- Sounds good to me, will do. > > We are almost on the same line but here is the explicit algorithm: > > if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) { > allow FIONREAD > } else { > if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) { > if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) { > allow FIONREAD > } else { > deny FIONREAD > } > } else { > if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) { > allow FIONREAD > } else { > deny FIONREAD > } > } > } > > The notation "FD's ruleset" refers to the Landlock domain that was being > evaluated when the FD was being opened, not necessarely the current > process's domain. > > The same logic should apply for all IOCTL delegations, and the tests > should check this behavior. We may want to create a new helper to ease > this IOCTL delegation and future ones. Thank you for making the algorithm that explicit -- that helps to trace down the differences. I can follow the logic now, but I still don't understand what your underlying rationale for that is? I believe that fundamentally, a core difference is: For an access right R and a file F, for these two cases: (a) the access right R is unhandled (nothing gets restricted) (b) the access right R is handled, but R is granted for F in a rule. I believe that accesses in case (a) and (b) to the file F should have the same results. This is at least how the existing Landlock implementation works, as far as I can tell. ("Refer" is an exceptional case, but we have documented that it was always "implicitly handled" in ABI V1, which makes it consistent again.) When I expand your code above to a boolean table, I end up with the following decisions, depending on whether IOCTL and READ are handled or not, and whether they are explicitly permitted for the file through a rule: Micka�l's IOCTL IOCTL IOCTL suggestion handled, handled, unhandled 2023-09-04 file file not permitted permitted -------------------------------------------------- READ handled, file permitted allow allow allow READ handled, f not permitted deny deny allow READ unhandled allow deny allow In patch set V3, this is different: Because I think that cases (a) and (b) from above should always behave the same, the first and third column and row must be symmetric and have the same entries. So, in patch set V3, it is sufficient if *one of* the two rights IOCTL and READ_FILE are present, in order to use the FIONREAD IOCTL: G�nther's IOCTL IOCTL IOCTL patch set V3 handled, handled, unhandled 2023-08-14 file file not permitted permitted -------------------------------------------------- READ handled, file permitted allow allow allow READ handled, f not permitted allow deny allow READ unhandled allow allow allow I have not spelled out the boolean logic tables for FIOQSIZE in the same way (they would also have three dimensions, strictly speaking), but I assume that the fundamental difference is the same -- you're also mentioning the "subtle difference between not handled and explicitly allowed" below. I honestly don't think that these cases should be different - it complicates both the contract with the callers and our internal implementation if we need to differentiate between three different configurations (handled + permitted, handled + not permitted, unhandled), rather than collapsing these into two (permitted, not permitted) as we currently do. I would like to encourage users of Landlock to "handle" as many rights as possible, and then poke holes into these restrictions for the files they need. But that is only an easy sell if we can actually guarantee that it behaves the same as if the right was unhandled. > BTW, FIONREAD requested on another FS would call vfs_ioctl() twice. This > should probably be fixed. Any though Christian? > > > > > > > > > > FIOQSIZE seems like it would be useful for both reading *and* writing? -- The > > > > > reading case is obvious, but for writers it's also useful if you want to seek > > > > > around in the file, and make sure that the position that you seek to already > > > > > exists. (I'm not sure whether that use case is relevant in practical > > > > > applications though.) -- Why would FIOQSIZE only be useful for readers? > > > > > > > > Good point! The use case you define for writing is interesting. However, > > > > would it make sense to seek at a specific offset without being able to > > > > know/read the content? I guest not in theory, but in practice we might > > > > want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is > > > > they only require to write (at a specific offset), or to deal with write > > > > errors. Anyway, I guess that this information can be inferred by trying > > > > to seek at a specific offset. The only limitation that this approach > > > > would bring is that it seems that we can seek into an FD even without > > > > read nor write right, and there is no specific (LSM) access control for > > > > this operation (and nobody seems to care about being able to read the > > > > size of a symlink once opened). If this is correct, I think we should > > > > indeed always allow FIOQSIZE. Being able to open a file requires > > > > LANDLOCK_ACCESS_FS_READ or WRITE anyway. It would be interesting to > > > > check and test with O_PATH though. > > > > > > FIOQSIZE should in fact only be allowed if LANDLOCK_ACCESS_FS_READ_FILE or > > > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled > > > and explicitly allowed for the FD. I guess FIOQSIZE is allowed without read > > > nor write mode (i.e. O_PATH), so it could be an issue for landlocked > > > applications but they can explicitly allow IOCTL for this case. When > > > we'll have a LANDLOCK_ACCESS_FS_READ_METADATA (or something similar), we > > > should also tie FIOQSIZE to this access right, and we'll be able to > > > fully handle all the use cases without fully allowing all other IOCTLs. > > > > I implemented this check for the Landlock access rights in the ioctl hook, but > > when testing it I realized that I could not ever get it to fail in practice: > > > > ioctl(2) generally returns EBADF when the file was opened with O_PATH. Early in > > the ioctl(2) syscall implementation, it returns EBADF when the struct fd does > > not have the ->file attribute set. (This is even true for the commands to > > manipulate the Close-on-exec flag, which don't strictly need that. But they > > might work through fcntl.) > > Yes, this is expected, but I'd like to keep these tests to guarantee > this behavior with all future kernel versions as well. > > > > > In my understanding from the open(2) man page, the only ways to open files are > > with one of O_RDONLY, O_RDWR, O_WRONLY or O_PATH: > > > > - O_RDONLY: we had LANDLOCK_ACCESS_FS_READ_FILE at the time of open(2). > > - O_WRONLY: we had LANDLOCK_ACCESS_FS_WRITE_FILE at the time of open(2). > > - O_RDWR: we had both of these two rights at the time of open(2). > > - O_PATH: any ioctl(2) attempt returns EBADF early on > > > > So at the time that the ioctl security hook gets called, we already know that > > the user must have had one of the LANDLOCK_ACCESS_FS_READ_FILE or > > LANDLOCK_ACCESS_FS_WRITE_FILE rights -- checking for it again is not strictly > > necessary? > > This is not the case if LANDLOCK_ACCESS_FS_{READ,WRITE}_FILE are not > handled by the ruleset, see the subtle difference between not handled > and explicitly allowed. Here is the corresponding explicit algorithm: > > if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) { > allow FIOQSIZE > } else { > if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) { > if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) { > allow FIOQSIZE > } else { > if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) { > if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) { > allow FIOQSIZE > } else { > deny FIOQSIZE > } > } else { > if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) { > allow FIOQSIZE > } else { > deny FIOQSIZE > } > } > } > } else { > if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) { > if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) { > allow FIOQSIZE > } else { > deny FIOQSIZE > } > } else { > if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) { > allow FIOQSIZE > } else { > deny FIOQSIZE > } > } > } > } > > > > > Am I missing something here? (In particular, am I overlooking additional ways > > to call open(2) where the read and write rights are not necessary, other than > > O_PATH?) > > You're correct about the file mode and IOCTL returning EBADF for O_PATH, > but you didn't take into account the fact that (for whatever reason) > rulesets may not handle read/write file access rights. > > > > > I'd propose this path forward: Let's keep the check for the rights as you > > suggested, but I would just keep it as an additional safety net there, for > > Landlock's internal consistency, and in case that future Linux versions > > introduce new ways to open files. > > Yes, you're correct, this is the right approach. Even if the kernel > doesn't need additional checks for now, we should still give > guarantees/promises that we can keep (i.e. part of Landlock's code), and > be consistent with Landlock's internals and code documentation. > > > I believe that at the moment, that check is > > equivalent to always permitting the FIOQSIZE command in that hook (with the same > > logic as for FIOCLEX, FIONCLEX etc). > > Not if LANDLOCK_ACCESS_FS_READ_FILE or LANDLOCK_ACCESS_FS_WRITE_FILE are > not allowed. > > > > > > > > > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security > > > > > hook check for one of the "getting file attribute" hooks?) > > > > > > > > > > So basically, the implementation that I currently ended up with is: > > > > > > > > > > > > > Before checking these commands, we first need to check that the original > > > > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new > > > > bit and replace the file's allowed_access field (see > > > > landlock_add_fs_access_mask() and similar helpers in the network patch > > > > series that does a similar thing but for ruleset's handle access > > > > rights), but here is the idea: > > > > > > > > if (!landlock_file_handles_ioctl(file)) > > > > return 0; > > > > > > > > > switch (cmd) { > > > > /* > > > > * Allows file descriptor and file description operations (see > > > > * fcntl commands). > > > > */ > > > > > case FIOCLEX: > > > > > case FIONCLEX: > > > > > case FIONBIO: > > > > > case FIOASYNC: > > > > > > > > > case FIOQSIZE: > > > > > > We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode > > > checks. A kselftest test should check that ENOTTY is returned according > > > to the file type and the access rights. > > > > It's not clear to me why we would need to add the same i_mode checks for > > S_ISDIR() || S_ISREG() || S_ISLNK() there? If these checks in do_vfs_ioctl() > > fail, it returns -ENOTTY. Is that not an appropriate error already? > > The LSM IOCTL hook is called before do_vfs_ioctl(), and I think that > Landlock should not change the error codes according to the error types > (i.e. return ENOTTY when the inode is something else than a directory, > regular file, or symlink). Indeed, I think it's valuable to not blindly > return EACCES when the IOCTL doesn't make sense for this file type. This > should help user space handle meaningful error messages, inconsistent > requests, and fallback mechanisms. Tests should check these return > codes, see the network patch series (and especially the latest reviews > and changes) that takes care of this kind of error codes compatibility. > > We could also return 0 (i.e. accept the request) and rely on the > do_vfs_ioctl() check to return ENOTTY, but this is unnecessary risky > from an access control point of view. Let's directly return ENOTTY as a > safeguard (which BTW also avoids some useless CPU instructions) and test > this behavior. > > I think an access control mechanism, and especially Landlock, should be > as discreet as possible, and help developers quickly debug issues (and > avoid going through the access control layer if it doesn't make sense). > I think error ordering like this could be useful but I'd like to get > other point of views. I see what you mean now, OK. Another option, btw, would be to return ENOTTY generally when Landlock denies an IOCTL attempt, instead of EACCES, as I have previously suggested in https://lore.kernel.org/all/ZNpnrCjYqFoGkwyf@google.com/ -- should we maybe just do that instead? I believe that users of ioctl(2) should be better equipped to deal with ENOTTY, because that is an error that ioctl(2) can return in general, whereas EACCES can only be returned if one of the specific subcommands returns it. According to the man page, ENOTTY is the error that ioctl(2) returns if the given "request does not apply to the kind of object that the file descriptor fd references". That is not 100% what is happening here, but from the errors listed in ioctl(2), this seems to be the closest, semantically. What do you think? > > > > > > > > > > return 0; > > > > > case FIONREAD: > > > > > if (file->f_mode & FMODE_READ) > > > > > > > > We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of > > > > FMODE_READ. > > > > Done. > > We could also mimic the do_vfs_ioctl() checks according to the file > being a regular file or not, but I think the FIONREAD semantic is well > defined and delegating this command to underlying VFS implementations > should not be an issue and be controlled the same way. It seems that > only the pipe FS also implements this IOCTL and we should treat it the > same way as for regular files. We should keep an eye on new > implementations of this IOCTL though, but I guess the limit of our > review stops at the FUSE boundary. > > We should also allow all IOCTLs implemented by pipefifo_fops. They are > unrelated to the underlying filesystem (and then don't store data) but > they can still be found on any of them, they can only have an impact on > the related IPC (not directly system-wide like char/block devices), and > these kind of files may be swapped from a FD unrelated to the filesystem > to a named pipe according to application configuration (e.g. pipe > redirection). IOCTLs on unix named socket should also be allowed, but > anyway, they cannot be opened with open(2) (ENXIO is returned), so > socket FDs should never get any restriction because of a path_beneath > rule, so we can simply ignore this case (but still document and test > it). Thanks, that's a good observation! Adding named pipes and named unix sockets to the TODO list for the next iteration. I had not thought of these. > Thinking more about the IOCTL control, I think we should help as much as > possible developers to not require the LANDLOCK_ACCESS_FS_IOCTL right > because that would mask their intent and it would grant a lot of > potentially-undefined accesses. Delegating at least most VFS IOCTLs > (i.e. standardized/generic IOCTLs) to regular Landlock access rights > should then be encouraged. > > Even if new VFS IOCTLs should be scarce and new VFS syscalls should be > encouraged instead, we will still be able to delegate future VFS IOCTLs > to existing Landlock access rights according to their semantic. OK, the approach makes sense. I'll respond on these individual items as I get around to implement them when we have clarified the unhandled/handled+permitted question above. > Taking the list of VFS IOCTLs you extracted, here is my updated point of > view: > > * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file > > system. Requires CAP_SYS_ADMIN. > > These acts on an entire filesystem, not a file, and this is an > administrative task, so we can rely on LANDLOCK_ACCESS_FS_IOCTL to > control these ones for now. > > > * FICLONE, FICLONERANGE, FIDEDUPRANGE - making files share physical storage > > between multiple files. These only work on some file systems, by design. > > For these IOCTLs, the kernel already check file permission with the > remap_verify_area() and generic_file_rw_checks() calls. We should then > follow the same logic. However, we should not check if the FD has the > read/write Landlock access rights in the IOCTL hook (to only check once, > and avoid TOCTOU), but only check if they are handled. The effective > checks will be done by the VFS code, and we can then keep the current > error ordering. > > FICLONE, FICLONERANGE should delegate to LANDLOCK_ACCESS_FS_WRITE_FILE. > The other FD, extracted from IOCTL argument, is checked against the > read permission, but this should only be part of a comment (in our IOCTL > hook implementation). > > FIDEDUPRANGE should delegate to LANDLOCK_ACCESS_FS_READ_FILE > > > * Commands that read file system internals: > > * FS_IOC_FIEMAP - get information about file extent mapping > > (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt) > > For FS_IOCT_FIEMAP, there is no file permission check. This should be > delegated to LANDLOCK_ACCESS_FS_READ_FILE or > LANDLOCK_ACCESS_FS_WRITE_FILE for the same reasons as FIOQSIZE. > > I'm not sure if we'll have to care about FIEMAP_FLAG_XATTR, but that > should not be an issue for now. > > > * FIBMAP - get a file's file system block number > > There is no file permission check for FIBMAP (only the process's > capabilities). I think the Landlock checks should be the same as for > FS_IOCT_FIEMAP. > > > * FIGETBSZ - get file system blocksize > > I guess this also enables to optimize file storage. It would probably > make sense to delegate this one to LANDLOCK_ACCESS_FS_WRITE_FILE, or > follow the FIOQSIZE logic? > > > * Accessing file attributes: > > * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2)) > > These should be delegated to a future LANDLOCK_ACCESS_FS_READ_METADATA, > so only LANDLOCK_ACCESS_FS_IOCTL for now. > > > * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes > > These should be delegated to a future LANDLOCK_ACCESS_FS_WRITE_METADATA, > so only LANDLOCK_ACCESS_FS_IOCTL for now. > > > * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64, > > FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS preallocation > > syscalls which predate fallocate(2). > > ioctl_preallocate()-related IOCTLs should also be controlled like > FIONREAD but according to LANDLOCK_ACCESS_FS_WRITE_FILE because there is > in fact already a check with the vfs_fallocate/security_file_permission > call. > > We should also check an IOCTL from an unrestricted special filesystems, > e.g. NS_GET_NSTYPE. > > What do you think?
On Mon, Sep 11, 2023 at 12:02:46PM +0200, Günther Noack wrote: > Hello! > > On Mon, Sep 04, 2023 at 08:08:29PM +0200, Mickaël Salaün wrote: > > On Sat, Sep 02, 2023 at 01:53:57PM +0200, Günther Noack wrote: > > > On Sat, Aug 26, 2023 at 08:26:30PM +0200, Mickaël Salaün wrote: > > > > On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote: > > > > > On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote: > > > > > > FIONREAD gives the number of bytes that are ready to read. This IOCTL seems > > > > > > only useful when the file is open for reading. However, do you think that we > > > > > > should correlate this with (a) LANDLOCK_ACCESS_FS_READ_FILE, or with (b) > > > > > > f->f_mode & FMODE_READ? (The difference is that in case (a), FIONREAD will work > > > > > > if you open a file O_WRONLY and you also have the LANDLOCK_ACCESS_FS_READ_FILE > > > > > > right for that file. In case (b), it would only work if you also opened the > > > > > > file for reading.) > > > > > > > > > > I think we should allow FIONREAD if LANDLOCK_ACCESS_FS_IOCTL is handled > > > > > and if LANDLOCK_ACCESS_FS_READ_FILE is explicitly allowed for this FD. > > > > > > Just paraphrasing for later reference, because I almost misunderstood it: > > > > > > FIONREAD should work even when LANDLOCK_ACCESS_FS_IOCTL is *handled*, > > > which is lingo for "listed in the ruleset_attr.handled_access_fs". > > > When it is listed there, that means that the Landlock policy does not > > > grant the LANDLOCK_ACCESS_FS_IOCTL right. > > > > > > So we treat FIONREAD as blanket-permitted independent of the > > > LANDLOCK_ACCESS_FS_IOCTL right, under the condition that we have > > > LANDLOCK_ACCESS_FS_READ_FILE for the file. -- Sounds good to me, will do. > > > > We are almost on the same line but here is the explicit algorithm: > > > > if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) { > > allow FIONREAD > > } else { > > if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) { > > if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) { > > allow FIONREAD > > } else { > > deny FIONREAD > > } > > } else { > > if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) { > > allow FIONREAD > > } else { > > deny FIONREAD > > } > > } > > } > > > > The notation "FD's ruleset" refers to the Landlock domain that was being > > evaluated when the FD was being opened, not necessarely the current > > process's domain. > > > > The same logic should apply for all IOCTL delegations, and the tests > > should check this behavior. We may want to create a new helper to ease > > this IOCTL delegation and future ones. > > Thank you for making the algorithm that explicit -- that helps to trace down the > differences. I can follow the logic now, but I still don't understand what your > underlying rationale for that is? > > I believe that fundamentally, a core difference is: > > For an access right R and a file F, for these two cases: > > (a) the access right R is unhandled (nothing gets restricted) > (b) the access right R is handled, but R is granted for F in a rule. > > I believe that accesses in case (a) and (b) to the file F should have the same > results. > > This is at least how the existing Landlock implementation works, as far as I can > tell. > > ("Refer" is an exceptional case, but we have documented that it was always > "implicitly handled" in ABI V1, which makes it consistent again.) > > > When I expand your code above to a boolean table, I end up with the following > decisions, depending on whether IOCTL and READ are handled or not, and whether > they are explicitly permitted for the file through a rule: > > > Mickaël's IOCTL IOCTL IOCTL > suggestion handled, handled, unhandled > 2023-09-04 file file not > permitted permitted > -------------------------------------------------- > READ handled, > file permitted allow allow allow > > READ handled, > f not permitted deny deny allow > > READ unhandled allow deny allow > > > In patch set V3, this is different: Because I think that cases (a) and (b) from > above should always behave the same, the first and third column and row must be > symmetric and have the same entries. So, in patch set V3, it is sufficient if > *one of* the two rights IOCTL and READ_FILE are present, in order to use the > FIONREAD IOCTL: > > > Günther's IOCTL IOCTL IOCTL > patch set V3 handled, handled, unhandled > 2023-08-14 file file not > permitted permitted > -------------------------------------------------- > READ handled, > file permitted allow allow allow > > READ handled, > f not permitted allow deny allow > > READ unhandled allow allow allow A first difference is about (READ unhandled) AND (IOCTL handled + file not permitted). It will not be possible to follow the same logic with new Landlock access right (e.g. LANDLOCK_ACCESS_FS_READ_METADATA that should also allow FS_IOC_FSGETXATTR), and I'd like to keep it consistent. A second difference is about (READ handled + f not permitted) AND (IOCTL handled + file permitted). The reasoning was to avoid giving too much power to LANDLOCK_ACCESS_FS_IOCTL and dowgrade it as new access rights are implemented. This looks quite similar to the CAP_SYS_ADMIN right that can basically do anything, and new capabilites are mainly a subset of this one. My proposal was to incrementally downgrade the power given by LANDLOCK_ACCESS_FS_IOCTL while still being compatible. On the I was thinking that, if we make a requirement to have the "new correct" access right, the application update might drop the IOCTL access right. I now think this reasoning is flawed. Indeed, this comparaison doesn't fit well because IOCTLs are not a superset of all access rights, and because nothing can force developers that already gave access to all IOCTLs for a file to not just add another access right (instead of swapping them). Instead, I think user space libraries should manage this kind of access right swapping when possible and have a fallback mechanism relying on the LANDLOCK_ACCESS_FS_IOCTL right. This would be valuable because they may be updated before the (stable system) kernel, and this would be easier for developers to manage. In a nutshell, it is about giving control for an action (e.g. FIONREAD) to either a unique access right or to a set of access rights. At first, I would have preferred to have a unique access right to control an action, because it is simpler (e.g. for audit/debug). On the other hand, we need to handle access rights that allow the same action (e.g. file read OR write for FIOQSIZE). I now think your approach (i.e. set of access rights to control an action) could make more sense. Another good point is to not downgrade the power of LANDLOCK_ACCESS_FS_IOCTL, which could in fact be difficult to understand for users. Nested Landlock domains should also be easier to manage with this logic. > > > I have not spelled out the boolean logic tables for FIOQSIZE in the same way > (they would also have three dimensions, strictly speaking), but I assume that > the fundamental difference is the same -- you're also mentioning the "subtle > difference between not handled and explicitly allowed" below. > > I honestly don't think that these cases should be different - it complicates > both the contract with the callers and our internal implementation if we need to > differentiate between three different configurations (handled + permitted, > handled + not permitted, unhandled), rather than collapsing these into two > (permitted, not permitted) as we currently do. > > I would like to encourage users of Landlock to "handle" as many rights as > possible, and then poke holes into these restrictions for the files they need. > But that is only an easy sell if we can actually guarantee that it behaves the > same as if the right was unhandled. I agree with that, but there are compatibility limitations, hence the first difference. This case should be rare though, i.e. app not supporting an existing access right but still willing to support a new one (IOCTL). > > > > BTW, FIONREAD requested on another FS would call vfs_ioctl() twice. This > > should probably be fixed. Any though Christian? > > > > > > > > > > > > > > FIOQSIZE seems like it would be useful for both reading *and* writing? -- The > > > > > > reading case is obvious, but for writers it's also useful if you want to seek > > > > > > around in the file, and make sure that the position that you seek to already > > > > > > exists. (I'm not sure whether that use case is relevant in practical > > > > > > applications though.) -- Why would FIOQSIZE only be useful for readers? > > > > > > > > > > Good point! The use case you define for writing is interesting. However, > > > > > would it make sense to seek at a specific offset without being able to > > > > > know/read the content? I guest not in theory, but in practice we might > > > > > want to avoid application to require LANDLOCK_ACCESS_FS_READ_FILE is > > > > > they only require to write (at a specific offset), or to deal with write > > > > > errors. Anyway, I guess that this information can be inferred by trying > > > > > to seek at a specific offset. The only limitation that this approach > > > > > would bring is that it seems that we can seek into an FD even without > > > > > read nor write right, and there is no specific (LSM) access control for > > > > > this operation (and nobody seems to care about being able to read the > > > > > size of a symlink once opened). If this is correct, I think we should > > > > > indeed always allow FIOQSIZE. Being able to open a file requires > > > > > LANDLOCK_ACCESS_FS_READ or WRITE anyway. It would be interesting to > > > > > check and test with O_PATH though. > > > > > > > > FIOQSIZE should in fact only be allowed if LANDLOCK_ACCESS_FS_READ_FILE or > > > > LANDLOCK_ACCESS_FS_WRITE_FILE or LANDLOCK_ACCESS_FS_READ_DIR are handled > > > > and explicitly allowed for the FD. I guess FIOQSIZE is allowed without read > > > > nor write mode (i.e. O_PATH), so it could be an issue for landlocked > > > > applications but they can explicitly allow IOCTL for this case. When > > > > we'll have a LANDLOCK_ACCESS_FS_READ_METADATA (or something similar), we > > > > should also tie FIOQSIZE to this access right, and we'll be able to > > > > fully handle all the use cases without fully allowing all other IOCTLs. > > > > > > I implemented this check for the Landlock access rights in the ioctl hook, but > > > when testing it I realized that I could not ever get it to fail in practice: > > > > > > ioctl(2) generally returns EBADF when the file was opened with O_PATH. Early in > > > the ioctl(2) syscall implementation, it returns EBADF when the struct fd does > > > not have the ->file attribute set. (This is even true for the commands to > > > manipulate the Close-on-exec flag, which don't strictly need that. But they > > > might work through fcntl.) > > > > Yes, this is expected, but I'd like to keep these tests to guarantee > > this behavior with all future kernel versions as well. > > > > > > > > In my understanding from the open(2) man page, the only ways to open files are > > > with one of O_RDONLY, O_RDWR, O_WRONLY or O_PATH: > > > > > > - O_RDONLY: we had LANDLOCK_ACCESS_FS_READ_FILE at the time of open(2). > > > - O_WRONLY: we had LANDLOCK_ACCESS_FS_WRITE_FILE at the time of open(2). > > > - O_RDWR: we had both of these two rights at the time of open(2). > > > - O_PATH: any ioctl(2) attempt returns EBADF early on > > > > > > So at the time that the ioctl security hook gets called, we already know that > > > the user must have had one of the LANDLOCK_ACCESS_FS_READ_FILE or > > > LANDLOCK_ACCESS_FS_WRITE_FILE rights -- checking for it again is not strictly > > > necessary? > > > > This is not the case if LANDLOCK_ACCESS_FS_{READ,WRITE}_FILE are not > > handled by the ruleset, see the subtle difference between not handled > > and explicitly allowed. Here is the corresponding explicit algorithm: > > > > if (LANDLOCK_ACCESS_FS_IOCTL is not handled by the FD's ruleset) { > > allow FIOQSIZE > > } else { > > if (LANDLOCK_ACCESS_FS_READ_FILE is handled by the FD's ruleset) { > > if (LANDLOCK_ACCESS_FS_READ_FILE is allowed for the FD) { > > allow FIOQSIZE > > } else { > > if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) { > > if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) { > > allow FIOQSIZE > > } else { > > deny FIOQSIZE > > } > > } else { > > if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) { > > allow FIOQSIZE > > } else { > > deny FIOQSIZE > > } > > } > > } > > } else { > > if (LANDLOCK_ACCESS_FS_WRITE_FILE is handled by the FD's ruleset) { > > if (LANDLOCK_ACCESS_FS_WRITE_FILE is allowed for the FD) { > > allow FIOQSIZE > > } else { > > deny FIOQSIZE > > } > > } else { > > if (LANDLOCK_ACCESS_FS_IOCTL is allowed for FD) { > > allow FIOQSIZE > > } else { > > deny FIOQSIZE > > } > > } > > } > > } > > > > > > > > Am I missing something here? (In particular, am I overlooking additional ways > > > to call open(2) where the read and write rights are not necessary, other than > > > O_PATH?) > > > > You're correct about the file mode and IOCTL returning EBADF for O_PATH, > > but you didn't take into account the fact that (for whatever reason) > > rulesets may not handle read/write file access rights. > > > > > > > > I'd propose this path forward: Let's keep the check for the rights as you > > > suggested, but I would just keep it as an additional safety net there, for > > > Landlock's internal consistency, and in case that future Linux versions > > > introduce new ways to open files. > > > > Yes, you're correct, this is the right approach. Even if the kernel > > doesn't need additional checks for now, we should still give > > guarantees/promises that we can keep (i.e. part of Landlock's code), and > > be consistent with Landlock's internals and code documentation. > > > > > I believe that at the moment, that check is > > > equivalent to always permitting the FIOQSIZE command in that hook (with the same > > > logic as for FIOCLEX, FIONCLEX etc). > > > > Not if LANDLOCK_ACCESS_FS_READ_FILE or LANDLOCK_ACCESS_FS_WRITE_FILE are > > not allowed. > > > > > > > > > > > > > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security > > > > > > hook check for one of the "getting file attribute" hooks?) > > > > > > > > > > > > So basically, the implementation that I currently ended up with is: > > > > > > > > > > > > > > > > Before checking these commands, we first need to check that the original > > > > > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new > > > > > bit and replace the file's allowed_access field (see > > > > > landlock_add_fs_access_mask() and similar helpers in the network patch > > > > > series that does a similar thing but for ruleset's handle access > > > > > rights), but here is the idea: > > > > > > > > > > if (!landlock_file_handles_ioctl(file)) > > > > > return 0; > > > > > > > > > > > switch (cmd) { > > > > > /* > > > > > * Allows file descriptor and file description operations (see > > > > > * fcntl commands). > > > > > */ > > > > > > case FIOCLEX: > > > > > > case FIONCLEX: > > > > > > case FIONBIO: > > > > > > case FIOASYNC: > > > > > > > > > > > case FIOQSIZE: > > > > > > > > We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode > > > > checks. A kselftest test should check that ENOTTY is returned according > > > > to the file type and the access rights. > > > > > > It's not clear to me why we would need to add the same i_mode checks for > > > S_ISDIR() || S_ISREG() || S_ISLNK() there? If these checks in do_vfs_ioctl() > > > fail, it returns -ENOTTY. Is that not an appropriate error already? > > > > The LSM IOCTL hook is called before do_vfs_ioctl(), and I think that > > Landlock should not change the error codes according to the error types > > (i.e. return ENOTTY when the inode is something else than a directory, > > regular file, or symlink). Indeed, I think it's valuable to not blindly > > return EACCES when the IOCTL doesn't make sense for this file type. This > > should help user space handle meaningful error messages, inconsistent > > requests, and fallback mechanisms. Tests should check these return > > codes, see the network patch series (and especially the latest reviews > > and changes) that takes care of this kind of error codes compatibility. > > > > We could also return 0 (i.e. accept the request) and rely on the > > do_vfs_ioctl() check to return ENOTTY, but this is unnecessary risky > > from an access control point of view. Let's directly return ENOTTY as a > > safeguard (which BTW also avoids some useless CPU instructions) and test > > this behavior. > > > > I think an access control mechanism, and especially Landlock, should be > > as discreet as possible, and help developers quickly debug issues (and > > avoid going through the access control layer if it doesn't make sense). > > I think error ordering like this could be useful but I'd like to get > > other point of views. > > I see what you mean now, OK. > > Another option, btw, would be to return ENOTTY generally when Landlock denies an > IOCTL attempt, instead of EACCES, as I have previously suggested in > https://lore.kernel.org/all/ZNpnrCjYqFoGkwyf@google.com/ -- should we maybe just > do that instead? > > I believe that users of ioctl(2) should be better equipped to deal with ENOTTY, > because that is an error that ioctl(2) can return in general, whereas EACCES can > only be returned if one of the specific subcommands returns it. > > According to the man page, ENOTTY is the error that ioctl(2) returns if the > given "request does not apply to the kind of object that the file descriptor fd > references". > > That is not 100% what is happening here, but from the errors listed in ioctl(2), > this seems to be the closest, semantically. > > What do you think? ENOTTY has a (kinda) well-defined semantic, which should not depend on an access control. Other LSMs already return EACCES for denied IOCTLs, so the man page should be updated with this information instead. ;) > > > > > > > > > > > > > > > return 0; > > > > > > case FIONREAD: > > > > > > if (file->f_mode & FMODE_READ) > > > > > > > > > > We should check LANDLOCK_ACCESS_FS_READ instead, which is a superset of > > > > > FMODE_READ. > > > > > > Done. > > > > We could also mimic the do_vfs_ioctl() checks according to the file > > being a regular file or not, but I think the FIONREAD semantic is well > > defined and delegating this command to underlying VFS implementations > > should not be an issue and be controlled the same way. It seems that > > only the pipe FS also implements this IOCTL and we should treat it the > > same way as for regular files. We should keep an eye on new > > implementations of this IOCTL though, but I guess the limit of our > > review stops at the FUSE boundary. > > > > We should also allow all IOCTLs implemented by pipefifo_fops. They are > > unrelated to the underlying filesystem (and then don't store data) but > > they can still be found on any of them, they can only have an impact on > > the related IPC (not directly system-wide like char/block devices), and > > these kind of files may be swapped from a FD unrelated to the filesystem > > to a named pipe according to application configuration (e.g. pipe > > redirection). IOCTLs on unix named socket should also be allowed, but > > anyway, they cannot be opened with open(2) (ENXIO is returned), so > > socket FDs should never get any restriction because of a path_beneath > > rule, so we can simply ignore this case (but still document and test > > it). > > Thanks, that's a good observation! Adding named pipes and named unix sockets to > the TODO list for the next iteration. I had not thought of these. > > > > Thinking more about the IOCTL control, I think we should help as much as > > possible developers to not require the LANDLOCK_ACCESS_FS_IOCTL right > > because that would mask their intent and it would grant a lot of > > potentially-undefined accesses. Delegating at least most VFS IOCTLs > > (i.e. standardized/generic IOCTLs) to regular Landlock access rights > > should then be encouraged. > > > > Even if new VFS IOCTLs should be scarce and new VFS syscalls should be > > encouraged instead, we will still be able to delegate future VFS IOCTLs > > to existing Landlock access rights according to their semantic. > > OK, the approach makes sense. I'll respond on these individual items as I get > around to implement them when we have clarified the unhandled/handled+permitted > question above. > > > > Taking the list of VFS IOCTLs you extracted, here is my updated point of > > view: > > > * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file > > > system. Requires CAP_SYS_ADMIN. > > > > These acts on an entire filesystem, not a file, and this is an > > administrative task, so we can rely on LANDLOCK_ACCESS_FS_IOCTL to > > control these ones for now. > > > > > * FICLONE, FICLONERANGE, FIDEDUPRANGE - making files share physical storage > > > between multiple files. These only work on some file systems, by design. > > > > For these IOCTLs, the kernel already check file permission with the > > remap_verify_area() and generic_file_rw_checks() calls. We should then > > follow the same logic. However, we should not check if the FD has the > > read/write Landlock access rights in the IOCTL hook (to only check once, > > and avoid TOCTOU), but only check if they are handled. The effective > > checks will be done by the VFS code, and we can then keep the current > > error ordering. > > > > FICLONE, FICLONERANGE should delegate to LANDLOCK_ACCESS_FS_WRITE_FILE. > > The other FD, extracted from IOCTL argument, is checked against the > > read permission, but this should only be part of a comment (in our IOCTL > > hook implementation). > > > > FIDEDUPRANGE should delegate to LANDLOCK_ACCESS_FS_READ_FILE > > > > > * Commands that read file system internals: > > > * FS_IOC_FIEMAP - get information about file extent mapping > > > (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt) > > > > For FS_IOCT_FIEMAP, there is no file permission check. This should be > > delegated to LANDLOCK_ACCESS_FS_READ_FILE or > > LANDLOCK_ACCESS_FS_WRITE_FILE for the same reasons as FIOQSIZE. > > > > I'm not sure if we'll have to care about FIEMAP_FLAG_XATTR, but that > > should not be an issue for now. > > > > > * FIBMAP - get a file's file system block number > > > > There is no file permission check for FIBMAP (only the process's > > capabilities). I think the Landlock checks should be the same as for > > FS_IOCT_FIEMAP. > > > > > * FIGETBSZ - get file system blocksize > > > > I guess this also enables to optimize file storage. It would probably > > make sense to delegate this one to LANDLOCK_ACCESS_FS_WRITE_FILE, or > > follow the FIOQSIZE logic? > > > > > * Accessing file attributes: > > > * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2)) > > > > These should be delegated to a future LANDLOCK_ACCESS_FS_READ_METADATA, > > so only LANDLOCK_ACCESS_FS_IOCTL for now. > > > > > * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes > > > > These should be delegated to a future LANDLOCK_ACCESS_FS_WRITE_METADATA, > > so only LANDLOCK_ACCESS_FS_IOCTL for now. > > > > > * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64, > > > FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS preallocation > > > syscalls which predate fallocate(2). > > > > ioctl_preallocate()-related IOCTLs should also be controlled like > > FIONREAD but according to LANDLOCK_ACCESS_FS_WRITE_FILE because there is > > in fact already a check with the vfs_fallocate/security_file_permission > > call. > > > > We should also check an IOCTL from an unrestricted special filesystems, > > e.g. NS_GET_NSTYPE. > > > > What do you think?
On Mon, Sep 11, 2023 at 05:25:34PM +0200, Mickaël Salaün wrote: > On Mon, Sep 11, 2023 at 12:02:46PM +0200, Günther Noack wrote: > > Hello! > > > > On Mon, Sep 04, 2023 at 08:08:29PM +0200, Mickaël Salaün wrote: > > > On Sat, Sep 02, 2023 at 01:53:57PM +0200, Günther Noack wrote: > > > > On Sat, Aug 26, 2023 at 08:26:30PM +0200, Mickaël Salaün wrote: > > > > > On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote: > > > > > > On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote: > > > > > > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security > > > > > > > hook check for one of the "getting file attribute" hooks?) > > > > > > > > > > > > > > So basically, the implementation that I currently ended up with is: > > > > > > > > > > > > > > > > > > > Before checking these commands, we first need to check that the original > > > > > > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new > > > > > > bit and replace the file's allowed_access field (see > > > > > > landlock_add_fs_access_mask() and similar helpers in the network patch > > > > > > series that does a similar thing but for ruleset's handle access > > > > > > rights), but here is the idea: > > > > > > > > > > > > if (!landlock_file_handles_ioctl(file)) > > > > > > return 0; > > > > > > > > > > > > > switch (cmd) { > > > > > > /* > > > > > > * Allows file descriptor and file description operations (see > > > > > > * fcntl commands). > > > > > > */ > > > > > > > case FIOCLEX: > > > > > > > case FIONCLEX: > > > > > > > case FIONBIO: > > > > > > > case FIOASYNC: > > > > > > > > > > > > > case FIOQSIZE: > > > > > > > > > > We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode > > > > > checks. A kselftest test should check that ENOTTY is returned according > > > > > to the file type and the access rights. > > > > > > > > It's not clear to me why we would need to add the same i_mode checks for > > > > S_ISDIR() || S_ISREG() || S_ISLNK() there? If these checks in do_vfs_ioctl() > > > > fail, it returns -ENOTTY. Is that not an appropriate error already? > > > > > > The LSM IOCTL hook is called before do_vfs_ioctl(), and I think that > > > Landlock should not change the error codes according to the error types > > > (i.e. return ENOTTY when the inode is something else than a directory, > > > regular file, or symlink). Indeed, I think it's valuable to not blindly > > > return EACCES when the IOCTL doesn't make sense for this file type. This > > > should help user space handle meaningful error messages, inconsistent > > > requests, and fallback mechanisms. Tests should check these return > > > codes, see the network patch series (and especially the latest reviews > > > and changes) that takes care of this kind of error codes compatibility. > > > > > > We could also return 0 (i.e. accept the request) and rely on the > > > do_vfs_ioctl() check to return ENOTTY, but this is unnecessary risky > > > from an access control point of view. Let's directly return ENOTTY as a > > > safeguard (which BTW also avoids some useless CPU instructions) and test > > > this behavior. > > > > > > I think an access control mechanism, and especially Landlock, should be > > > as discreet as possible, and help developers quickly debug issues (and > > > avoid going through the access control layer if it doesn't make sense). > > > I think error ordering like this could be useful but I'd like to get > > > other point of views. > > > > I see what you mean now, OK. > > > > Another option, btw, would be to return ENOTTY generally when Landlock denies an > > IOCTL attempt, instead of EACCES, as I have previously suggested in > > https://lore.kernel.org/all/ZNpnrCjYqFoGkwyf@google.com/ -- should we maybe just > > do that instead? > > > > I believe that users of ioctl(2) should be better equipped to deal with ENOTTY, > > because that is an error that ioctl(2) can return in general, whereas EACCES can > > only be returned if one of the specific subcommands returns it. > > > > According to the man page, ENOTTY is the error that ioctl(2) returns if the > > given "request does not apply to the kind of object that the file descriptor fd > > references". > > > > That is not 100% what is happening here, but from the errors listed in ioctl(2), > > this seems to be the closest, semantically. > > > > What do you think? > > ENOTTY has a (kinda) well-defined semantic, which should not depend on > an access control. Other LSMs already return EACCES for denied IOCTLs, > so the man page should be updated with this information instead. ;) Well, thinking about this again, for the sake of consistency with other IOCTLs, we should not specifically handle IOCTL error codes, but instead return either 0 or -EACCES. The network error cases are special because the LSM is called before some user-provided data are interpreted by the network stack, and in this case we need to interpret these data ourself. But in the case of IOCTLs, we may only need to handle the cases where an IOCTL command may be interpreted by different implementations (e.g. VFS or FS implementation), but even that is not a good idea, see below. For FIOQSIZE, I don't think anymore that we should try to mimic the do_vfs_ioctl() implementation. In fact, this approach could end up confusing developers and leaking metadata (see FIGETBSZ). Even with FIONREAD, the FS (i.e. vfs_ioctl() call) should follow the same semantic as the VFS (i.e. do_vfs_ioctl() code), so we should treat them the same and keep it simple: either return 0 or -EACCES. About the file_ioctl() IOCTLs, we should check that there are no overlapping with other IOCTLs (with different name). I think we should trust the FS implementation to implement the same semantic, but much less the device drivers. The main issue is with VFS and FS code returning -ENOIOCTLCMD because in this case it is forwarded to any implementation. However, in the case of delegation, and as a safeguard, what we could do to avoid driver IOCTL command overlaps is to check if the file is a block or character device. In this case, we just don't delegate any access right but make LANDLOCK_ACCESS_FS_IOCTL handle them (we need to manage VFS's IOCTLs that deal with block/char device though). Again, we should make sure that -ENOIOCTLCMD will not trick us (for the delegate cases). I'm not sure how the link between drivers and the FS storing the related block/char device is managed though. Does that make sense?
Hello! On Mon, Sep 11, 2023 at 05:25:31PM +0200, Mickaël Salaün wrote: > On Mon, Sep 11, 2023 at 12:02:46PM +0200, Günther Noack wrote: > > Thank you for making the algorithm that explicit -- that helps to trace down the > > differences. I can follow the logic now, but I still don't understand what your > > underlying rationale for that is? > > > > I believe that fundamentally, a core difference is: > > > > For an access right R and a file F, for these two cases: > > > > (a) the access right R is unhandled (nothing gets restricted) > > (b) the access right R is handled, but R is granted for F in a rule. > > > > I believe that accesses in case (a) and (b) to the file F should have the same > > results. > > > > This is at least how the existing Landlock implementation works, as far as I can > > tell. > > > > ("Refer" is an exceptional case, but we have documented that it was always > > "implicitly handled" in ABI V1, which makes it consistent again.) > > > > > > When I expand your code above to a boolean table, I end up with the following > > decisions, depending on whether IOCTL and READ are handled or not, and whether > > they are explicitly permitted for the file through a rule: > > > > > > Mickaël's IOCTL IOCTL IOCTL > > suggestion handled, handled, unhandled > > 2023-09-04 file file not > > permitted permitted > > -------------------------------------------------- > > READ handled, > > file permitted allow allow allow > > > > READ handled, > > f not permitted deny deny allow > > > > READ unhandled allow deny allow > > > > > > In patch set V3, this is different: Because I think that cases (a) and (b) from > > above should always behave the same, the first and third column and row must be > > symmetric and have the same entries. So, in patch set V3, it is sufficient if > > *one of* the two rights IOCTL and READ_FILE are present, in order to use the > > FIONREAD IOCTL: > > > > > > Günther's IOCTL IOCTL IOCTL > > patch set V3 handled, handled, unhandled > > 2023-08-14 file file not > > permitted permitted > > -------------------------------------------------- > > READ handled, > > file permitted allow allow allow > > > > READ handled, > > f not permitted allow deny allow > > > > READ unhandled allow allow allow > > A first difference is about (READ unhandled) AND (IOCTL handled + > file not permitted). It will not be possible to follow the same logic > with new Landlock access right (e.g. LANDLOCK_ACCESS_FS_READ_METADATA > that should also allow FS_IOC_FSGETXATTR), and I'd like to keep it > consistent. > > A second difference is about (READ handled + f not permitted) AND > (IOCTL handled + file permitted). The reasoning was to avoid giving too > much power to LANDLOCK_ACCESS_FS_IOCTL and dowgrade it as new access > rights are implemented. This looks quite similar to the CAP_SYS_ADMIN > right that can basically do anything, and new capabilites are mainly a > subset of this one. My proposal was to incrementally downgrade the power > given by LANDLOCK_ACCESS_FS_IOCTL while still being compatible. On the > I was thinking that, if we make a requirement to have the "new correct" > access right, the application update might drop the IOCTL access right. > I now think this reasoning is flawed. > > Indeed, this comparaison doesn't fit well because IOCTLs are not a > superset of all access rights, and because nothing can force developers > that already gave access to all IOCTLs for a file to not just add > another access right (instead of swapping them). > > Instead, I think user space libraries should manage this kind of access > right swapping when possible and have a fallback mechanism relying on > the LANDLOCK_ACCESS_FS_IOCTL right. This would be valuable because they > may be updated before the (stable system) kernel, and this would be > easier for developers to manage. > > In a nutshell, it is about giving control for an action (e.g. FIONREAD) > to either a unique access right or to a set of access rights. At first, > I would have preferred to have a unique access right to control an > action, because it is simpler (e.g. for audit/debug). On the other hand, > we need to handle access rights that allow the same action (e.g. file > read OR write for FIOQSIZE). I now think your approach (i.e. set of > access rights to control an action) could make more sense. Another good > point is to not downgrade the power of LANDLOCK_ACCESS_FS_IOCTL, which > could in fact be difficult to understand for users. Nested Landlock > domains should also be easier to manage with this logic. After we discussed this difficult topic briefly off-list, let me try to summarize my takeaways and write it up here for reference. I think the requirements for the logic of the IOCTL right are as follows: (1) In the future, if a new FOO access right is introduced, this right should implicitly give access to FOO-related IOCTLs on the affected same files, *without requiring the LANDLOCK_ACCESS_FS_IOCTL right*. Example: If in Landlock version 10, we introduce LANDLOCK_ACCESS_FS_GFX for graphics-related functionality, this access right should potentially give access to graphics-related ioctl commands. I'll use the "GFX" example below as a stand-in for a generic future access right which should give access to a set of IOCTL commands. and then the ones which are a bit more obvious: (2) When stacking additional Landlock layers, the thread's available access can only be restricted further (it should not accidentally be able to do more than before). (3) Landlock usages need to stay compatible across kernel versions. The Landlock usages that are in use today need to do the same thing in future kernel versions. I had indeed overlooked requirement (1) and did not realize that my proposal was going to be at odds with that. ## Some counterexamples for approaches that don't work So: Counterexample for why my earlier proposal (OR-combination) does not work: In my proposal, a GFX-related IOCTL would be permitted when *either one* of the ..._GFX or the ..._IOCTL rights are available for the file. (The READ right in the tables above should work the same as the GFX or FOO rights from requirement (1), for consistency). So a user who today uses handled: LANDLOCK_ACCESS_FS_IOCTL allowed: (nothing) will expect that GFX-related IOCTL operations are forbidden. (We do not know yet whether the "GFX" access right will ever exist, therefore it is covered by LANDLOCK_ACCESS_FS_IOCTL.) Now we introduce the LANDLOCK_ACCESS_FS_GFX right, and suddenly, GFX-related IOCTL commands are checked with a new logic: You *either* need to have the LANDLOCK_ACCESS_FS_IOCTL right, *or* the LANDLOCK_ACCESS_FS_GFX right. So when the user again uses handled: LANDLOCK_ACCESS_FS_IOCTL allowed: (nothing) the user would according to the new logic suddenly *have* the LANDLOCK_ACCESS_FS_GFX right, and these IOCTL commands would be permitted. This is a change of how Landlock behaves compared to the earlier version, and that is at odds with rule (3). The other obvious bitwise combination (AND) does not work either -- that one would violate requirement (1). ## A new proposal We have discussed above that one option would be to start distinguishing between the case where a right is "not handled" and the case where the right is "handled, but allowed on the file". This is not very nice, because it would be inconsistent with the semantics which we had before for all other rights. After thinking a bit more about it, one way to look at it is that we are using the "handled" flags to control how the IOCTLs are grouped. I agree that we have to control the IOCTL grouping, but I am not sure whether the "handled" flags are the right place to do that. -- We could just as well pass instructions about the IOCTL grouping out of band, and I think it might make that logic clearer: To put forward something concrete, how about this: * LANDLOCK_ACCESS_FS_IOCTL: This access right controls the invocation of IOCTL commands, unless these commands are controlled by another access right. In every layer, each IOCTL command is only controlled through one access right. * LANDLOCK_ACCESS_FS_READ_FILE: This access right controls opening files for reading, and additionally the use of the FIONREAD ioctl command. * We introduce a flag in struct landlock_ruleset_attr which controls whether the graphics-related IOCTLs are controlled through the LANDLOCK_ACCESS_FS_GFX access right, rather than through LANDLOCK_ACCESS_FS_IOCTL. (This could potentially also be put in the "flags" argument to landlock_create_ruleset(), but it feels a bit more appropriate in the struct I think, as it influences the interpretation of the logic. But I'm open to suggestions.) Example: Without the flag, the IOCTL groups will be: These are always permitted: FIOCLEX, FIONCLEX, FIONBIO, etc. LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD LANDLOCK_ACCESS_FS_IOCTL: controls all other IOCTL commands but when users set the flag, the IOCTL groups will be: These are always permitted: FIOCLEX, FIONCLEX, FIONBIO, etc. LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD LANDLOCK_ACCESS_FS_GFX: controls (list of gfx-related IOCTLs) LANDLOCK_ACCESS_FS_IOCTL: controls all other IOCTL commands Implementation-wise, I think it would actually look very similar to what would be needed for your proposal of having a new special meaning for "handled". It would have the slight advantage that the new flag is actually only needed at the time when we introduce a new way of grouping the IOCTL commands, so we would only burden users with the additional complexity when it's actually required. One implementation approach that I find reasonable to think about is to create "synthetic" access rights when rulesets are enabled. That is, we introduce LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL (name TBD), but we keep this constant private to the kernel. * *At ruleset enablement time*, we populate the bit for this access right either from the LANDLOCK_ACCESS_FS_GFX or the LANDLOCK_ACCESS_FS_IOCTL bit from the same access_mask_t, depending on the IOCTL grouping which the ruleset is configured with. * *In hook_file_open*, we then check for LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL for the GFX-related IOCTL commands. I'm in favor of using the synthetic access rights, because I find it clearer to understand that the effective access rights for a file from different layers are just combined with a bitwise AND, and will give the right results. We could probably also make these path walk helpers aware of the special cases and only have the synthetic right in layer_masks_dom, but I'd prefer not to complicate these helpers even further. Sorry for the long mail, I hope that the examples clarify it a bit. :) In summary, it seems conceptually cleaner to me to control every IOCTL command with only one access right, and let users control which one that should be with a separate flag, so that "handled" keeps its original semantics. It would also have the upside that we can delay that implementation until the time where we actually introduce new IOCTL-aware access rights on top of the current patch st. I'd be interested to hear your thoughts on it. —Günther
On Fri, Oct 20, 2023 at 12:09:36AM +0200, Günther Noack wrote: > Hello! > > On Mon, Sep 11, 2023 at 05:25:31PM +0200, Mickaël Salaün wrote: > > On Mon, Sep 11, 2023 at 12:02:46PM +0200, Günther Noack wrote: > > > Thank you for making the algorithm that explicit -- that helps to trace down the > > > differences. I can follow the logic now, but I still don't understand what your > > > underlying rationale for that is? > > > > > > I believe that fundamentally, a core difference is: > > > > > > For an access right R and a file F, for these two cases: > > > > > > (a) the access right R is unhandled (nothing gets restricted) > > > (b) the access right R is handled, but R is granted for F in a rule. > > > > > > I believe that accesses in case (a) and (b) to the file F should have the same > > > results. > > > > > > This is at least how the existing Landlock implementation works, as far as I can > > > tell. > > > > > > ("Refer" is an exceptional case, but we have documented that it was always > > > "implicitly handled" in ABI V1, which makes it consistent again.) > > > > > > > > > When I expand your code above to a boolean table, I end up with the following > > > decisions, depending on whether IOCTL and READ are handled or not, and whether > > > they are explicitly permitted for the file through a rule: > > > > > > > > > Mickaël's IOCTL IOCTL IOCTL > > > suggestion handled, handled, unhandled > > > 2023-09-04 file file not > > > permitted permitted > > > -------------------------------------------------- > > > READ handled, > > > file permitted allow allow allow > > > > > > READ handled, > > > f not permitted deny deny allow > > > > > > READ unhandled allow deny allow > > > > > > > > > In patch set V3, this is different: Because I think that cases (a) and (b) from > > > above should always behave the same, the first and third column and row must be > > > symmetric and have the same entries. So, in patch set V3, it is sufficient if > > > *one of* the two rights IOCTL and READ_FILE are present, in order to use the > > > FIONREAD IOCTL: > > > > > > > > > Günther's IOCTL IOCTL IOCTL > > > patch set V3 handled, handled, unhandled > > > 2023-08-14 file file not > > > permitted permitted > > > -------------------------------------------------- > > > READ handled, > > > file permitted allow allow allow > > > > > > READ handled, > > > f not permitted allow deny allow > > > > > > READ unhandled allow allow allow > > > > A first difference is about (READ unhandled) AND (IOCTL handled + > > file not permitted). It will not be possible to follow the same logic > > with new Landlock access right (e.g. LANDLOCK_ACCESS_FS_READ_METADATA > > that should also allow FS_IOC_FSGETXATTR), and I'd like to keep it > > consistent. > > > > A second difference is about (READ handled + f not permitted) AND > > (IOCTL handled + file permitted). The reasoning was to avoid giving too > > much power to LANDLOCK_ACCESS_FS_IOCTL and dowgrade it as new access > > rights are implemented. This looks quite similar to the CAP_SYS_ADMIN > > right that can basically do anything, and new capabilites are mainly a > > subset of this one. My proposal was to incrementally downgrade the power > > given by LANDLOCK_ACCESS_FS_IOCTL while still being compatible. On the > > I was thinking that, if we make a requirement to have the "new correct" > > access right, the application update might drop the IOCTL access right. > > I now think this reasoning is flawed. > > > > Indeed, this comparaison doesn't fit well because IOCTLs are not a > > superset of all access rights, and because nothing can force developers > > that already gave access to all IOCTLs for a file to not just add > > another access right (instead of swapping them). > > > > Instead, I think user space libraries should manage this kind of access > > right swapping when possible and have a fallback mechanism relying on > > the LANDLOCK_ACCESS_FS_IOCTL right. This would be valuable because they > > may be updated before the (stable system) kernel, and this would be > > easier for developers to manage. > > > > In a nutshell, it is about giving control for an action (e.g. FIONREAD) > > to either a unique access right or to a set of access rights. At first, > > I would have preferred to have a unique access right to control an > > action, because it is simpler (e.g. for audit/debug). On the other hand, > > we need to handle access rights that allow the same action (e.g. file > > read OR write for FIOQSIZE). I now think your approach (i.e. set of > > access rights to control an action) could make more sense. Another good > > point is to not downgrade the power of LANDLOCK_ACCESS_FS_IOCTL, which > > could in fact be difficult to understand for users. Nested Landlock > > domains should also be easier to manage with this logic. Hmm, in fact I think my initial reasoning was better. I still agree with my suggestion from 2023-09-04, and I think you're now proposing the same. Handling access right in an exclusive way (i.e. a specific IOCTL command is always handle by only one access rigth, but this one may depend on the handle access rights) is indeed better because it will force developers to not use the generic IOCTL access right instead of e.g. the GFX one once they want to use it. In practice, it can reduce the set of IOCTL commands allowed for LANDLOCK_ACCESS_FS_IOCTL-only file hierarchies if users only allow the new LANDLOCK_ACCESS_FS_GFX to the appropriate (e.g. /dev/dri) files. To summarize, the reasoning is as follow: when READ is handled, the related IOCTL commands (e.g. FIONREAD) are delegated to the READ access right (both for the ruleset and the related rules), and the IOCTL access right doesn't handle at all FIONREAD. If READ is not handled, then the IOCTL access right handles FIONREAD as well. The same logic applies to any current or future access rights, like LANDLOCK_ACCESS_FS_GFX. > > After we discussed this difficult topic briefly off-list, let me try to > summarize my takeaways and write it up here for reference. > > I think the requirements for the logic of the IOCTL right are as follows: > > (1) In the future, if a new FOO access right is introduced, this right should > implicitly give access to FOO-related IOCTLs on the affected same files, > *without requiring the LANDLOCK_ACCESS_FS_IOCTL right*. > > Example: If in Landlock version 10, we introduce LANDLOCK_ACCESS_FS_GFX for > graphics-related functionality, this access right should potentially give > access to graphics-related ioctl commands. I'll use the "GFX" example > below as a stand-in for a generic future access right which should give > access to a set of IOCTL commands. > > and then the ones which are a bit more obvious: > > (2) When stacking additional Landlock layers, the thread's available access can > only be restricted further (it should not accidentally be able to do more > than before). > > (3) Landlock usages need to stay compatible across kernel versions. > The Landlock usages that are in use today need to do the same thing > in future kernel versions. > > I had indeed overlooked requirement (1) and did not realize that my proposal was > going to be at odds with that. > > > > ## Some counterexamples for approaches that don't work > > So: Counterexample for why my earlier proposal (OR-combination) does not work: > > In my proposal, a GFX-related IOCTL would be permitted when *either one* of > the ..._GFX or the ..._IOCTL rights are available for the file. (The READ > right in the tables above should work the same as the GFX or FOO rights from > requirement (1), for consistency). > > So a user who today uses > > handled: LANDLOCK_ACCESS_FS_IOCTL > allowed: (nothing) > > will expect that GFX-related IOCTL operations are forbidden. (We do not know > yet whether the "GFX" access right will ever exist, therefore it is covered by > LANDLOCK_ACCESS_FS_IOCTL.) > > Now we introduce the LANDLOCK_ACCESS_FS_GFX right, and suddenly, GFX-related > IOCTL commands are checked with a new logic: You *either* need to have the > LANDLOCK_ACCESS_FS_IOCTL right, *or* the LANDLOCK_ACCESS_FS_GFX right. So > when the user again uses > > handled: LANDLOCK_ACCESS_FS_IOCTL > allowed: (nothing) > > the user would according to the new logic suddenly *have* the > LANDLOCK_ACCESS_FS_GFX right, and these IOCTL commands would be permitted. > > This is a change of how Landlock behaves compared to the earlier version, > and that is at odds with rule (3). > > > The other obvious bitwise combination (AND) does not work either -- that one > would violate requirement (1). > Good summary, thanks! > > ## A new proposal > > We have discussed above that one option would be to start distinguishing between > the case where a right is "not handled" and the case where the right is > "handled, but allowed on the file". > > This is not very nice, because it would be inconsistent with the semantics which > we had before for all other rights. > > After thinking a bit more about it, one way to look at it is that we are using > the "handled" flags to control how the IOCTLs are grouped. I agree that we have > to control the IOCTL grouping, but I am not sure whether the "handled" flags are > the right place to do that. -- We could just as well pass instructions about the > IOCTL grouping out of band, and I think it might make that logic clearer: > > To put forward something concrete, how about this: > > * LANDLOCK_ACCESS_FS_IOCTL: This access right controls the invocation of IOCTL > commands, unless these commands are controlled by another access right. > > In every layer, each IOCTL command is only controlled through one access right. Yes, I agree with that, see the reasoning about handling access right in an exclusive way above. > > * LANDLOCK_ACCESS_FS_READ_FILE: This access right controls opening files for > reading, and additionally the use of the FIONREAD ioctl command. Yes > > * We introduce a flag in struct landlock_ruleset_attr which controls whether the > graphics-related IOCTLs are controlled through the LANDLOCK_ACCESS_FS_GFX > access right, rather than through LANDLOCK_ACCESS_FS_IOCTL. > > (This could potentially also be put in the "flags" argument to > landlock_create_ruleset(), but it feels a bit more appropriate in the struct I > think, as it influences the interpretation of the logic. But I'm open to > suggestions.) > What would be the difference with creating a LANDLOCK_ACCESS_FS_GFX_IOCTL access right? The main issue with this approach is that it complexifies the usage of Landlock, and users would need to tweak more knobs to configure a ruleset. What about keeping my proposal (mainly the IOCTL handling and delegation logic) for the user interface, and translate that for kernel internals to your proposal? See the below example. > > Example: Without the flag, the IOCTL groups will be: > > These are always permitted: FIOCLEX, FIONCLEX, FIONBIO, etc. > LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD > LANDLOCK_ACCESS_FS_IOCTL: controls all other IOCTL commands > > but when users set the flag, the IOCTL groups will be: > > These are always permitted: FIOCLEX, FIONCLEX, FIONBIO, etc. > LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD > LANDLOCK_ACCESS_FS_GFX: controls (list of gfx-related IOCTLs) > LANDLOCK_ACCESS_FS_IOCTL: controls all other IOCTL commands > Does this mean that handling LANDLOCK_ACCESS_FS_GFX without the flag would not allow GFX-related IOCTL commands? Thit would be inconsistent with the way LANDLOCK_ACCESS_FS_READ_FILE is handled. Would this flag works with non-GFX access rights as well? Would there be potentially one new flag per new access right? > > Implementation-wise, I think it would actually look very similar to what would > be needed for your proposal of having a new special meaning for "handled". It > would have the slight advantage that the new flag is actually only needed at the > time when we introduce a new way of grouping the IOCTL commands, so we would > only burden users with the additional complexity when it's actually required. Indeed, and burdening users with more flags would increase the cost of (properly) using Landlock. I'm definitely in favor to make the Landlock interface as simple as possible, taking into account the inherent compatibilty complexity, and pushing most of this complexity handling to user space libraries, and if it not possible, pushing the rest of the complexity into the kernel. > > One implementation approach that I find reasonable to think about is to create > "synthetic" access rights when rulesets are enabled. That is, we introduce > LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL (name TBD), but we keep this constant > private to the kernel. > > * *At ruleset enablement time*, we populate the bit for this access right either > from the LANDLOCK_ACCESS_FS_GFX or the LANDLOCK_ACCESS_FS_IOCTL bit from the > same access_mask_t, depending on the IOCTL grouping which the ruleset is > configured with. > > * *In hook_file_open*, we then check for LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL > for the GFX-related IOCTL commands. > > I'm in favor of using the synthetic access rights, because I find it clearer to > understand that the effective access rights for a file from different layers are > just combined with a bitwise AND, and will give the right results. We could > probably also make these path walk helpers aware of the special cases and only > have the synthetic right in layer_masks_dom, but I'd prefer not to complicate > these helpers even further. I like this synthetic access right approach, but what worries me is that it will potentially double the number of access rights. This is not an issue for the handled access right (i.e. per ruleset layer), but we should avoid that for allowed accesses (i.e. rules). Indeed, the layer_masks[] size is proportional to the number of potential allowed access rights, and increasing this array could increase the kernel stack size (see is_access_to_paths_allowed). It would not be an issue for now though, we have a lot of room, it is just something to keep in mind. Because of the way we need to compare file hierarchies (cf. FS_REFER), it seems to be safer to only rely on (synthetic) access rights. So I think it is the right approach. > > > Sorry for the long mail, I hope that the examples clarify it a bit. :) > > In summary, it seems conceptually cleaner to me to control every IOCTL command > with only one access right, and let users control which one that should be with > a separate flag, so that "handled" keeps its original semantics. It would also > have the upside that we can delay that implementation until the time where we > actually introduce new IOCTL-aware access rights on top of the current patch st. I don't see how we'll not get an inconsistent logic: a first one with old/current access rights, and another one for future access rights (e.g. GFX). > > I'd be interested to hear your thoughts on it. Thanks for this detailed explanation, that is useful. I'm in favor of the synthetic access right, but I'd like to not add optional flags to the user API. What do you think about the kernel doing the translation to the synthetic access rights? To make the reasoning easier for the kernel implementation, following the synthetic access rights idea, we can create these groups: * IOCTL_CMD_G1: FIOQSIZE * IOCTL_CMD_G2: FS_IOCT_FIEMAP | FIBMAP | FIGETBSZ * IOCTL_CMD_G3: FIONREAD | FIDEDUPRANGE * IOCTL_CMD_G4: FICLONE | FICLONERANGE | FS_IOC_RESVSP | FS_IOC_RESVSP64 | FS_IOC_UNRESVSP | FS_IOC_UNRESVSP64 | FS_IOC_ZERO_RANGE Existing (and future) access rights would automatically get the related IOCTL fine-grained rights *if* LANDLOCK_ACCESS_FS_IOCTL is handled: * LANDLOCK_ACCESS_FS_WRITE_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4 * LANDLOCK_ACCESS_FS_READ_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3 * LANDLOCK_ACCESS_FS_READ_DIR: IOCTL_CMD_G1 This works with the ruleset handled access rights and the related rules allowed accesses by simply ORing the access rights. We should also keep in mind that some IOCTL commands may only be related to some specific file types or filesystems, either now or in the future (see the GFX example).
On Fri, Oct 20, 2023 at 04:57:39PM +0200, Mickaël Salaün wrote: > On Fri, Oct 20, 2023 at 12:09:36AM +0200, Günther Noack wrote: > > * We introduce a flag in struct landlock_ruleset_attr which controls whether the > > graphics-related IOCTLs are controlled through the LANDLOCK_ACCESS_FS_GFX > > access right, rather than through LANDLOCK_ACCESS_FS_IOCTL. > > > > (This could potentially also be put in the "flags" argument to > > landlock_create_ruleset(), but it feels a bit more appropriate in the struct I > > think, as it influences the interpretation of the logic. But I'm open to > > suggestions.) > > > > What would be the difference with creating a > LANDLOCK_ACCESS_FS_GFX_IOCTL access right? > > The main issue with this approach is that it complexifies the usage of > Landlock, and users would need to tweak more knobs to configure a > ruleset. > > What about keeping my proposal (mainly the IOCTL handling and delegation > logic) for the user interface, and translate that for kernel internals > to your proposal? See the below example. Yes! I have pondered this for about a day now, and tried to break the example in various ways, but I believe you are right with this -- I think we can actually use the "handled" flags to control the IOCTL grouping, and then translate all of it quickly to synthetic access rights for the internal logic. When doing the translation only once during ruleset enablement time, we can keep using the existing logic for the synthetic rights and it'll obviously work correctly when layers are stacked. (I paraphrase it in more detail at the end, to make sure we are on the same page. -- But I think we are.) > > Example: Without the flag, the IOCTL groups will be: > > > > These are always permitted: FIOCLEX, FIONCLEX, FIONBIO, etc. > > LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD > > LANDLOCK_ACCESS_FS_IOCTL: controls all other IOCTL commands > > > > but when users set the flag, the IOCTL groups will be: > > > > These are always permitted: FIOCLEX, FIONCLEX, FIONBIO, etc. > > LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD > > LANDLOCK_ACCESS_FS_GFX: controls (list of gfx-related IOCTLs) > > LANDLOCK_ACCESS_FS_IOCTL: controls all other IOCTL commands > > > > Does this mean that handling LANDLOCK_ACCESS_FS_GFX without the flag > would not allow GFX-related IOCTL commands? Thit would be inconsistent > with the way LANDLOCK_ACCESS_FS_READ_FILE is handled. Yes, that is how I had imagined that. It's true that it's slightly inconsistent in usage, and you are right that it creates some new concepts in the API which are maybe avoidable. Let's try it the way you proposed and control it with the "handled" flags. > Would this flag works with non-GFX access rights as well? Would there be > potentially one new flag per new access right? > > > > > Implementation-wise, I think it would actually look very similar to what would > > be needed for your proposal of having a new special meaning for "handled". It > > would have the slight advantage that the new flag is actually only needed at the > > time when we introduce a new way of grouping the IOCTL commands, so we would > > only burden users with the additional complexity when it's actually required. > > Indeed, and burdening users with more flags would increase the cost of > (properly) using Landlock. > > I'm definitely in favor to make the Landlock interface as simple as > possible, taking into account the inherent compatibilty complexity, and > pushing most of this complexity handling to user space libraries, and if > it not possible, pushing the rest of the complexity into the kernel. Ack, sounds good. > > One implementation approach that I find reasonable to think about is to create > > "synthetic" access rights when rulesets are enabled. That is, we introduce > > LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL (name TBD), but we keep this constant > > private to the kernel. > > > > * *At ruleset enablement time*, we populate the bit for this access right either > > from the LANDLOCK_ACCESS_FS_GFX or the LANDLOCK_ACCESS_FS_IOCTL bit from the > > same access_mask_t, depending on the IOCTL grouping which the ruleset is > > configured with. > > > > * *In hook_file_open*, we then check for LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL > > for the GFX-related IOCTL commands. > > > > I'm in favor of using the synthetic access rights, because I find it clearer to > > understand that the effective access rights for a file from different layers are > > just combined with a bitwise AND, and will give the right results. We could > > probably also make these path walk helpers aware of the special cases and only > > have the synthetic right in layer_masks_dom, but I'd prefer not to complicate > > these helpers even further. > > I like this synthetic access right approach, but what worries me is that > it will potentially double the number of access rights. This is not an > issue for the handled access right (i.e. per ruleset layer), but we > should avoid that for allowed accesses (i.e. rules). Indeed, the > layer_masks[] size is proportional to the number of potential allowed > access rights, and increasing this array could increase the kernel stack > size (see is_access_to_paths_allowed). It would not be an issue for now > though, we have a lot of room, it is just something to keep in mind. Yes, acknowledged. FWIW, LANDLOCK_ACCESS_FS_IOCTL is already 1 << 15, so adding the synthetic rights will indeed make access_mask_t go up to 32 bit. (This was already done in the patch for the metadata access, but that one was not merged yet.) I also feel that the stack usage is the case where this is most likely to be an issue. > Because of the way we need to compare file hierarchies (cf. FS_REFER), > it seems to be safer to only rely on (synthetic) access rights. So I > think it is the right approach. > > > > > > > Sorry for the long mail, I hope that the examples clarify it a bit. :) > > > > In summary, it seems conceptually cleaner to me to control every IOCTL command > > with only one access right, and let users control which one that should be with > > a separate flag, so that "handled" keeps its original semantics. It would also > > have the upside that we can delay that implementation until the time where we > > actually introduce new IOCTL-aware access rights on top of the current patch st. > > I don't see how we'll not get an inconsistent logic: a first one with > old/current access rights, and another one for future access rights > (e.g. GFX). > > > > > I'd be interested to hear your thoughts on it. > > Thanks for this detailed explanation, that is useful. > > I'm in favor of the synthetic access right, but I'd like to not add > optional flags to the user API. What do you think about the kernel > doing the translation to the synthetic access rights? > > To make the reasoning easier for the kernel implementation, following > the synthetic access rights idea, we can create these groups: > > * IOCTL_CMD_G1: FIOQSIZE > * IOCTL_CMD_G2: FS_IOCT_FIEMAP | FIBMAP | FIGETBSZ > * IOCTL_CMD_G3: FIONREAD | FIDEDUPRANGE > * IOCTL_CMD_G4: FICLONE | FICLONERANGE | FS_IOC_RESVSP | FS_IOC_RESVSP64 > | FS_IOC_UNRESVSP | FS_IOC_UNRESVSP64 | FS_IOC_ZERO_RANGE > > Existing (and future) access rights would automatically get the related > IOCTL fine-grained rights *if* LANDLOCK_ACCESS_FS_IOCTL is handled: > * LANDLOCK_ACCESS_FS_WRITE_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4 > * LANDLOCK_ACCESS_FS_READ_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3 > * LANDLOCK_ACCESS_FS_READ_DIR: IOCTL_CMD_G1 > > This works with the ruleset handled access rights and the related rules > allowed accesses by simply ORing the access rights. > > We should also keep in mind that some IOCTL commands may only be related > to some specific file types or filesystems, either now or in the future > (see the GFX example). I am coming around to your approach with using "handled" bits to determine the grouping. Let me paraphrase some key concepts to make sure we are on the same page: * The IOCTL groups are modeled as synthetic access rights, IOCTL_CMD_G1...G4 in your example. Each IOCTL command maps to exactly one of these groups. Because the presence of these groups is an implementation detail in the kernel, we can adapt it later and make it more fine-grained if needed. * We use "handled" bits like LANDLOCK_ACCESS_FS_WRITE_FILE to determine the synthetic access rights. We can populate the synthetic IOCTL_CMD_G1...G4 groups depending on how the "handled" bits are populated. In my understanding, the logic could roughly be this: static access_mask_t expand_ioctl(access_mask_t handled, access_mask_t am, access_mask_t src, access_mask_t dst) { if (handled & src) { /* If "src" access right is handled, populate "dst" from "src". */ return am | ((am & src) ? dst : 0); } else { /* Otherwise, populate "dst" flag from "ioctl" flag. */ return am | ((am & LANDLOCK_ACCESS_FS_IOCTL) ? dst : 0); } } static access_mask_t expand_all_ioctl(access_mask_t handled, access_mask_t am) { am = expand_ioctl(handled, am, LANDLOCK_ACCESS_FS_WRITE_FILE, IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4); am = expand_ioctl(handled, am, LANDLOCK_ACCESS_FS_READ_FILE, IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3); am = expand_ioctl(handled, am, LANDLOCK_ACCESS_FS_READ_DIR, IOCTL_CMD_G1); return am; } and then during the installing of a ruleset, we'd call expand_all_ioctl(handled, access) for each specified file access, and expand_all_ioctl(handled, handled) for the handled access rights, to populate the synthetic IOCTL_CMD_G* access rights. In expand_ioctl() above, if "src" is *not* handled, we populate the associated synthetic access rights "dst" from the value in LANDLOCK_ACCESS_FS_IOCTL. With that, when enabling a ruleset, we map everything to the most specific grouping which is available, and later on, the LSM hook can just ignore that different grouping configurations are possible. * In the ioctl LSM hook, each possible cmd is controlled by exactly one access right. The ones that you have listed are all controlled by one of the IOCTL_CMD_G1...G4 access rights, and all others by LANDLOCK_ACCESS_FS_IOCTL. I was previously concerned that the usage of "handled" to control the grouping would be at odds with the layer composition logic, but with this logic, we are now mapping these to the synthetic access rights at enablement time, and all the ruleset composition logic can stay working as it is (at least until we run out of bits in access_mask_t). I've also been concerned before that we would break compatibility across versions, but this also seems less likely now that we've discussed this in all this detail %-) I suspect that the normal upgrade path from one Landlock version to the next will be for most users to always use the full set of "handled" flags that their library knows about. When we add the hypothetical "GFX" flag to that set, this will change the IOCTL grouping a bit, so that files which were previously listed as having the LANDLOCK_ACCESS_FS_IOCTL right, might now not be enabled for GFX ioctls. But that is (A) probably correct anyway in most cases, and (B) users upgrading from one Landlock ABI version to the next have a chance to read their library changelog as part of that upgrade. I think this is a reasonable approach. If you agree, I'm willing to give it a shot and adapt the patch set to implement that. —Günther
On Thu, Oct 26, 2023 at 12:07:28AM +0200, Günther Noack wrote: > On Fri, Oct 20, 2023 at 04:57:39PM +0200, Mickaël Salaün wrote: > > On Fri, Oct 20, 2023 at 12:09:36AM +0200, Günther Noack wrote: > > > * We introduce a flag in struct landlock_ruleset_attr which controls whether the > > > graphics-related IOCTLs are controlled through the LANDLOCK_ACCESS_FS_GFX > > > access right, rather than through LANDLOCK_ACCESS_FS_IOCTL. > > > > > > (This could potentially also be put in the "flags" argument to > > > landlock_create_ruleset(), but it feels a bit more appropriate in the struct I > > > think, as it influences the interpretation of the logic. But I'm open to > > > suggestions.) > > > > > > > What would be the difference with creating a > > LANDLOCK_ACCESS_FS_GFX_IOCTL access right? > > > > The main issue with this approach is that it complexifies the usage of > > Landlock, and users would need to tweak more knobs to configure a > > ruleset. > > > > What about keeping my proposal (mainly the IOCTL handling and delegation > > logic) for the user interface, and translate that for kernel internals > > to your proposal? See the below example. > > Yes! > > I have pondered this for about a day now, and tried to break the example in > various ways, but I believe you are right with this -- I think we can actually > use the "handled" flags to control the IOCTL grouping, and then translate all of > it quickly to synthetic access rights for the internal logic. When doing the > translation only once during ruleset enablement time, we can keep using the > existing logic for the synthetic rights and it'll obviously work correctly when > layers are stacked. (I paraphrase it in more detail at the end, to make sure we > are on the same page. -- But I think we are.) > > > > > Example: Without the flag, the IOCTL groups will be: > > > > > > These are always permitted: FIOCLEX, FIONCLEX, FIONBIO, etc. > > > LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD > > > LANDLOCK_ACCESS_FS_IOCTL: controls all other IOCTL commands > > > > > > but when users set the flag, the IOCTL groups will be: > > > > > > These are always permitted: FIOCLEX, FIONCLEX, FIONBIO, etc. > > > LANDLOCK_ACCESS_FS_READ_FILE: controls FIONREAD > > > LANDLOCK_ACCESS_FS_GFX: controls (list of gfx-related IOCTLs) > > > LANDLOCK_ACCESS_FS_IOCTL: controls all other IOCTL commands > > > > > > > Does this mean that handling LANDLOCK_ACCESS_FS_GFX without the flag > > would not allow GFX-related IOCTL commands? Thit would be inconsistent > > with the way LANDLOCK_ACCESS_FS_READ_FILE is handled. > > Yes, that is how I had imagined that. It's true that it's slightly inconsistent > in usage, and you are right that it creates some new concepts in the API which > are maybe avoidable. Let's try it the way you proposed and control it with the > "handled" flags. > > > > Would this flag works with non-GFX access rights as well? Would there be > > potentially one new flag per new access right? > > > > > > > > Implementation-wise, I think it would actually look very similar to what would > > > be needed for your proposal of having a new special meaning for "handled". It > > > would have the slight advantage that the new flag is actually only needed at the > > > time when we introduce a new way of grouping the IOCTL commands, so we would > > > only burden users with the additional complexity when it's actually required. > > > > Indeed, and burdening users with more flags would increase the cost of > > (properly) using Landlock. > > > > I'm definitely in favor to make the Landlock interface as simple as > > possible, taking into account the inherent compatibilty complexity, and > > pushing most of this complexity handling to user space libraries, and if > > it not possible, pushing the rest of the complexity into the kernel. > > Ack, sounds good. > > > > > One implementation approach that I find reasonable to think about is to create > > > "synthetic" access rights when rulesets are enabled. That is, we introduce > > > LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL (name TBD), but we keep this constant > > > private to the kernel. > > > > > > * *At ruleset enablement time*, we populate the bit for this access right either > > > from the LANDLOCK_ACCESS_FS_GFX or the LANDLOCK_ACCESS_FS_IOCTL bit from the > > > same access_mask_t, depending on the IOCTL grouping which the ruleset is > > > configured with. > > > > > > * *In hook_file_open*, we then check for LANDLOCK_ACCESS_FS_SYNTHETIC_GFX_IOCTL > > > for the GFX-related IOCTL commands. > > > > > > I'm in favor of using the synthetic access rights, because I find it clearer to > > > understand that the effective access rights for a file from different layers are > > > just combined with a bitwise AND, and will give the right results. We could > > > probably also make these path walk helpers aware of the special cases and only > > > have the synthetic right in layer_masks_dom, but I'd prefer not to complicate > > > these helpers even further. > > > > I like this synthetic access right approach, but what worries me is that > > it will potentially double the number of access rights. This is not an > > issue for the handled access right (i.e. per ruleset layer), but we > > should avoid that for allowed accesses (i.e. rules). Indeed, the > > layer_masks[] size is proportional to the number of potential allowed > > access rights, and increasing this array could increase the kernel stack > > size (see is_access_to_paths_allowed). It would not be an issue for now > > though, we have a lot of room, it is just something to keep in mind. > > Yes, acknowledged. > > FWIW, LANDLOCK_ACCESS_FS_IOCTL is already 1 << 15, so adding the synthetic > rights will indeed make access_mask_t go up to 32 bit. (This was already done > in the patch for the metadata access, but that one was not merged yet.) I also > feel that the stack usage is the case where this is most likely to be an issue. > > > > Because of the way we need to compare file hierarchies (cf. FS_REFER), > > it seems to be safer to only rely on (synthetic) access rights. So I > > think it is the right approach. > > > > > > > > > > > Sorry for the long mail, I hope that the examples clarify it a bit. :) > > > > > > In summary, it seems conceptually cleaner to me to control every IOCTL command > > > with only one access right, and let users control which one that should be with > > > a separate flag, so that "handled" keeps its original semantics. It would also > > > have the upside that we can delay that implementation until the time where we > > > actually introduce new IOCTL-aware access rights on top of the current patch st. > > > > I don't see how we'll not get an inconsistent logic: a first one with > > old/current access rights, and another one for future access rights > > (e.g. GFX). > > > > > > > > I'd be interested to hear your thoughts on it. > > > > Thanks for this detailed explanation, that is useful. > > > > I'm in favor of the synthetic access right, but I'd like to not add > > optional flags to the user API. What do you think about the kernel > > doing the translation to the synthetic access rights? > > > > To make the reasoning easier for the kernel implementation, following > > the synthetic access rights idea, we can create these groups: > > > > * IOCTL_CMD_G1: FIOQSIZE > > * IOCTL_CMD_G2: FS_IOCT_FIEMAP | FIBMAP | FIGETBSZ > > * IOCTL_CMD_G3: FIONREAD | FIDEDUPRANGE > > * IOCTL_CMD_G4: FICLONE | FICLONERANGE | FS_IOC_RESVSP | FS_IOC_RESVSP64 > > | FS_IOC_UNRESVSP | FS_IOC_UNRESVSP64 | FS_IOC_ZERO_RANGE > > > > Existing (and future) access rights would automatically get the related > > IOCTL fine-grained rights *if* LANDLOCK_ACCESS_FS_IOCTL is handled: > > * LANDLOCK_ACCESS_FS_WRITE_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4 > > * LANDLOCK_ACCESS_FS_READ_FILE: IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3 > > * LANDLOCK_ACCESS_FS_READ_DIR: IOCTL_CMD_G1 > > > > This works with the ruleset handled access rights and the related rules > > allowed accesses by simply ORing the access rights. > > > > We should also keep in mind that some IOCTL commands may only be related > > to some specific file types or filesystems, either now or in the future > > (see the GFX example). > > I am coming around to your approach with using "handled" bits to determine the > grouping. Let me paraphrase some key concepts to make sure we are on the same > page: > > * The IOCTL groups are modeled as synthetic access rights, IOCTL_CMD_G1...G4 in > your example. Each IOCTL command maps to exactly one of these groups. > > Because the presence of these groups is an implementation detail in the > kernel, we can adapt it later and make it more fine-grained if needed. > > * We use "handled" bits like LANDLOCK_ACCESS_FS_WRITE_FILE to determine the > synthetic access rights. > > We can populate the synthetic IOCTL_CMD_G1...G4 groups depending on how the > "handled" bits are populated. > > In my understanding, the logic could roughly be this: > > static access_mask_t expand_ioctl(access_mask_t handled, access_mask_t am, > access_mask_t src, access_mask_t dst) > { The third column "IOCTL unhandled" is not reflected here. What about this patch? if (!(handled & LANDLOCK_ACCESS_FS_IOCTL)) { return am | dst; } > if (handled & src) { > /* If "src" access right is handled, populate "dst" from "src". */ > return am | ((am & src) ? dst : 0); > } else { > /* Otherwise, populate "dst" flag from "ioctl" flag. */ > return am | ((am & LANDLOCK_ACCESS_FS_IOCTL) ? dst : 0); > } > } > > static access_mask_t expand_all_ioctl(access_mask_t handled, access_mask_t am) > { Instead of reapeating "am | " in expand_ioctl() and assigning am several times in expand_all_ioctl(), you could simply do something like that: return am | expand_ioctl(handled, am, ...) | expand_ioctl(handled, am, ...) | expand_ioctl(handled, am, ...); > am = expand_ioctl(handled, am, > LANDLOCK_ACCESS_FS_WRITE_FILE, > IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4); > am = expand_ioctl(handled, am, > LANDLOCK_ACCESS_FS_READ_FILE, > IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3); > am = expand_ioctl(handled, am, > LANDLOCK_ACCESS_FS_READ_DIR, > IOCTL_CMD_G1); > return am; > } > > and then during the installing of a ruleset, we'd call > expand_all_ioctl(handled, access) for each specified file access, and > expand_all_ioctl(handled, handled) for the handled access rights, > to populate the synthetic IOCTL_CMD_G* access rights. We can do these transformations directly in the new landlock_add_fs_access_mask() and landlock_append_fs_rule(). Please base the next series on https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next This branch might be rebased from time to time, but only minor changes will get there. > > In expand_ioctl() above, if "src" is *not* handled, we populate the associated > synthetic access rights "dst" from the value in LANDLOCK_ACCESS_FS_IOCTL. > With that, when enabling a ruleset, we map everything to the most specific > grouping which is available, and later on, the LSM hook can just ignore that > different grouping configurations are possible. > > * In the ioctl LSM hook, each possible cmd is controlled by exactly one access > right. The ones that you have listed are all controlled by one of the > IOCTL_CMD_G1...G4 access rights, and all others by LANDLOCK_ACCESS_FS_IOCTL. > > I was previously concerned that the usage of "handled" to control the grouping > would be at odds with the layer composition logic, but with this logic, we are > now mapping these to the synthetic access rights at enablement time, and all the > ruleset composition logic can stay working as it is (at least until we run out > of bits in access_mask_t). > > I've also been concerned before that we would break compatibility across > versions, but this also seems less likely now that we've discussed this in all > this detail %-) > > I suspect that the normal upgrade path from one Landlock version to the next > will be for most users to always use the full set of "handled" flags that their > library knows about. When we add the hypothetical "GFX" flag to that set, this > will change the IOCTL grouping a bit, so that files which were previously listed > as having the LANDLOCK_ACCESS_FS_IOCTL right, might now not be enabled for GFX > ioctls. But that is (A) probably correct anyway in most cases, and (B) users > upgrading from one Landlock ABI version to the next have a chance to read their > library changelog as part of that upgrade. Yes, explicitly adding a new flag to a function argument should indeed lead to read the related documentation, and hopefully test the code in an up-to-date sandbox environment! This strategy should help avoid long-term use of the generic LANDLOCK_ACCESS_FS_IOCTL but converge to new dedicated access rights instead. > > I think this is a reasonable approach. If you agree, I'm willing to give it a > shot and adapt the patch set to implement that. This looks great! > > —Günther
Hello Mickaël! Thanks for the review! On Thu, Oct 26, 2023 at 04:55:30PM +0200, Mickaël Salaün wrote: > The third column "IOCTL unhandled" is not reflected here. What about > this patch? > > if (!(handled & LANDLOCK_ACCESS_FS_IOCTL)) { > return am | dst; > } You are right that this needs special treatment. The reasoning is the scenario where a user creates a ruleset where LANDLOCK_ACCESS_FS_READ_FILE is handled, but LANDLOCK_ACCESS_FS_IOCTL is not. In that case, when a file is opened for which we do not have the READ_FILE access right, without your additional check, the IOCTLs associated with READ_FILE would be forbidden. But this is also a Landlock usage that was possible before the introduction of the IOCTL handling, and so all IOCTLs should work in that case. > > > if (handled & src) { > > /* If "src" access right is handled, populate "dst" from "src". */ > > return am | ((am & src) ? dst : 0); > > } else { > > /* Otherwise, populate "dst" flag from "ioctl" flag. */ > > return am | ((am & LANDLOCK_ACCESS_FS_IOCTL) ? dst : 0); > > } > > } > > > > static access_mask_t expand_all_ioctl(access_mask_t handled, access_mask_t am) > > { > > Instead of reapeating "am | " in expand_ioctl() and assigning am several > times in expand_all_ioctl(), you could simply do something like that: > > return am | > expand_ioctl(handled, am, ...) | > expand_ioctl(handled, am, ...) | > expand_ioctl(handled, am, ...); Agreed, this is more elegant. Will do. > > am = expand_ioctl(handled, am, > > LANDLOCK_ACCESS_FS_WRITE_FILE, > > IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4); > > am = expand_ioctl(handled, am, > > LANDLOCK_ACCESS_FS_READ_FILE, > > IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3); > > am = expand_ioctl(handled, am, > > LANDLOCK_ACCESS_FS_READ_DIR, > > IOCTL_CMD_G1); > > return am; > > } > > > > and then during the installing of a ruleset, we'd call > > expand_all_ioctl(handled, access) for each specified file access, and > > expand_all_ioctl(handled, handled) for the handled access rights, > > to populate the synthetic IOCTL_CMD_G* access rights. > > We can do these transformations directly in the new > landlock_add_fs_access_mask() and landlock_append_fs_rule(). Working on these changes, the location of these transformations is one of the last outstanding problems that I don't like yet. I have added the expansion code to landlock_add_fs_access_mask() and landlock_append_fs_rule() as you suggested. This works, but as a result, this (somewhat complicated) expansion logic is now part of the ruleset.o module, where it seems a bit too FS-specific. I think that maybe we can pull this out further, but I'll probably send you a patch set with the current status before doing that, so that we are on the same page. > Please base the next series on > https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next > This branch might be rebased from time to time, but only minor changes > will get there. OK, will do. In summary, I'll send a patch soon. FYI, some open questions I still have are: * Logic * How will userspace libraries handle best-effort fallback, when expanded IOCTL access rights come into play? (Still need to think about this more.) * Internal code layout * Move expansion logic out of ruleset.o module into syscalls.o? * Find more appropriate names for IOCTL_CMD_G1,...,IOCTL_CMD_G4 but we can discuss these in the context of the next patch set. —Günther
On Fri, Nov 03, 2023 at 02:06:53PM +0100, Günther Noack wrote: > Hello Mickaël! > > Thanks for the review! > > On Thu, Oct 26, 2023 at 04:55:30PM +0200, Mickaël Salaün wrote: > > The third column "IOCTL unhandled" is not reflected here. What about > > this patch? > > > > if (!(handled & LANDLOCK_ACCESS_FS_IOCTL)) { > > return am | dst; > > } > > You are right that this needs special treatment. The reasoning is the scenario > where a user creates a ruleset where LANDLOCK_ACCESS_FS_READ_FILE is handled, > but LANDLOCK_ACCESS_FS_IOCTL is not. In that case, when a file is opened for > which we do not have the READ_FILE access right, without your additional check, > the IOCTLs associated with READ_FILE would be forbidden. But this is also a > Landlock usage that was possible before the introduction of the IOCTL handling, > and so all IOCTLs should work in that case. > > > > > > if (handled & src) { > > > /* If "src" access right is handled, populate "dst" from "src". */ > > > return am | ((am & src) ? dst : 0); > > > } else { > > > /* Otherwise, populate "dst" flag from "ioctl" flag. */ > > > return am | ((am & LANDLOCK_ACCESS_FS_IOCTL) ? dst : 0); > > > } > > > } > > > > > > static access_mask_t expand_all_ioctl(access_mask_t handled, access_mask_t am) > > > { > > > > Instead of reapeating "am | " in expand_ioctl() and assigning am several > > times in expand_all_ioctl(), you could simply do something like that: > > > > return am | > > expand_ioctl(handled, am, ...) | > > expand_ioctl(handled, am, ...) | > > expand_ioctl(handled, am, ...); > > Agreed, this is more elegant. Will do. > > > > > am = expand_ioctl(handled, am, > > > LANDLOCK_ACCESS_FS_WRITE_FILE, > > > IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G4); > > > am = expand_ioctl(handled, am, > > > LANDLOCK_ACCESS_FS_READ_FILE, > > > IOCTL_CMD_G1 | IOCTL_CMD_G2 | IOCTL_CMD_G3); > > > am = expand_ioctl(handled, am, > > > LANDLOCK_ACCESS_FS_READ_DIR, > > > IOCTL_CMD_G1); > > > return am; > > > } > > > > > > and then during the installing of a ruleset, we'd call > > > expand_all_ioctl(handled, access) for each specified file access, and > > > expand_all_ioctl(handled, handled) for the handled access rights, > > > to populate the synthetic IOCTL_CMD_G* access rights. > > > > We can do these transformations directly in the new > > landlock_add_fs_access_mask() and landlock_append_fs_rule(). > > Working on these changes, the location of these transformations is one of the > last outstanding problems that I don't like yet. > > I have added the expansion code to landlock_add_fs_access_mask() and > landlock_append_fs_rule() as you suggested. > > This works, but as a result, this (somewhat complicated) expansion logic is now > part of the ruleset.o module, where it seems a bit too FS-specific. I think > that maybe we can pull this out further, but I'll probably send you a patch set > with the current status before doing that, so that we are on the same page. I guess we can put the expand functions in fs.c . But at that point we need an actual patch to discuss such details. > > > > Please base the next series on > > https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=next > > This branch might be rebased from time to time, but only minor changes > > will get there. > > OK, will do. > > > In summary, I'll send a patch soon. > > FYI, some open questions I still have are: > > * Logic > * How will userspace libraries handle best-effort fallback, > when expanded IOCTL access rights come into play? > (Still need to think about this more.) If users set the GFX right, the library should fallback to the IOCTL right if GFX is not supported. > * Internal code layout > * Move expansion logic out of ruleset.o module into syscalls.o? > * Find more appropriate names for IOCTL_CMD_G1,...,IOCTL_CMD_G4 Actually, I think these groups should be static const variables defined in the function that uses them, so the naming would not change much. Maybe something like ioctl_groupN? > > but we can discuss these in the context of the next patch set. Definitely > > —Günther
Hello! These patches add simple ioctl(2) support to Landlock. Objective ~~~~~~~~~ Make ioctl(2) requests restrictable with Landlock, in a way that is useful for real-world applications. Proposed approach ~~~~~~~~~~~~~~~~~ Introduce the LANDLOCK_ACCESS_FS_IOCTL right, which restricts the use of ioctl(2) on file descriptors. We attach the LANDLOCK_ACCESS_FS_IOCTL right to opened file descriptors, as we already do for LANDLOCK_ACCESS_FS_TRUNCATE. We make an exception for the common and known-harmless IOCTL commands FIOCLEX, FIONCLEX, FIONBIO, FIOASYNC and FIONREAD. These IOCTL commands are always permitted. The functionality of the first four is already available through fcntl(2), and FIONREAD only returns the number of ready-to-read bytes. I believe that this approach works for the majority of use cases, and offers a good trade-off between Landlock API and implementation complexity and flexibility when the feature is used. Current limitations ~~~~~~~~~~~~~~~~~~~ With this patch set, ioctl(2) requests can *not* be filtered based on file type, device number (dev_t) or on the ioctl(2) request number. On the initial RFC patch set [1], we have reached consensus to start with this simpler coarse-grained approach, and build additional IOCTL restriction capabilities on top in subsequent steps. [1] https://lore.kernel.org/linux-security-module/d4f1395c-d2d4-1860-3a02-2a0c023dd761@digikod.net/ Notable implications of this approach ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ * Existing inherited file descriptors stay unaffected when a program enables Landlock. This means in particular that in common scenarios, the terminal's IOCTLs (ioctl_tty(2)) continue to work. * ioctl(2) continues to be available for file descriptors acquired through means other than open(2). Example: Network sockets, memfd_create(2), file descriptors that are already open before the Landlock ruleset is enabled. Examples ~~~~~~~~ Starting a sandboxed shell from $HOME with samples/landlock/sandboxer: LL_FS_RO=/ LL_FS_RW=. ./sandboxer /bin/bash The LANDLOCK_ACCESS_FS_IOCTL right is part of the "read-write" rights here, so we expect that newly opened files outside of $HOME don't work with ioctl(2). * "stty" works: It probes terminal properties * "stty </dev/tty" fails: /dev/tty can be reopened, but the IOCTL is denied. * "eject" fails: ioctls to use CD-ROM drive are denied. * "ls /dev" works: It uses ioctl to get the terminal size for columnar layout * The text editors "vim" and "mg" work. (GNU Emacs fails because it attempts to reopen /dev/tty.) How we arrived at the list of always-permitted IOCTL commands ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ To decide which IOCTL commands should be blanket-permitted I went through the list of IOCTL commands mentioned in fs/ioctl.c and looked at them individually to understand what they are about. The following list is my conclusion from that. We should always allow the following IOCTL commands: * FIOCLEX, FIONCLEX - these work on the file descriptor and manipulate the close-on-exec flag * FIONBIO, FIOASYNC - these work on the struct file and enable nonblocking-IO and async flags * FIONREAD - get the number of bytes available for reading (the implementation is defined per file type) The first four are also available through fcntl with the F_SETFD and F_SETFL commands. The following commands mentioned in fs/ioctl.c should be guarded by the LANDLOCK_ACCESS_FS_IOCTL access right, the same as the other ioctl commands, because they are nontrivial: * FIFREEZE, FITHAW - work on superblock(!) to freeze/thaw the file system. Requires CAP_SYS_ADMIN. * FICLONE, FICLONERANGE, FIDEDUPRANGE - making files share physical storage between multiple files. These only work on some file systems, by design. * Commands that read file system internals: * FS_IOC_FIEMAP - get information about file extent mapping (c.f. https://www.kernel.org/doc/Documentation/filesystems/fiemap.txt) * FIBMAP - get a file's file system block number * FIGETBSZ - get file system blocksize * Accessing file attributes: * FS_IOC_GETFLAGS, FS_IOC_SETFLAGS - manipulate inode flags (ioctl_iflags(2)) * FS_IOC_FSGETXATTR, FS_IOC_FSSETXATTR - more attributes * FS_IOC_RESVSP, FS_IOC_RESVSP64, FS_IOC_UNRESVSP, FS_IOC_UNRESVSP64, FS_IOC_ZERO_RANGE: Backwards compatibility with legacy XFS preallocation syscalls which predate fallocate(2). Related Work ~~~~~~~~~~~~ OpenBSD's pledge(2) [2] restricts ioctl(2) independent of the file descriptor which is used. The implementers maintain multiple allow-lists of predefined ioctl(2) operations required for different application domains such as "audio", "bpf", "tty" and "inet". OpenBSD does not guarantee ABI backwards compatibility to the same extent as Linux does, so it's easier for them to update these lists in later versions. It might not be a feasible approach for Linux though. [2] https://man.openbsd.org/OpenBSD-7.3/pledge.2 Changes ~~~~~~~ V3: * always permit the IOCTL commands FIOCLEX, FIONCLEX, FIONBIO, FIOASYNC and FIONREAD, independent of LANDLOCK_ACCESS_FS_IOCTL * increment ABI version in the same commit where the feature is introduced * testing changes * use FIOQSIZE instead of TTY IOCTL commands (FIOQSIZE works with regular files, directories and memfds) * run the memfd test with both Landlock enabled and disabled * add a test for the always-permitted IOCTL commands V2: * rebased on mic-next * added documentation * exercise ioctl(2) in the memfd test * test: Use layout0 for the test --- V1: https://lore.kernel.org/linux-security-module/20230502171755.9788-1-gnoack3000@gmail.com/ V2: https://lore.kernel.org/linux-security-module/20230623144329.136541-1-gnoack@google.com/ Günther Noack (5): landlock: Add ioctl access right selftests/landlock: Test ioctl support selftests/landlock: Test ioctl with memfds samples/landlock: Add support for LANDLOCK_ACCESS_FS_IOCTL landlock: Document ioctl support Documentation/userspace-api/landlock.rst | 74 ++++++++--- include/uapi/linux/landlock.h | 31 +++-- samples/landlock/sandboxer.c | 12 +- security/landlock/fs.c | 38 +++++- 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 | 133 +++++++++++++++++-- 8 files changed, 249 insertions(+), 45 deletions(-) base-commit: 35ca4239929737bdc021ee923f97ebe7aff8fcc4