diff mbox series

[v2] landlock: Clarify documentation for the LANDLOCK_ACCESS_FS_REFER right

Message ID 20230213210115.5150-1-gnoack3000@gmail.com (mailing list archive)
State Handled Elsewhere
Headers show
Series [v2] landlock: Clarify documentation for the LANDLOCK_ACCESS_FS_REFER right | expand

Commit Message

Günther Noack Feb. 13, 2023, 9:01 p.m. UTC
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.

Includes wording and semantics corrections by Mickaël Salaün.

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


base-commit: ceaa837f96adb69c0df0397937cd74991d5d821a

Comments

Mickaël Salaün Feb. 14, 2023, 12:04 p.m. UTC | #1
On 13/02/2023 22:01, 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.
> 
> Includes wording and semantics corrections by Mickaël Salaün.
> 
> 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..f6bccd87aa0 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

This is from me, but do you think "reparenting a file hierarchy" is not 
confusing? What about "reparenting a file or a directory"? Not sure 
which one is better.

I'm not sure either if we should use "deny" or "forbidden". Is there a 
difference? According to https://www.wikidiff.com/deny/forbid it seems 
that "deny" would be more appropriate because Landlock rules add 
exceptions to a forbidden set of actions… However, "deny" needs to be 
followed by "access" for the same use, which makes your wording correct. 
Just a thought.


> + *   always denied by default.  If left unspecified during the creation of a
> + *   ruleset, linking and renaming files between different directories will be
> + *   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 for the source file. Otherwise, the operation results in an

The files/directories don't grant accesses but the sandbox/domain do 
grant some accesses for a set of file hierarchies.

What about "Any forbidden actions on the source file must also be 
forbidden on the destination file."
Or "Any denied accesses on the source file…"

This seems a bit weird according to the previous "must hold for the 
access rights on the source and destination directory" though.


> + *     ``EXDEV`` error.
> + *
> + *   * When linking or renaming, the ``LANDLOCK_ACCESS_FS_MAKE_*`` right for
> + *     the respective file type is required in the destination directory.

s/is required in the destination/must be granted for the destination/ ?


> + *     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,

Same "must be granted for"


> + *     the operation results in an ``EACCES`` error.
> + *
> + *   If multiple requirements are not met, the ``EACCES`` error code takes
> + *   precedence over ``EXDEV``.
>    *
>    * .. warning::
>    *
> 
> base-commit: ceaa837f96adb69c0df0397937cd74991d5d821a
Günther Noack Feb. 15, 2023, 6:33 p.m. UTC | #2
Thanks for the feedback, Mickaël!

See some proposals for rephrasings inline. I tried to avoid passive
voice to make it easier to follow. Please let me know what you think.

(Any native English speakers are more than welcome to chime in as well. 8-))

–-Günther

On Tue, Feb 14, 2023 at 01:04:04PM +0100, Mickaël Salaün wrote:
> 
> On 13/02/2023 22:01, 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.
> > 
> > Includes wording and semantics corrections by Mickaël Salaün.
> > 
> > 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..f6bccd87aa0 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
> 
> This is from me, but do you think "reparenting a file hierarchy" is not
> confusing? What about "reparenting a file or a directory"? Not sure which
> one is better.

I find that sentence confusing as well, but the "reparenting a file"
part is not the confusing part to me.

Proposal for this paragraph:

  This access right is available since the second version of the
  Landlock.  This is also the only access right which is implicitly
  handled by any ruleset, even if the right is not specified at the
  time of creating the ruleset.  So by default, Landlock will deny
  linking and reparenting files between different directories, and
  only grant this right when it is explicitly permitted to a directory
  by adding a rule.

  When using the first Landlock ABI version, Landlock will always deny
  the reparenting of files between different directories.

> 
> I'm not sure either if we should use "deny" or "forbidden". Is there a
> difference? According to https://www.wikidiff.com/deny/forbid it seems that
> "deny" would be more appropriate because Landlock rules add exceptions to a
> forbidden set of actions… However, "deny" needs to be followed by "access"
> for the same use, which makes your wording correct. Just a thought.
> 
> 
> > + *   always denied by default.  If left unspecified during the creation of a
> > + *   ruleset, linking and renaming files between different directories will be
> > + *   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:

Proposal for this paragraph:

  In addition to the source and destination directories having the
  %LANDLOCK_ACCESS_FS_REFER access right, the attempted link or rename
  operation must meet the following constraints:

> > + *
> > + *   * The destination directory may not grant any access rights which are
> > + *     forbidden for the source file. Otherwise, the operation results in an
> 
> The files/directories don't grant accesses but the sandbox/domain do grant
> some accesses for a set of file hierarchies.
> 
> What about "Any forbidden actions on the source file must also be forbidden
> on the destination file."
> Or "Any denied accesses on the source file…"

Both valid points. How about the following phrasing which is
formulated a bit closer to the actual goal (not creating a loophole
through which you can gain more access rights for a file):

  * The reparented file may not attain more access rights in the
    destination directory than it previously had in the source
    directory.  If this is attempted, the operation results in an
    ``EXDEV`` error.

> This seems a bit weird according to the previous "must hold for the access
> rights on the source and destination directory" though.

+1, I reformulated that too above.

> 
> 
> > + *     ``EXDEV`` error.
> > + *
> > + *   * When linking or renaming, the ``LANDLOCK_ACCESS_FS_MAKE_*`` right for
> > + *     the respective file type is required in the destination directory.
> 
> s/is required in the destination/must be granted for the destination/ ?

Done.

> 
> 
> > + *     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,
> 
> Same "must be granted for"

Done.

> 
> 
> > + *     the operation results in an ``EACCES`` error.
> > + *
> > + *   If multiple requirements are not met, the ``EACCES`` error code takes
> > + *   precedence over ``EXDEV``.
> >    *
> >    * .. warning::
> >    *
> > 
> > base-commit: ceaa837f96adb69c0df0397937cd74991d5d821a
Günther Noack Feb. 15, 2023, 8:34 p.m. UTC | #3
On Wed, Feb 15, 2023 at 07:33:15PM +0100, Günther Noack wrote:
> Thanks for the feedback, Mickaël!
> 
> See some proposals for rephrasings inline. I tried to avoid passive
> voice to make it easier to follow. Please let me know what you think.
> 
> (Any native English speakers are more than welcome to chime in as well. 8-))
> 
> –-Günther
> 
> On Tue, Feb 14, 2023 at 01:04:04PM +0100, Mickaël Salaün wrote:
> > 
> > On 13/02/2023 22:01, 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.
> > > 
> > > Includes wording and semantics corrections by Mickaël Salaün.
> > > 
> > > 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..f6bccd87aa0 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
> > 
> > This is from me, but do you think "reparenting a file hierarchy" is not
> > confusing? What about "reparenting a file or a directory"? Not sure which
> > one is better.
> 
> I find that sentence confusing as well, but the "reparenting a file"
> part is not the confusing part to me.
> 
> Proposal for this paragraph:
> 
>   This access right is available since the second version of the
>   Landlock.  This is also the only access right which is implicitly
>   handled by any ruleset, even if the right is not specified at the
>   time of creating the ruleset.  So by default, Landlock will deny
>   linking and reparenting files between different directories, and
>   only grant this right when it is explicitly permitted to a directory
>   by adding a rule.
> 
>   When using the first Landlock ABI version, Landlock will always deny
>   the reparenting of files between different directories.

Sorry, correction (+ABI, s/to/for/):

  This access right is available since the second version of the
  Landlock ABI.  This is also the only access right which is
  implicitly handled by any ruleset, even if the right is not
  specified at the time of creating the ruleset. So, by default,
  Landlock will deny linking and reparenting files between different
  directories, and only grant this right when it is explicitly
  permitted for a directory by adding a rule.
 
  When using the first Landlock ABI version, Landlock will always deny
  the reparenting of files between different directories.
> 
> > 
> > I'm not sure either if we should use "deny" or "forbidden". Is there a
> > difference? According to https://www.wikidiff.com/deny/forbid it seems that
> > "deny" would be more appropriate because Landlock rules add exceptions to a
> > forbidden set of actions… However, "deny" needs to be followed by "access"
> > for the same use, which makes your wording correct. Just a thought.
> > 
> > 
> > > + *   always denied by default.  If left unspecified during the creation of a
> > > + *   ruleset, linking and renaming files between different directories will be
> > > + *   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:
> 
> Proposal for this paragraph:
> 
>   In addition to the source and destination directories having the
>   %LANDLOCK_ACCESS_FS_REFER access right, the attempted link or rename
>   operation must meet the following constraints:
> 
> > > + *
> > > + *   * The destination directory may not grant any access rights which are
> > > + *     forbidden for the source file. Otherwise, the operation results in an
> > 
> > The files/directories don't grant accesses but the sandbox/domain do grant
> > some accesses for a set of file hierarchies.
> > 
> > What about "Any forbidden actions on the source file must also be forbidden
> > on the destination file."
> > Or "Any denied accesses on the source file…"
> 
> Both valid points. How about the following phrasing which is
> formulated a bit closer to the actual goal (not creating a loophole
> through which you can gain more access rights for a file):
> 
>   * The reparented file may not attain more access rights in the
>     destination directory than it previously had in the source
>     directory.  If this is attempted, the operation results in an
>     ``EXDEV`` error.
> 
> > This seems a bit weird according to the previous "must hold for the access
> > rights on the source and destination directory" though.
> 
> +1, I reformulated that too above.
> 
> > 
> > 
> > > + *     ``EXDEV`` error.
> > > + *
> > > + *   * When linking or renaming, the ``LANDLOCK_ACCESS_FS_MAKE_*`` right for
> > > + *     the respective file type is required in the destination directory.
> > 
> > s/is required in the destination/must be granted for the destination/ ?
> 
> Done.
> 
> > 
> > 
> > > + *     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,
> > 
> > Same "must be granted for"
> 
> Done.
> 
> > 
> > 
> > > + *     the operation results in an ``EACCES`` error.
> > > + *
> > > + *   If multiple requirements are not met, the ``EACCES`` error code takes
> > > + *   precedence over ``EXDEV``.
> > >    *
> > >    * .. warning::
> > >    *
> > > 
> > > base-commit: ceaa837f96adb69c0df0397937cd74991d5d821a
Mickaël Salaün Feb. 16, 2023, 6:42 p.m. UTC | #4
On 15/02/2023 21:34, Günther Noack wrote:
> On Wed, Feb 15, 2023 at 07:33:15PM +0100, Günther Noack wrote:
>> Thanks for the feedback, Mickaël!
>>
>> See some proposals for rephrasings inline. I tried to avoid passive
>> voice to make it easier to follow. Please let me know what you think.
>>
>> (Any native English speakers are more than welcome to chime in as well. 8-))

Alex, could you please get a look? It is plan for this update to also go 
to the man pages.

>>
>> –-Günther
>>
>> On Tue, Feb 14, 2023 at 01:04:04PM +0100, Mickaël Salaün wrote:
>>>
>>> On 13/02/2023 22:01, 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.
>>>>
>>>> Includes wording and semantics corrections by Mickaël Salaün.
>>>>
>>>> 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..f6bccd87aa0 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
>>>
>>> This is from me, but do you think "reparenting a file hierarchy" is not
>>> confusing? What about "reparenting a file or a directory"? Not sure which
>>> one is better.
>>
>> I find that sentence confusing as well, but the "reparenting a file"
>> part is not the confusing part to me.
>>
>> Proposal for this paragraph:
>>
>>    This access right is available since the second version of the
>>    Landlock.  This is also the only access right which is implicitly
>>    handled by any ruleset, even if the right is not specified at the
>>    time of creating the ruleset.  So by default, Landlock will deny
>>    linking and reparenting files between different directories, and
>>    only grant this right when it is explicitly permitted to a directory
>>    by adding a rule.
>>
>>    When using the first Landlock ABI version, Landlock will always deny
>>    the reparenting of files between different directories.
> 
> Sorry, correction (+ABI, s/to/for/):
> 
>    This access right is available since the second version of the
>    Landlock ABI.  This is also the only access right which is
>    implicitly handled by any ruleset, even if the right is not
>    specified at the time of creating the ruleset. So, by default,
>    Landlock will deny linking and reparenting files between different
>    directories, and only grant this right when it is explicitly

and will only grant…?

>    permitted for a directory by adding a rule.
>   
>    When using the first Landlock ABI version, Landlock will always deny
>    the reparenting of files between different directories.

Looks good to me!

>>
>>>
>>> I'm not sure either if we should use "deny" or "forbidden". Is there a
>>> difference? According to https://www.wikidiff.com/deny/forbid it seems that
>>> "deny" would be more appropriate because Landlock rules add exceptions to a
>>> forbidden set of actions… However, "deny" needs to be followed by "access"
>>> for the same use, which makes your wording correct. Just a thought.
>>>
>>>
>>>> + *   always denied by default.  If left unspecified during the creation of a
>>>> + *   ruleset, linking and renaming files between different directories will be
>>>> + *   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:
>>
>> Proposal for this paragraph:
>>
>>    In addition to the source and destination directories having the
>>    %LANDLOCK_ACCESS_FS_REFER access right, the attempted link or rename
>>    operation must meet the following constraints:
>>
>>>> + *
>>>> + *   * The destination directory may not grant any access rights which are
>>>> + *     forbidden for the source file. Otherwise, the operation results in an
>>>
>>> The files/directories don't grant accesses but the sandbox/domain do grant
>>> some accesses for a set of file hierarchies.
>>>
>>> What about "Any forbidden actions on the source file must also be forbidden
>>> on the destination file."
>>> Or "Any denied accesses on the source file…"
>>
>> Both valid points. How about the following phrasing which is
>> formulated a bit closer to the actual goal (not creating a loophole
>> through which you can gain more access rights for a file):
>>
>>    * The reparented file may not attain more access rights in the

s/may not/cannot/ ?
s/attain/gain/ ?

>>      destination directory than it previously had in the source
>>      directory.  If this is attempted, the operation results in an
>>      ``EXDEV`` error.

Better too!

This is becoming a bit difficult to follow, you can send a new patch 
with Alex in Cc. :)

>>
>>> This seems a bit weird according to the previous "must hold for the access
>>> rights on the source and destination directory" though.
>>
>> +1, I reformulated that too above.
>>
>>>
>>>
>>>> + *     ``EXDEV`` error.
>>>> + *
>>>> + *   * When linking or renaming, the ``LANDLOCK_ACCESS_FS_MAKE_*`` right for
>>>> + *     the respective file type is required in the destination directory.
>>>
>>> s/is required in the destination/must be granted for the destination/ ?
>>
>> Done.
>>
>>>
>>>
>>>> + *     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,
>>>
>>> Same "must be granted for"
>>
>> Done.
>>
>>>
>>>
>>>> + *     the operation results in an ``EACCES`` error.
>>>> + *
>>>> + *   If multiple requirements are not met, the ``EACCES`` error code takes
>>>> + *   precedence over ``EXDEV``.
>>>>     *
>>>>     * .. warning::
>>>>     *
>>>>
>>>> base-commit: ceaa837f96adb69c0df0397937cd74991d5d821a
Günther Noack Feb. 16, 2023, 7:48 p.m. UTC | #5
Hello!

On Thu, Feb 16, 2023 at 07:42:51PM +0100, Mickaël Salaün wrote:
> On 15/02/2023 21:34, Günther Noack wrote:
> > Sorry, correction (+ABI, s/to/for/):
> > 
> >    This access right is available since the second version of the
> >    Landlock ABI.  This is also the only access right which is
> >    implicitly handled by any ruleset, even if the right is not
> >    specified at the time of creating the ruleset. So, by default,
> >    Landlock will deny linking and reparenting files between different
> >    directories, and only grant this right when it is explicitly
> 
> and will only grant…?

Good point, done.

> > > Both valid points. How about the following phrasing which is
> > > formulated a bit closer to the actual goal (not creating a loophole
> > > through which you can gain more access rights for a file):
> > > 
> > >    * The reparented file may not attain more access rights in the
> 
> s/may not/cannot/ ?

I think "may not" is used when it's about permissions, whereas "can
not" is about ability. "may not" seems more appropriate here, because
the process is still free to attempt it, and we are explaining the
consequences below.

> s/attain/gain/ ?

Yes, thanks -- apparently, "attain" is more used for goals whereas
"gain" more for resources, so "gain" seems more correct here.

> 
> > >      destination directory than it previously had in the source
> > >      directory.  If this is attempted, the operation results in an
> > >      ``EXDEV`` error.
> 
> Better too!
> 
> This is becoming a bit difficult to follow, you can send a new patch with
> Alex in Cc. :)

Thanks for the feedback, I will send a revised patch.

–-Günther
diff mbox series

Patch

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index f3223f96469..f6bccd87aa0 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, 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 for the source file. 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::
  *