Message-ID: <2bce0b5a-a679-93f2-995c-cb0e80c82bf2@digikod.net>
Date:   Mon, 28 Nov 2022 21:26:40 +0100
From:   Mickaël Salaün <mic@...ikod.net>
To:     "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>
Cc:     willemdebruijn.kernel@...il.com, gnoack3000@...il.com,
        linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org, artem.kuzin@...wei.com,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH v8 12/12] landlock: Document Landlock's network support


On 28/11/2022 07:44, Konstantin Meskhidze (A) wrote:
> 
> 
> 11/17/2022 9:44 PM, Mickaël Salaün пишет:
>>
>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>> Describes network access rules for TCP sockets. Adds network access
>>> example in the tutorial. Points out AF_UNSPEC socket family behaviour.
>>> Adds kernel configuration support for network.
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>>> ---
>>>
>>> Changes since v7:
>>> * Fixes documentaion logic errors and typos as Mickaёl suggested:
>>> https://lore.kernel.org/netdev/9f354862-2bc3-39ea-92fd-53803d9bbc21@digikod.net/
>>>
>>> Changes since v6:
>>> * Adds network support documentaion.
>>>
>>> ---
>>>    Documentation/userspace-api/landlock.rst | 72 +++++++++++++++++++-----
>>>    1 file changed, 59 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
>>> index d8cd8cd9ce25..d0610ec9ce05 100644
>>> --- a/Documentation/userspace-api/landlock.rst
>>> +++ b/Documentation/userspace-api/landlock.rst
>>> @@ -11,10 +11,10 @@ Landlock: unprivileged access control
>>>    :Date: October 2022
>>>
>>>    The goal of Landlock is to enable to restrict ambient rights (e.g. global
>>> -filesystem access) for a set of processes.  Because Landlock is a stackable
>>> -LSM, it makes possible to create safe security sandboxes as new security layers
>>> -in addition to the existing system-wide access-controls. This kind of sandbox
>>> -is expected to help mitigate the security impact of bugs or
>>> +filesystem or network access) for a set of processes.  Because Landlock
>>> +is a stackable LSM, it makes possible to create safe security sandboxes as new
>>> +security layers in addition to the existing system-wide access-controls. This
>>> +kind of sandbox is expected to help mitigate the security impact of bugs or
>>>    unexpected/malicious behaviors in user space applications.  Landlock empowers
>>>    any process, including unprivileged ones, to securely restrict themselves.
>>>
>>> @@ -30,18 +30,20 @@ Landlock rules
>>>
>>>    A Landlock rule describes an action on an object.  An object is currently a
>>>    file hierarchy, and the related filesystem actions are defined with `access
>>> -rights`_.  A set of rules is aggregated in a ruleset, which can then restrict
>>> -the thread enforcing it, and its future children.
>>> +rights`_.  Since ABI version 4 a port data appears with related network actions
>>> +for TCP socket families.  A set of rules is aggregated in a ruleset, which
>>> +can then restrict the thread enforcing it, and its future children.
>>>
>>>    Defining and enforcing a security policy
>>>    ----------------------------------------
>>>
>>>    We first need to define the ruleset that will contain our rules.  For this
>>>    example, the ruleset will contain rules that only allow read actions, but write
>>> -actions will be denied.  The ruleset then needs to handle both of these kind of
>>> +actions will be denied. The ruleset then needs to handle both of these kind of
>>>    actions.  This is required for backward and forward compatibility (i.e. the
>>>    kernel and user space may not know each other's supported restrictions), hence
>>> -the need to be explicit about the denied-by-default access rights.
>>> +the need to be explicit about the denied-by-default access rights.  Also ruleset
>>> +will have network rules for specific ports, so it should handle network actions.
>>>
>>>    .. code-block:: c
>>>
>>> @@ -62,6 +64,9 @@ the need to be explicit about the denied-by-default access rights.
>>>                LANDLOCK_ACCESS_FS_MAKE_SYM |
>>>                LANDLOCK_ACCESS_FS_REFER |
>>>                LANDLOCK_ACCESS_FS_TRUNCATE,
>>> +        .handled_access_net =
>>> +            LANDLOCK_ACCESS_NET_BIND_TCP |
>>> +            LANDLOCK_ACCESS_NET_CONNECT_TCP,
>>>        };
>>>
>>>    Because we may not know on which kernel version an application will be
>>> @@ -70,14 +75,18 @@ should try to protect users as much as possible whatever the kernel they are
>>>    using.  To avoid binary enforcement (i.e. either all security features or
>>>    none), we can leverage a dedicated Landlock command to get the current version
>>>    of the Landlock ABI and adapt the handled accesses.  Let's check if we should
>>> -remove the ``LANDLOCK_ACCESS_FS_REFER`` or ``LANDLOCK_ACCESS_FS_TRUNCATE``
>>> -access rights, which are only supported starting with the second and third
>>> -version of the ABI.
>>> +remove the `LANDLOCK_ACCESS_FS_REFER` or `LANDLOCK_ACCESS_FS_TRUNCATE` or
>>> +network access rights, which are only supported starting with the second,
>>
>> This is a bad rebase.
> 
>     Sorry. Did not get it.

This hunk (and maybe others) changes unrelated things (e.g. back quotes).


>>
>>
>>> +third and fourth version of the ABI.
>>>
>>>    .. code-block:: c
>>>
>>>        int abi;
>>>
>>> +    #define ACCESS_NET_BIND_CONNECT ( \
>>> +    LANDLOCK_ACCESS_NET_BIND_TCP | \
>>> +    LANDLOCK_ACCESS_NET_CONNECT_TCP)
>>
>> Please add a 4-spaces prefix for these two lines.
> 
>     Like this??
> 	#define ACCESS_NET_BIND_CONNECT ( \
>               LANDLOCK_ACCESS_NET_BIND_TCP | \
>               LANDLOCK_ACCESS_NET_CONNECT_TCP)

Like for other indentations in the documentation (e.g. ruleset_attr 
definition):

#define ACCESS_NET_BIND_CONNECT ( \
     LANDLOCK_ACCESS_NET_BIND_TCP | \
     LANDLOCK_ACCESS_NET_CONNECT_TCP)

Powered by blists - more mailing lists