| Message ID | 20231208155121.1943775-10-gnoack@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Landlock: IOCTL support | expand |
On Fri, Dec 08, 2023 at 04:51:21PM +0100, Günther Noack wrote: > In the paragraph above the fallback logic, use the shorter phrasing > from the landlock(7) man page. > > Signed-off-by: Günther Noack <gnoack@google.com> > --- > Documentation/userspace-api/landlock.rst | 119 ++++++++++++++++++++--- > 1 file changed, 104 insertions(+), 15 deletions(-) > > +Restricting IOCTL commands > +-------------------------- > + > +When the ``LANDLOCK_ACCESS_FS_IOCTL`` access right is handled, Landlock will > +restrict the invocation of IOCTL commands. However, to *permit* these IOCTL > +commands again, some of these IOCTL commands are then granted through other, > +preexisting access rights. > + > +For example, consider a program which handles ``LANDLOCK_ACCESS_FS_IOCTL`` and > +``LANDLOCK_ACCESS_FS_READ_FILE``. The program *permits* > +``LANDLOCK_ACCESS_FS_READ_FILE`` on a file ``foo.log``. > + > +By virtue of granting this access on the ``foo.log`` file, it is now possible to > +use common and harmless IOCTL commands which are useful when reading files, such > +as ``FIONREAD``. > + > +On the other hand, if the program permits ``LANDLOCK_ACCESS_FS_IOCTL`` on > +another file, ``FIONREAD`` will not work on that file when it is opened. As > +soon as ``LANDLOCK_ACCESS_FS_READ_FILE`` is *handled* in the ruleset, the IOCTL > +commands affected by it can not be reenabled though ``LANDLOCK_ACCESS_FS_IOCTL`` > +any more, but are then governed by ``LANDLOCK_ACCESS_FS_READ_FILE``. > + > +The following table illustrates how IOCTL attempts for ``FIONREAD`` are > +filtered, depending on how a Landlock ruleset handles and permits the > +``LANDLOCK_ACCESS_FS_IOCTL`` and ``LANDLOCK_ACCESS_FS_READ_FILE`` access rights: > + > ++------------------------+-------------+-------------------+-------------------+ > +| | ``IOCTL`` | ``IOCTL`` handled | ``IOCTL`` handled | > +| | not handled | and permitted | and not permitted | > ++------------------------+-------------+-------------------+-------------------+ > +| ``READ_FILE`` not | allow | allow | deny | > +| handled | | | | > ++------------------------+ +-------------------+-------------------+ > +| ``READ_FILE`` handled | | allow | > +| and permitted | | | > ++------------------------+ +-------------------+-------------------+ > +| ``READ_FILE`` handled | | deny | > +| and not permitted | | | > ++------------------------+-------------+-------------------+-------------------+ Great! Could you please format this table with the flat-table syntax? See https://docs.kernel.org/doc-guide/sphinx.html#tables > + > +The full list of IOCTL commands and the access rights which affect them is > +documented below.
Hello Mickaël! Thanks for the review! On Mon, Dec 11, 2023 at 08:04:33AM +0100, Mickaël Salaün wrote: > On Fri, Dec 08, 2023 at 04:51:21PM +0100, Günther Noack wrote: > > ++------------------------+-------------+-------------------+-------------------+ > > +| | ``IOCTL`` | ``IOCTL`` handled | ``IOCTL`` handled | > > +| | not handled | and permitted | and not permitted | > > ++------------------------+-------------+-------------------+-------------------+ > > +| ``READ_FILE`` not | allow | allow | deny | > > +| handled | | | | > > ++------------------------+ +-------------------+-------------------+ > > +| ``READ_FILE`` handled | | allow | > > +| and permitted | | | > > ++------------------------+ +-------------------+-------------------+ > > +| ``READ_FILE`` handled | | deny | > > +| and not permitted | | | > > ++------------------------+-------------+-------------------+-------------------+ > > Great! Could you please format this table with the flat-table syntax? > See https://docs.kernel.org/doc-guide/sphinx.html#tables This link actually says that “Kernel style for tables is to prefer simple table syntax or grid table syntax” (instead of the flat-table syntax). This "visual" style is more cumbersome to edit, but editing documentation happens less than reading it, so further edits are less likely. I also find it easier to reason about what the cell sizes are that way, rather than having to wrap my head around special :rspan: and :cspan: syntax. If you are not strongly opposed to it, I'd prefer to keep the existing style, but we can do it either way if you feel strongly about it. Let me know how important this is to you. Thanks, —Günther
On Mon, Dec 11, 2023 at 09:49:14AM +0100, Günther Noack wrote: > Hello Mickaël! > > Thanks for the review! > > On Mon, Dec 11, 2023 at 08:04:33AM +0100, Mickaël Salaün wrote: > > On Fri, Dec 08, 2023 at 04:51:21PM +0100, Günther Noack wrote: > > > ++------------------------+-------------+-------------------+-------------------+ > > > +| | ``IOCTL`` | ``IOCTL`` handled | ``IOCTL`` handled | > > > +| | not handled | and permitted | and not permitted | > > > ++------------------------+-------------+-------------------+-------------------+ > > > +| ``READ_FILE`` not | allow | allow | deny | > > > +| handled | | | | > > > ++------------------------+ +-------------------+-------------------+ > > > +| ``READ_FILE`` handled | | allow | > > > +| and permitted | | | > > > ++------------------------+ +-------------------+-------------------+ > > > +| ``READ_FILE`` handled | | deny | > > > +| and not permitted | | | > > > ++------------------------+-------------+-------------------+-------------------+ > > > > Great! Could you please format this table with the flat-table syntax? > > See https://docs.kernel.org/doc-guide/sphinx.html#tables > > This link actually says that “Kernel style for tables is to prefer simple table > syntax or grid table syntax” (instead of the flat-table syntax). > > This "visual" style is more cumbersome to edit, but editing documentation > happens less than reading it, so further edits are less likely. I also find it > easier to reason about what the cell sizes are that way, rather than having to > wrap my head around special :rspan: and :cspan: syntax. Indeed, let's keep this ascii art. > > If you are not strongly opposed to it, I'd prefer to keep the existing style, > but we can do it either way if you feel strongly about it. Let me know how > important this is to you. > > Thanks, > —Günther >
On Fri, Dec 08, 2023 at 04:51:21PM +0100, Günther Noack wrote: > In the paragraph above the fallback logic, use the shorter phrasing > from the landlock(7) man page. > > Signed-off-by: Günther Noack <gnoack@google.com> > --- > Documentation/userspace-api/landlock.rst | 119 ++++++++++++++++++++--- > 1 file changed, 104 insertions(+), 15 deletions(-) > > +Restricting IOCTL commands > +-------------------------- > + > +When the ``LANDLOCK_ACCESS_FS_IOCTL`` access right is handled, Landlock will I only use "right" (instead of "access right") when LANDLOCK_ACCESS_* precede to avoid repetition. > +restrict the invocation of IOCTL commands. However, to *permit* these IOCTL This patch introduces the "permit*" wording instead of the currently used "allowed", which is inconsistent. > +commands again, some of these IOCTL commands are then granted through other, > +preexisting access rights. > + > +For example, consider a program which handles ``LANDLOCK_ACCESS_FS_IOCTL`` and > +``LANDLOCK_ACCESS_FS_READ_FILE``. The program *permits* > +``LANDLOCK_ACCESS_FS_READ_FILE`` on a file ``foo.log``. > + > +By virtue of granting this access on the ``foo.log`` file, it is now possible to > +use common and harmless IOCTL commands which are useful when reading files, such > +as ``FIONREAD``. > + > +On the other hand, if the program permits ``LANDLOCK_ACCESS_FS_IOCTL`` on > +another file, ``FIONREAD`` will not work on that file when it is opened. As > +soon as ``LANDLOCK_ACCESS_FS_READ_FILE`` is *handled* in the ruleset, the IOCTL > +commands affected by it can not be reenabled though ``LANDLOCK_ACCESS_FS_IOCTL`` > +any more, but are then governed by ``LANDLOCK_ACCESS_FS_READ_FILE``. > + > +The following table illustrates how IOCTL attempts for ``FIONREAD`` are > +filtered, depending on how a Landlock ruleset handles and permits the > +``LANDLOCK_ACCESS_FS_IOCTL`` and ``LANDLOCK_ACCESS_FS_READ_FILE`` access rights: > + > ++------------------------+-------------+-------------------+-------------------+ > +| | ``IOCTL`` | ``IOCTL`` handled | ``IOCTL`` handled | I was a bit confused at first read, wondering why IOCTL was quoted, then I realized that it was in fact LANDLOCK_ACCESS_FS_IOCTL. Maybe using the "FS_" prefix would avoid this kind of misreading (same for READ_FILE)? > +| | not handled | and permitted | and not permitted | > ++------------------------+-------------+-------------------+-------------------+ > +| ``READ_FILE`` not | allow | allow | deny | > +| handled | | | | > ++------------------------+ +-------------------+-------------------+ > +| ``READ_FILE`` handled | | allow | > +| and permitted | | | > ++------------------------+ +-------------------+-------------------+ > +| ``READ_FILE`` handled | | deny | > +| and not permitted | | | If it makes the raw text easier to read, it should be OK to extend this table to 100 columns (I guess checkpatch.pl will not complain). > ++------------------------+-------------+-------------------+-------------------+ > + > +The full list of IOCTL commands and the access rights which affect them is > +documented below. > > Compatibility > ============= > @@ -457,6 +514,28 @@ Memory usage > Kernel memory allocated to create rulesets is accounted and can be restricted > by the Documentation/admin-guide/cgroup-v1/memory.rst. > > +IOCTL support > +------------- > + > +The ``LANDLOCK_ACCESS_FS_IOCTL`` access right restricts the use of > +:manpage:`ioctl(2)`, but it only applies to newly opened files. This means > +specifically that pre-existing file descriptors like stdin, stdout and stderr > +are unaffected. > + > +Users should be aware that TTY devices have traditionally permitted to control > +other processes on the same TTY through the ``TIOCSTI`` and ``TIOCLINUX`` IOCTL > +commands. It is therefore recommended to close inherited TTY file descriptors, > +or to reopen them from ``/proc/self/fd/*`` without the > +``LANDLOCK_ACCESS_FS_IOCTL`` right, if possible. The :manpage:`isatty(3)` > +function checks whether a given file descriptor is a TTY. > + > +Landlock's IOCTL support is coarse-grained at the moment, but may become more > +fine-grained in the future. Until then, users are advised to establish the > +guarantees that they need through the file hierarchy, by only permitting the > +``LANDLOCK_ACCESS_FS_IOCTL`` right on files where it is really harmless. In > +cases where you can control the mounts, the ``nodev`` mount option can help to > +rule out that device files can be accessed. > +
On Wed, Dec 13, 2023 at 12:25:15PM +0100, Mickaël Salaün wrote: > On Fri, Dec 08, 2023 at 04:51:21PM +0100, Günther Noack wrote: > > Documentation/userspace-api/landlock.rst | 119 ++++++++++++++++++++--- > > 1 file changed, 104 insertions(+), 15 deletions(-) > > > > > +Restricting IOCTL commands > > +-------------------------- > > + > > +When the ``LANDLOCK_ACCESS_FS_IOCTL`` access right is handled, Landlock will > > I only use "right" (instead of "access right") when LANDLOCK_ACCESS_* > precede to avoid repetition. Done. > > +restrict the invocation of IOCTL commands. However, to *permit* these IOCTL > > This patch introduces the "permit*" wording instead of the currently > used "allowed", which is inconsistent. Done. > > ++------------------------+-------------+-------------------+-------------------+ > > +| | ``IOCTL`` | ``IOCTL`` handled | ``IOCTL`` handled | > > I was a bit confused at first read, wondering why IOCTL was quoted, then > I realized that it was in fact LANDLOCK_ACCESS_FS_IOCTL. Maybe using the > "FS_" prefix would avoid this kind of misreading (same for READ_FILE)? Done. > > +| | not handled | and permitted | and not permitted | > > ++------------------------+-------------+-------------------+-------------------+ > > +| ``READ_FILE`` not | allow | allow | deny | > > +| handled | | | | > > ++------------------------+ +-------------------+-------------------+ > > +| ``READ_FILE`` handled | | allow | > > +| and permitted | | | > > ++------------------------+ +-------------------+-------------------+ > > +| ``READ_FILE`` handled | | deny | > > +| and not permitted | | | > > If it makes the raw text easier to read, it should be OK to extend this > table to 100 columns (I guess checkpatch.pl will not complain). I got it down to 72 columns and it still reads reasonably well. (Emacs has support for editing ASCII tables. :)) —Günther
diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst index 2e3822677061..8398851964e6 100644 --- a/Documentation/userspace-api/landlock.rst +++ b/Documentation/userspace-api/landlock.rst @@ -75,7 +75,8 @@ to be explicit about the denied-by-default access rights. LANDLOCK_ACCESS_FS_MAKE_BLOCK | LANDLOCK_ACCESS_FS_MAKE_SYM | LANDLOCK_ACCESS_FS_REFER | - LANDLOCK_ACCESS_FS_TRUNCATE, + LANDLOCK_ACCESS_FS_TRUNCATE | + LANDLOCK_ACCESS_FS_IOCTL, .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | LANDLOCK_ACCESS_NET_CONNECT_TCP, @@ -84,10 +85,10 @@ to be explicit about the denied-by-default access rights. Because we may not know on which kernel version an application will be executed, it is safer to follow a best-effort security approach. Indeed, we should try to protect users as much as possible whatever the kernel they are -using. To avoid binary enforcement (i.e. either all security features or -none), we can leverage a dedicated Landlock command to get the current version -of the Landlock ABI and adapt the handled accesses. Let's check if we should -remove access rights which are only supported in higher versions of the ABI. +using. + +To be compatible with older Linux versions, we detect the available Landlock ABI +version, and only use the available subset of access rights: .. code-block:: c @@ -113,6 +114,10 @@ remove access rights which are only supported in higher versions of the ABI. ruleset_attr.handled_access_net &= ~(LANDLOCK_ACCESS_NET_BIND_TCP | LANDLOCK_ACCESS_NET_CONNECT_TCP); + __attribute__((fallthrough)); + case 4: + /* Removes LANDLOCK_ACCESS_FS_IOCTL for ABI < 5 */ + ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL; } This enables to create an inclusive ruleset that will contain our rules. @@ -224,6 +229,7 @@ access rights per directory enables to change the location of such directory without relying on the destination directory access rights (except those that are required for this operation, see ``LANDLOCK_ACCESS_FS_REFER`` documentation). + Having self-sufficient hierarchies also helps to tighten the required access rights to the minimal set of data. This also helps avoid sinkhole directories, i.e. directories where data can be linked to but not linked from. However, @@ -317,18 +323,69 @@ It should also be noted that truncating files does not require the system call, this can also be done through :manpage:`open(2)` with the flags ``O_RDONLY | O_TRUNC``. -When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE`` -right is associated with the newly created file descriptor and will be used for -subsequent truncation attempts using :manpage:`ftruncate(2)`. The behavior is -similar to opening a file for reading or writing, where permissions are checked -during :manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and +The truncate right is associated with the opened file (see below). + +Rights associated with file descriptors +--------------------------------------- + +When opening a file, the availability of the ``LANDLOCK_ACCESS_FS_TRUNCATE`` and +``LANDLOCK_ACCESS_FS_IOCTL`` rights is associated with the newly created file +descriptor and will be used for subsequent truncation and ioctl attempts using +:manpage:`ftruncate(2)` and :manpage:`ioctl(2)`. The behavior is similar to +opening a file for reading or writing, where permissions are checked during +:manpage:`open(2)`, but not during the subsequent :manpage:`read(2)` and :manpage:`write(2)` calls. -As a consequence, it is possible to have multiple open file descriptors for the -same file, where one grants the right to truncate the file and the other does -not. It is also possible to pass such file descriptors between processes, -keeping their Landlock properties, even when these processes do not have an -enforced Landlock ruleset. +As a consequence, it is possible that a process has multiple open file +descriptors referring to the same file, but Landlock enforces different things +when operating with these file descriptors. This can happen when a Landlock +ruleset gets enforced and the process keeps file descriptors which were opened +both before and after the enforcement. It is also possible to pass such file +descriptors between processes, keeping their Landlock properties, even when some +of the involved processes do not have an enforced Landlock ruleset. + +Restricting IOCTL commands +-------------------------- + +When the ``LANDLOCK_ACCESS_FS_IOCTL`` access right is handled, Landlock will +restrict the invocation of IOCTL commands. However, to *permit* these IOCTL +commands again, some of these IOCTL commands are then granted through other, +preexisting access rights. + +For example, consider a program which handles ``LANDLOCK_ACCESS_FS_IOCTL`` and +``LANDLOCK_ACCESS_FS_READ_FILE``. The program *permits* +``LANDLOCK_ACCESS_FS_READ_FILE`` on a file ``foo.log``. + +By virtue of granting this access on the ``foo.log`` file, it is now possible to +use common and harmless IOCTL commands which are useful when reading files, such +as ``FIONREAD``. + +On the other hand, if the program permits ``LANDLOCK_ACCESS_FS_IOCTL`` on +another file, ``FIONREAD`` will not work on that file when it is opened. As +soon as ``LANDLOCK_ACCESS_FS_READ_FILE`` is *handled* in the ruleset, the IOCTL +commands affected by it can not be reenabled though ``LANDLOCK_ACCESS_FS_IOCTL`` +any more, but are then governed by ``LANDLOCK_ACCESS_FS_READ_FILE``. + +The following table illustrates how IOCTL attempts for ``FIONREAD`` are +filtered, depending on how a Landlock ruleset handles and permits the +``LANDLOCK_ACCESS_FS_IOCTL`` and ``LANDLOCK_ACCESS_FS_READ_FILE`` access rights: + ++------------------------+-------------+-------------------+-------------------+ +| | ``IOCTL`` | ``IOCTL`` handled | ``IOCTL`` handled | +| | not handled | and permitted | and not permitted | ++------------------------+-------------+-------------------+-------------------+ +| ``READ_FILE`` not | allow | allow | deny | +| handled | | | | ++------------------------+ +-------------------+-------------------+ +| ``READ_FILE`` handled | | allow | +| and permitted | | | ++------------------------+ +-------------------+-------------------+ +| ``READ_FILE`` handled | | deny | +| and not permitted | | | ++------------------------+-------------+-------------------+-------------------+ + +The full list of IOCTL commands and the access rights which affect them is +documented below. Compatibility ============= @@ -457,6 +514,28 @@ Memory usage Kernel memory allocated to create rulesets is accounted and can be restricted by the Documentation/admin-guide/cgroup-v1/memory.rst. +IOCTL support +------------- + +The ``LANDLOCK_ACCESS_FS_IOCTL`` access right restricts the use of +:manpage:`ioctl(2)`, but it only applies to newly opened files. This means +specifically that pre-existing file descriptors like stdin, stdout and stderr +are unaffected. + +Users should be aware that TTY devices have traditionally permitted to control +other processes on the same TTY through the ``TIOCSTI`` and ``TIOCLINUX`` IOCTL +commands. It is therefore recommended to close inherited TTY file descriptors, +or to reopen them from ``/proc/self/fd/*`` without the +``LANDLOCK_ACCESS_FS_IOCTL`` right, if possible. The :manpage:`isatty(3)` +function checks whether a given file descriptor is a TTY. + +Landlock's IOCTL support is coarse-grained at the moment, but may become more +fine-grained in the future. Until then, users are advised to establish the +guarantees that they need through the file hierarchy, by only permitting the +``LANDLOCK_ACCESS_FS_IOCTL`` right on files where it is really harmless. In +cases where you can control the mounts, the ``nodev`` mount option can help to +rule out that device files can be accessed. + Previous limitations ==================== @@ -494,6 +573,16 @@ bind and connect actions to only a set of allowed ports thanks to the new ``LANDLOCK_ACCESS_NET_BIND_TCP`` and ``LANDLOCK_ACCESS_NET_CONNECT_TCP`` access rights. +IOCTL (ABI < 5) +--------------- + +IOCTL operations could not be denied before the fifth Landlock ABI, so +:manpage:`ioctl(2)` is always allowed when using a kernel that only supports an +earlier ABI. + +Starting with the Landlock ABI version 5, it is possible to restrict the use of +:manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL`` access right. + .. _kernel_support: Kernel support
In the paragraph above the fallback logic, use the shorter phrasing from the landlock(7) man page. Signed-off-by: Günther Noack <gnoack@google.com> --- Documentation/userspace-api/landlock.rst | 119 ++++++++++++++++++++--- 1 file changed, 104 insertions(+), 15 deletions(-)