[PATCH] landlock: Clarify documentation for the LANDLOCK_ACCESS_FS_REFER right

[PATCH] landlock: Clarify documentation for the LANDLOCK_ACCESS_FS_REFER right

[Günther Noack]

Clarify the "refer" documentation by splitting up a big paragraph of text.

- Call out specifically that the denial by default applies to ABI v1 as well.
- Turn the three additional constraints for link/rename operations
  into bullet points, to give it more structure.

Signed-off-by: Günther Noack <gnoack3000@gmail.com>
---
 include/uapi/linux/landlock.h | 41 ++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index f3223f96469..0cc58e0361f 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -130,21 +130,32 @@ struct landlock_path_beneath_attr {
  * - %LANDLOCK_ACCESS_FS_MAKE_BLOCK: Create (or rename or link) a block device.
  * - %LANDLOCK_ACCESS_FS_MAKE_SYM: Create (or rename or link) a symbolic link.
  * - %LANDLOCK_ACCESS_FS_REFER: Link or rename a file from or to a different
- *   directory (i.e. reparent a file hierarchy).  This access right is
- *   available since the second version of the Landlock ABI.  This is also the
- *   only access right which is always considered handled by any ruleset in
- *   such a way that reparenting a file hierarchy is always denied by default.
- *   To avoid privilege escalation, it is not enough to add a rule with this
- *   access right.  When linking or renaming a file, the destination directory
- *   hierarchy must also always have the same or a superset of restrictions of
- *   the source hierarchy.  If it is not the case, or if the domain doesn't
- *   handle this access right, such actions are denied by default with errno
- *   set to ``EXDEV``.  Linking also requires a ``LANDLOCK_ACCESS_FS_MAKE_*``
- *   access right on the destination directory, and renaming also requires a
- *   ``LANDLOCK_ACCESS_FS_REMOVE_*`` access right on the source's (file or
- *   directory) parent.  Otherwise, such actions are denied with errno set to
- *   ``EACCES``.  The ``EACCES`` errno prevails over ``EXDEV`` to let user space
- *   efficiently deal with an unrecoverable error.
+ *   directory (i.e. reparent a file hierarchy).
+ *
+ *   This access right is available since the second version of the Landlock
+ *   ABI.  This is also the only access right which is always considered
+ *   handled by any ruleset in such a way that reparenting a file hierarchy is
+ *   always denied by default.  If left unspecified during the creation of a
+ *   ruleset, linking and renaming files between different directories will be
+ *   forbidden, also when done on a kernel using Landlock ABI v1.
+ *
+ *   In addition to permitting this access right, the following constraints
+ *   must hold for the access rights on the source and destination directory:
+ *
+ *   * The destination directory may not grant any access rights which are
+ *     forbidden in the source directory.  Otherwise, the operation results in
+ *     an ``EXDEV`` error.
+ *
+ *   * When linking or renaming, the ``LANDLOCK_ACCESS_FS_MAKE_*`` right for
+ *     the respective file type is required in the destination directory.
+ *     Otherwise, the operation results in an ``EACCES`` error.
+ *
+ *   * When renaming, the ``LANDLOCK_ACCESS_FS_REMOVE_*`` right for the
+ *     respective file type is required in the source directory.  Otherwise,
+ *     the operation results in an ``EACCES`` error.
+ *
+ *   If multiple requirements are not met, the ``EACCES`` error code takes
+ *   precedence over ``EXDEV``.
  *
  * .. warning::
  *

base-commit: 6d796c50f84ca79f1722bb131799e5a5710c4700
-- 
2.39.1

Re: [PATCH] landlock: Clarify documentation for the LANDLOCK_ACCESS_FS_REFER right

[Mickaël Salaün]

Thanks for the doc improvement Günther!

On 02/02/2023 21:46, Günther Noack wrote:
> Clarify the "refer" documentation by splitting up a big paragraph of text.
> 
> - Call out specifically that the denial by default applies to ABI v1 as well.
> - Turn the three additional constraints for link/rename operations
>    into bullet points, to give it more structure.
> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>   include/uapi/linux/landlock.h | 41 ++++++++++++++++++++++-------------
>   1 file changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index f3223f96469..0cc58e0361f 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -130,21 +130,32 @@ struct landlock_path_beneath_attr {
>    * - %LANDLOCK_ACCESS_FS_MAKE_BLOCK: Create (or rename or link) a block device.
>    * - %LANDLOCK_ACCESS_FS_MAKE_SYM: Create (or rename or link) a symbolic link.
>    * - %LANDLOCK_ACCESS_FS_REFER: Link or rename a file from or to a different
> - *   directory (i.e. reparent a file hierarchy).  This access right is
> - *   available since the second version of the Landlock ABI.  This is also the
> - *   only access right which is always considered handled by any ruleset in
> - *   such a way that reparenting a file hierarchy is always denied by default.
> - *   To avoid privilege escalation, it is not enough to add a rule with this
> - *   access right.  When linking or renaming a file, the destination directory
> - *   hierarchy must also always have the same or a superset of restrictions of
> - *   the source hierarchy.  If it is not the case, or if the domain doesn't
> - *   handle this access right, such actions are denied by default with errno
> - *   set to ``EXDEV``.  Linking also requires a ``LANDLOCK_ACCESS_FS_MAKE_*``
> - *   access right on the destination directory, and renaming also requires a
> - *   ``LANDLOCK_ACCESS_FS_REMOVE_*`` access right on the source's (file or
> - *   directory) parent.  Otherwise, such actions are denied with errno set to
> - *   ``EACCES``.  The ``EACCES`` errno prevails over ``EXDEV`` to let user space
> - *   efficiently deal with an unrecoverable error.
> + *   directory (i.e. reparent a file hierarchy).
> + *
> + *   This access right is available since the second version of the Landlock
> + *   ABI.  This is also the only access right which is always considered
> + *   handled by any ruleset in such a way that reparenting a file hierarchy is
> + *   always denied by default.  If left unspecified during the creation of a
> + *   ruleset, linking and renaming files between different directories will be
> + *   forbidden, also when done on a kernel using Landlock ABI v1.

What about:
forbidden, which is also the case when using the first Landlock ABI.


> + *
> + *   In addition to permitting this access right, the following constraints
> + *   must hold for the access rights on the source and destination directory:
> + *
> + *   * The destination directory may not grant any access rights which are
> + *     forbidden in the source directory.  Otherwise, the operation results in

"forbidden to the source file."

Indeed, the check is done according to the source file and compared to 
the potential destination file. For instance, the parent source 
directory may not allow to execute the source file, but the link will be 
allowed even if the destination directory allows file execution in the 
case of the execution right being tied to the (linked) file itself.


> + *     an ``EXDEV`` error.
> + *
> + *   * When linking or renaming, the ``LANDLOCK_ACCESS_FS_MAKE_*`` right for
> + *     the respective file type is required in the destination directory.
> + *     Otherwise, the operation results in an ``EACCES`` error.
> + *
> + *   * When renaming, the ``LANDLOCK_ACCESS_FS_REMOVE_*`` right for the
> + *     respective file type is required in the source directory.  Otherwise,
> + *     the operation results in an ``EACCES`` error.

There is also the RENAME_EXCHANGE case when we need make and remove 
access rights in the source and in the destination directory, but it 
would probably confuse readers to specify that.


> + *
> + *   If multiple requirements are not met, the ``EACCES`` error code takes
> + *   precedence over ``EXDEV``.
>    *
>    * .. warning::
>    *
> 
> base-commit: 6d796c50f84ca79f1722bb131799e5a5710c4700

Re: [PATCH] landlock: Clarify documentation for the LANDLOCK_ACCESS_FS_REFER right

[Günther Noack]

Hello Mickaël!

On Mon, Feb 13, 2023 at 08:14:27PM +0100, Mickaël Salaün wrote:
> Thanks for the doc improvement Günther!
> 
> On 02/02/2023 21:46, Günther Noack wrote:
> > Clarify the "refer" documentation by splitting up a big paragraph of text.
> > 
> > - Call out specifically that the denial by default applies to ABI v1 as well.
> > - Turn the three additional constraints for link/rename operations
> >    into bullet points, to give it more structure.
> > 
> > Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> > ---
> >   include/uapi/linux/landlock.h | 41 ++++++++++++++++++++++-------------
> >   1 file changed, 26 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > index f3223f96469..0cc58e0361f 100644
> > --- a/include/uapi/linux/landlock.h
> > +++ b/include/uapi/linux/landlock.h
> > @@ -130,21 +130,32 @@ struct landlock_path_beneath_attr {
> > + *   This access right is available since the second version of the Landlock
> > + *   ABI.  This is also the only access right which is always considered
> > + *   handled by any ruleset in such a way that reparenting a file hierarchy is
> > + *   always denied by default.  If left unspecified during the creation of a
> > + *   ruleset, linking and renaming files between different directories will be
> > + *   forbidden, also when done on a kernel using Landlock ABI v1.
> 
> What about:
> forbidden, which is also the case when using the first Landlock ABI.

Applied, makes sense.

> > + *   In addition to permitting this access right, the following constraints
> > + *   must hold for the access rights on the source and destination directory:
> > + *
> > + *   * The destination directory may not grant any access rights which are
> > + *     forbidden in the source directory.  Otherwise, the operation results in
> 
> "forbidden to the source file."
> 
> Indeed, the check is done according to the source file and compared to the
> potential destination file. For instance, the parent source directory may
> not allow to execute the source file, but the link will be allowed even if
> the destination directory allows file execution in the case of the execution
> right being tied to the (linked) file itself.

Ah, I didn't notice that, thanks for pointing it out! Changed.

> > + *     an ``EXDEV`` error.
> > + *
> > + *   * When linking or renaming, the ``LANDLOCK_ACCESS_FS_MAKE_*`` right for
> > + *     the respective file type is required in the destination directory.
> > + *     Otherwise, the operation results in an ``EACCES`` error.
> > + *
> > + *   * When renaming, the ``LANDLOCK_ACCESS_FS_REMOVE_*`` right for the
> > + *     respective file type is required in the source directory.  Otherwise,
> > + *     the operation results in an ``EACCES`` error.
> 
> There is also the RENAME_EXCHANGE case when we need make and remove access
> rights in the source and in the destination directory, but it would probably
> confuse readers to specify that.

I'll leave it out for now, to keep this patch focused on rewording. I
feel that adding it now would open another can of worms better left
for a more dedicated code review. ;)

–-Günther