[PATCH v1] landlock: Explain file descriptor access rights

[Günther Noack]

Hi!

Thanks for sending this patch! I have overlooked to update this in the
original patch set.

On Mon, Dec 05, 2022 at 12:26:21PM +0100, Mickaël Salaün wrote:
> Starting with LANDLOCK_ACCESS_FS_TRUNCATE, it is worth explaining why we
> choose to restrict access checks at open time.  This new "File
> descriptor access rights" section is complementary to the existing
> "Inode access rights" section.
> 
> Cc: Günther Noack <gnoack3000 at gmail.com>
> Signed-off-by: Mickaël Salaün <mic at digikod.net>
> Link: https://lore.kernel.org/r/20221205112621.3530557-1-mic@digikod.net
> ---
>  Documentation/security/landlock.rst | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/security/landlock.rst b/Documentation/security/landlock.rst
> index c0029d5d02eb..bd0af6031ebb 100644
> --- a/Documentation/security/landlock.rst
> +++ b/Documentation/security/landlock.rst
> @@ -7,7 +7,7 @@ Landlock LSM: kernel documentation
>  ==================================
>  
>  :Author: Mickaël Salaün
> -:Date: September 2022
> +:Date: November 2022
>  
>  Landlock's goal is to create scoped access-control (i.e. sandboxing).  To
>  harden a whole system, this feature should be available to any process,
> @@ -45,8 +45,8 @@ Guiding principles for safe access controls
>  Design choices
>  ==============
>  
> -Filesystem access rights
> -------------------------
> +Inode access rights
> +-------------------
>  
>  All access rights are tied to an inode and what can be accessed through it.
>  Reading the content of a directory does not imply to be allowed to read the
> @@ -57,6 +57,25 @@ directory, not the unlinked inode.  This is the reason why
>  ``LANDLOCK_ACCESS_FS_REMOVE_FILE`` or ``LANDLOCK_ACCESS_FS_REFER`` are not
>  allowed to be tied to files but only to directories.
>  
> +File descriptor access rights
> +-----------------------------
> +
> +Access rights are checked and tied to file descriptors at open time.  As a
> +consequence, it may be allowed to create a file without being allowed to
> +truncate it if the file hierarchy doesn't grant such access right.  The
> +rationale is that this approach is simple and consistent with all access
> +rights, however file requests are based on path or based on file descriptor
> +(obtained with the same path, by a thread restricted with the same Landlock
> +domain).  For instance, updating an application from using :manpage:`mknod` and
> +:manpage:`truncate` to initialize a file (i.e. path-based), to using
> +:manpage:`open` and :manpage:`ftruncate` to initialize the same file (i.e. file
> +descriptor-based) should work the same way with the same Landlock restrictions.

Nit: The paragraph seems a bit long and is mixing more general
considerations with examples. Maybe they could be split into separate
paragraphs?

Regarding the "As a consequence..." example: I would word it as "...it
may be allowed to open a file for writing without being allowed to
ftruncate the resulting file descriptor...".

The example you are giving with creating files is also accurate, but
it is potentially confusing, because creat() and open(..., O_TRUNC)
are also implicitly truncating files when opening the file.

Regarding "The rationale is that this approach is simple and
consistent...": The word "simple" is often a sign that we could be
more concrete, and there is also a risk that a reader might not
perceive it as simple ;)  How about this:

```

The rationale is that equivalent sequences of operations should lead
to the same results, when they are executed under the same Landlock
domain.

For example, for the LANDLOCK_ACCESS_FS_TRUNCATE right, the following
sequences of operations are roughly equivalent and should have the
same result:

* truncate(path)
* fd = open(path, O_WRONLY); ftruncate(fd); close(fd)

```

(I think it's a bit more readable with bullet points, and the
truncate/ftruncate example might be a bit more familiar than the
somewhat unusual mknod?)

> +
> +Processes not sandboxed by Landlock may still be restricted for operations on
> +file descriptors coming from a sandboxed process.  Indeed, this is required to
> +keep a consistent access control over the whole system, and avoid unattended
> +bypasses through file descriptor passing (i.e. confused deputy attack).

"May still be restricted" is leaving the reader a bit in the dark
about the exact circumstances where this might happen? I think we
could be more bold and give a guarantee here, like:

```

Access rights attached to file descriptors are retained even if the
file descriptor is passed between Unix processes (e.g. through a Unix
Domain Socket). The access rights will be enforced even if the
receiving process is not sandboxed by Landlock.

```

--Günther

> +
>  Tests
>  =====
>  
> 
> base-commit: 0b4ab8cd635e8b21e42c14b9e4810ca701babd11
> -- 
> 2.38.1
> 

--