[PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Mickaël Salaün]

On 23/08/2022 22:07, Günther Noack wrote:
> On Tue, Aug 23, 2022 at 04:41:23PM +0200, Mickaël Salaün wrote:
>> With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first
>> globally denied-by-default access right.  Indeed, this lifted an initial
>> Landlock limitation to rename and link files, which was initially always
>> denied when the source or the destination were different directories.
>>
>> This led to an inconsistent backward compatibility behavior which was
>> only taken into account if no domain layer were using the new
>> LANDLOCK_ACCESS_FS_REFER right.  However, in a scenario where layers are
>> using the first and the second Landlock ABI (i.e.
>> LANDLOCK_ACCESS_FS_REFER or not), the access control behaves like if all
>> domains were handling LANDLOCK_ACCESS_FS_REFER with their rules
>> implicitely allowing such right.

"the access control behaves like if domains not handling 
LANDLOCK_ACCESS_FS_REFER are in fact handling it and with their rules 
implicitely allowing such right."

Is this better?


>>  Indeed, the not-handled access rights
>> were allowed, which should not be the case for LANDLOCK_ACCESS_FS_REFER.
>> It should be noted that this bug only allowed safe renames or links, but
>> no privilege escalation because the LANDLOCK_ACCESS_FS_REFER properties
>> were still enforced (e.g. only allowed to move a file according to all
>> other access rights, and if it doesn't give more Landlock accesses).
> 
> I had trouble understanding this paragraph - but I tried it out and I believe
> I understand what you mean; let me try to paraphrase to see whether we are on
> the same page:
> 
> This is a bug where chaining an additional ruleset on top of another one can
> increase the number of possible operations, rather than just reduce it.
> 
> Steps to reproduce:
>   - Enforce ruleset 1 *without* handled_access_fs containing "refer"
>   - Enforce ruleset 2 with handled_access_fs containing "refer"
> 
> Expected behaviour:
>   - refer-operations (rename, link...) should not be permitted because
>     ruleset 1 uses ABI v1 and refer-operations are always forbidden there.
> 
> Observed behaviour:
>   - refer-operations (rename, link...) work as if LANDLOCK_ACCESS_FS_REFER had
>     been handled in both rulesets and all rules within

"All rules within the ruleset/layer not (theoritically) handling refer 
operations". Layers handling refer-operations behave as expected and are 
not affected by this bug.

> 
> Do I understand this correctly?

Yes, except for the rules correctly handling refer operations.


> 
> 
> I believe I can reproduce it with the Go sandboxing tool (go install
> github.com/landlock-lsm/go-landlock/cmd/landlock-restrict at latest) like this:
> 
> Starting with these directory contents:
> 
>    $ find .
>    .
>    ./dst
>    ./src
>    ./src/hello
> 
> Then it is not possible to use refer (linking) with an ABIv1 ruleset (expected):
> 
>    $ $HOME/go/bin/landlock-restrict -1 -ro / -rw . -- \
>      /bin/ln src/hello dst/hello
>    /bin/ln: failed to create hard link 'dst/hello' => 'src/hello': Invalid cross-device link
> 
> But when chaining that ruleset with a ABIv2 ruleset, it suddenly does become
> possible to do "refer" operations:
> 
>    $ $HOME/go/bin/landlock-restrict -1 -ro / -rw . -- \
>      $HOME/go/bin/landlock-restrict -2 -ro / -rw +refer . -- \
>      /bin/ln src/hello dst/hello
>    $ find .
>    .
>    ./dst
>    ./dst/hello
>    ./src
>    ./src/hello

This is correct. In this case, the first landlock-restrict command 
behaves as the second command when the second is executed (i.e. a new 
refer-aware layer is added).


> 
> I need to still understand the underlying code better to reason about it,
> but this is the issue that this patch is fixing, right?
> 
> —Günther
> 
>> This change adds an ACCESS_INITIALLY_DENIED list of denied-by-default
>> rights, which (only) contains LANDLOCK_ACCESS_FS_REFER.  All domains are
>> treated as if they are also handling this list, but without modifying
>> their fs_access_masks field, which enables correct domain audit.
>>
>> A side effect is that the errno code returned by rename(2) or link(2)
>> *may* be changed from EXDEV to EACCES according to the enforced
>> restrictions.  Indeed, we now have the mechanic to identify if an access
>> is denied because of a required right (e.g. LANDLOCK_ACCESS_FS_MAKE_REG,
>> LANDLOCK_ACCESS_FS_REMOVE_FILE) or if it is denied because of missing
>> LANDLOCK_ACCESS_FS_REFER rights.  This may result in different errno
>> codes than for the initial Landlock version, but this approach is better
>> for rename/link compatibility reasons, and it wasn't possible before
>> (hence no backport to ABI v1).  The layout1.rename_file test reflects
>> this change.
>>
>> Add the layout1.refer_denied_by_default* tests to check that the
>> behavior of a ruleset not handling LANDLOCK_ACCESS_FS_REFER (ABI v1) is
>> unchanged even if another layer handles LANDLOCK_ACCESS_FS_REFER (i.e.
>> ABI v1 precedence).  Make sure rule's absolute access rights are correct
>> by testing with and without a matching path.
>>
>> Extend layout1.inval tests to check that a denied-by-default access
>> right is not necessarily part of a domain's handled access rights.
>>
>> Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
>> Cc: Günther Noack <gnoack3000 at gmail.com>
>> Cc: Paul Moore <paul at paul-moore.com>
>> Signed-off-by: Mickaël Salaün <mic at digikod.net>
>> Link: https://lore.kernel.org/r/20220823144123.633721-1-mic@digikod.net