Message-ID: <7831e55d-34ef-cf74-3d47-15e2e1edf96c@digikod.net>
Date:   Mon, 23 Nov 2020 22:16:27 +0100
From:   Mickaël Salaün <mic@...ikod.net>
To:     Jann Horn <jannh@...gle.com>, Arnd Bergmann <arnd@...db.de>
Cc:     James Morris <jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Andy Lutomirski <luto@...capital.net>,
        Anton Ivanov <anton.ivanov@...bridgegreys.com>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Jeff Dike <jdike@...toit.com>,
        Jonathan Corbet <corbet@....net>,
        Kees Cook <keescook@...omium.org>,
        Michael Kerrisk <mtk.manpages@...il.com>,
        Richard Weinberger <richard@....at>,
        Shuah Khan <shuah@...nel.org>,
        Vincent Dagonneau <vincent.dagonneau@....gouv.fr>,
        Kernel Hardening <kernel-hardening@...ts.openwall.com>,
        Linux API <linux-api@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        linux-security-module <linux-security-module@...r.kernel.org>,
        the arch/x86 maintainers <x86@...nel.org>,
        Mickaël Salaün <mic@...ux.microsoft.com>
Subject: Re: [PATCH v24 07/12] landlock: Support filesystem access-control


On 23/11/2020 20:44, Jann Horn wrote:
> On Sat, Nov 21, 2020 at 11:06 AM Mickaël Salaün <mic@...ikod.net> wrote:
>> On 21/11/2020 08:00, Jann Horn wrote:
>>> On Thu, Nov 12, 2020 at 9:52 PM Mickaël Salaün <mic@...ikod.net> wrote:
>>>> Thanks to the Landlock objects and ruleset, it is possible to identify
>>>> inodes according to a process's domain.  To enable an unprivileged
>>>> process to express a file hierarchy, it first needs to open a directory
>>>> (or a file) and pass this file descriptor to the kernel through
>>>> landlock_add_rule(2).  When checking if a file access request is
>>>> allowed, we walk from the requested dentry to the real root, following
>>>> the different mount layers.  The access to each "tagged" inodes are
>>>> collected according to their rule layer level, and ANDed to create
>>>> access to the requested file hierarchy.  This makes possible to identify
>>>> a lot of files without tagging every inodes nor modifying the
>>>> filesystem, while still following the view and understanding the user
>>>> has from the filesystem.
>>>>
>>>> Add a new ARCH_EPHEMERAL_INODES for UML because it currently does not
>>>> keep the same struct inodes for the same inodes whereas these inodes are
>>>> in use.
>>>>
>>>> This commit adds a minimal set of supported filesystem access-control
>>>> which doesn't enable to restrict all file-related actions.  This is the
>>>> result of multiple discussions to minimize the code of Landlock to ease
>>>> review.  Thanks to the Landlock design, extending this access-control
>>>> without breaking user space will not be a problem.  Moreover, seccomp
>>>> filters can be used to restrict the use of syscall families which may
>>>> not be currently handled by Landlock.
>>>>
>>>> Cc: Al Viro <viro@...iv.linux.org.uk>
>>>> Cc: Anton Ivanov <anton.ivanov@...bridgegreys.com>
>>>> Cc: James Morris <jmorris@...ei.org>
>>>> Cc: Jann Horn <jannh@...gle.com>
>>>> Cc: Jeff Dike <jdike@...toit.com>
>>>> Cc: Kees Cook <keescook@...omium.org>
>>>> Cc: Richard Weinberger <richard@....at>
>>>> Cc: Serge E. Hallyn <serge@...lyn.com>
>>>> Signed-off-by: Mickaël Salaün <mic@...ux.microsoft.com>
>>>> ---
>>>>
>>>> Changes since v23:
>>>> * Enforce deterministic interleaved path rules.  To have consistent
>>>>   layered rules, granting access to a path implies that all accesses
>>>>   tied to inodes, from the requested file to the real root, must be
>>>>   checked.  Otherwise, stacked rules may result to overzealous
>>>>   restrictions.  By excluding the ability to add exceptions in the same
>>>>   layer (e.g. /a allowed, /a/b denied, and /a/b/c allowed), we get
>>>>   deterministic interleaved path rules.  This removes an optimization
>>>
>>> I don't understand the "deterministic interleaved path rules" part.
>>
>> I explain bellow.
>>
>>>
>>>
>>> What if I have a policy like this?
>>>
>>> /home/user READ
>>> /home/user/Downloads READ+WRITE
>>>
>>> That's a reasonable policy, right?
>>
>> Definitely, I forgot this, thanks for the outside perspective!
>>
>>>
>>> If I then try to open /home/user/Downloads/foo in WRITE mode, the loop
>>> will first check against the READ+WRITE rule for /home/user, that
>>> check will pass, and then it will check against the READ rule for /,
>>> which will deny the access, right? That seems bad.
>>
>> Yes that was the intent.
>>
>>>
>>>
>>> The v22 code ensured that for each layer, the most specific rule (the
>>> first we encounter on the walk) always wins, right? What's the problem
>>> with that?
>>
>> This can be explained with the interleaved_masked_accesses test:
>> https://github.com/landlock-lsm/linux/blob/landlock-v24/tools/testing/selftests/landlock/fs_test.c#L647
>>
>> In this case there is 4 stacked layers:
>> layer 1: allows s1d1/s1d2/s1d3/file1
>> layer 2: allows s1d1/s1d2/s1d3
>>          denies s1d1/s1d2
>> layer 3: allows s1d1
>> layer 4: allows s1d1/s1d2
>>
>> In the v23, access to file1 would be allowed until layer 3, but layer 4
>> would merge a new rule for the s1d2 inode. Because we don't record where
>> exactly the access come from, we can't tell that layer 2 allowed access
>> thanks to s1d3 and that its s1d2 rule was ignored. I think this behavior
>> doesn't make sense from the user point of view.
> 
> Aah, I think I'm starting to understand the issue now. Basically, with
> the current UAPI, the semantics have to be "an access is permitted if,
> for each policy layer, at least one rule encountered on the pathwalk
> permits the access; rules that deny the access are irrelevant". And if
> it turns out that someone needs to be able to deny access to specific
> inodes, we'll have to extend struct landlock_path_beneath_attr.

Right, I'll add this to the documentation (aligned with the new
implementation).

> 
> That reminds me... if we do need to make such a change in the future,
> it would be easier in terms of UAPI compatibility if
> landlock_add_rule() used copy_struct_from_user(), which is designed to
> create backwards and forwards compatibility with other version of UAPI
> headers. So adding that now might save us some headaches later.

I used copy_struct_from_user() before v21, but Arnd wasn't a fan of
having type and size arguments, so we simplified the UAPI in the v21 by
removing the size argument. The type argument is enough to extend the
structure, but indeed, we lose the forward compatibility. Relying on one
syscall per rule type seems too much, though.

Arnd, what do you think?

> 
> 
>> In the v24, access to file1 would only be allowed with layer 1. The
>> layer 2, would deny access to file1 because of the s1d2 rule. This makes
>> the reasoning consistent and deterministic whatever the layers are,
>> while storing the same access and layer bits. But I agree that this may
>> not be desirable.
>>
>> In a perfect v25, file1 should be allowed by all these layers. I didn't
>> find a simple solution to this while minimizing the memory allocated by
>> rule (cf. struct landlock_rule: mainly 32-bits for access rights and
>> 64-bits for the layers that contributed to this ANDed accesses). I would
>> like to avoid storing 32-bits access rights per stacked layer. Do you
>> see another solution?
> 
> I don't think you can avoid storing the access rights per layer unless
> you actually merge the layers when setting up the ruleset (which would
> be messy). But I don't think that's a big problem. A straightforward
> implementation might become inefficient if you stack too many policy
> layers, but I don't think that's a problem for an initial
> implementation - the common usecase is probably going to be a single
> layer, or maybe two, or something like that?

If layers are correctly used, a few I guess.

> 
> If you had a ton of layers, most of them would likely specify the same
> access permissions - so one possible optimization might be to use the
> current representation if all rules matching the inode specify the
> same permissions, and use a different representation otherwise. But I
> don't think such an optimization is necessary at this point.

I agree, I'll not implement such optimization for now.

Powered by blists - more mailing lists