Message-ID: <db38b163-ceb9-c74b-bcd5-402c646abea7@huawei-partners.com>
Date: Thu, 3 Oct 2024 17:00:14 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Günther Noack <gnoack@...gle.com>
CC: <mic@...ikod.net>, <willemdebruijn.kernel@...il.com>,
	<gnoack3000@...il.com>, <linux-security-module@...r.kernel.org>,
	<netdev@...r.kernel.org>, <netfilter-devel@...r.kernel.org>,
	<yusongping@...wei.com>, <artem.kuzin@...wei.com>,
	<konstantin.meskhidze@...wei.com>
Subject: Re: [RFC PATCH v3 19/19] landlock: Document socket rule type support

On 10/1/2024 10:09 AM, Günther Noack wrote:
> Hello!
> 
> On Wed, Sep 04, 2024 at 06:48:24PM +0800, Mikhail Ivanov wrote:
>> Extend documentation with socket rule type description.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
>> ---
>>   Documentation/userspace-api/landlock.rst | 46 ++++++++++++++++++++----
>>   1 file changed, 40 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
>> index 37dafce8038b..4bf45064faa1 100644
>> --- a/Documentation/userspace-api/landlock.rst
>> +++ b/Documentation/userspace-api/landlock.rst
>> @@ -33,7 +33,7 @@ A Landlock rule describes an action on an object which the process intends to
>>   perform.  A set of rules is aggregated in a ruleset, which can then restrict
>>   the thread enforcing it, and its future children.
>>   
>> -The two existing types of rules are:
>> +The three existing types of rules are:
>>   
>>   Filesystem rules
>>       For these rules, the object is a file hierarchy,
>> @@ -44,14 +44,19 @@ Network rules (since ABI v4)
>>       For these rules, the object is a TCP port,
>>       and the related actions are defined with `network access rights`.
>>   
>> +Socket rules (since ABI v6)
>> +    For these rules, the object is a pair of an address family and a socket type,
>> +    and the related actions are defined with `socket access rights`.
>> +
>>   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 filesystem
>> -read actions and establish a specific TCP connection. Filesystem write
>> -actions and other TCP actions will be denied.
>> +read actions, create TCP sockets and establish a specific TCP connection.
>> +Filesystem write actions, creating non-TCP sockets and other TCP
>> +actions will be denied.
>>   
>>   The ruleset then needs to handle both these kinds of actions.  This is
>>   required for backward and forward compatibility (i.e. the kernel and user
>> @@ -81,6 +86,8 @@ to be explicit about the denied-by-default access rights.
>>           .handled_access_net =
>>               LANDLOCK_ACCESS_NET_BIND_TCP |
>>               LANDLOCK_ACCESS_NET_CONNECT_TCP,
>> +        .handled_access_socket =
>> +            LANDLOCK_ACCESS_SOCKET_CREATE,
>>       };
>>   
>>   Because we may not know on which kernel version an application will be
>> @@ -119,6 +126,11 @@ version, and only use the available subset of access rights:
>>       case 4:
>>           /* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
>>           ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
>> +        __attribute__((fallthrough));
>> +	case 5:
>> +		/* Removes socket support for ABI < 6 */
>> +		ruleset_attr.handled_access_socket &=
>> +			~LANDLOCK_ACCESS_SOCKET_CREATE;
> 
> When I patched this in, the indentation of this "case" was off, compared to the
> rest of the code example.  (The code example uses spaces for indentation, not
> tabs.)
Thanks for noticing this! Will be fixed.

> 
>>       }
>>   
>>   This enables to create an inclusive ruleset that will contain our rules.
>> @@ -170,6 +182,20 @@ for the ruleset creation, by filtering access rights according to the Landlock
>>   ABI version.  In this example, this is not required because all of the requested
>>   ``allowed_access`` rights are already available in ABI 1.
>>   
>> +For socket access-control, we can add a rule to allow TCP sockets creation. UNIX,
>> +UDP IP and other protocols will be denied by the ruleset.
>> +
>> +.. code-block:: c
>> +
>> +    struct landlock_net_port_attr tcp_socket = {
>> +        .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> +        .family = AF_INET,
>> +        .type = SOCK_STREAM,
>> +    };
>> +
>> +    err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> +                            &tcp_socket, 0);
>> +
> 
> IMHO, the length of the "Defining and enforcing a security policy" section is
> slowly getting out of hand.  This was easier to follow when it was only file
> system rules. -- I wonder whether we should split this up in subsections for the
> individual steps to give this a more logical outline, e.g.
> 
> * Creating a ruleset
> * Adding rules to the ruleset
>    * Adding a file system rule
>    * Adding a network rule
>    * Adding a socket rule
> * Enforcing the ruleset

I agree, it's important to keep usage usage description as simple as it
possible. Should I include related commit in current patchset?

> 
>>   For network access-control, we can add a set of rules that allow to use a port
>>   number for a specific action: HTTPS connections.
>>   
>> @@ -186,7 +212,8 @@ number for a specific action: HTTPS connections.
>>   The next step is to restrict the current thread from gaining more privileges
>>   (e.g. through a SUID binary).  We now have a ruleset with the first rule
>>   allowing read access to ``/usr`` while denying all other handled accesses for
>> -the filesystem, and a second rule allowing HTTPS connections.
>> +the filesystem, a second rule allowing TCP sockets and a third rule allowing
>> +HTTPS connections.
>>   
>>   .. code-block:: c
>>   
>> @@ -404,7 +431,7 @@ Access rights
>>   -------------
>>   
>>   .. kernel-doc:: include/uapi/linux/landlock.h
>> -    :identifiers: fs_access net_access
>> +    :identifiers: fs_access net_access socket_access
>>   
>>   Creating a new ruleset
>>   ----------------------
>> @@ -423,7 +450,7 @@ Extending a ruleset
>>   
>>   .. kernel-doc:: include/uapi/linux/landlock.h
>>       :identifiers: landlock_rule_type landlock_path_beneath_attr
>> -                  landlock_net_port_attr
>> +                  landlock_net_port_attr landlock_socket_attr
>>   
>>   Enforcing a ruleset
>>   -------------------
>> @@ -541,6 +568,13 @@ earlier ABI.
>>   Starting with the Landlock ABI version 5, it is possible to restrict the use of
>>   :manpage:`ioctl(2)` using the new ``LANDLOCK_ACCESS_FS_IOCTL_DEV`` right.
>>   
>> +Socket support (ABI < 6)
>> +-------------------------
>> +
>> +Starting with the Landlock ABI version 6, it is now possible to restrict
>> +creation of user space sockets to only a set of allowed protocols thanks
>> +to the new ``LANDLOCK_ACCESS_SOCKET_CREATE`` access right.
>> +
>>   .. _kernel_support:
>>   
>>   Kernel support
>> -- 
>> 2.34.1
>>
> 
> There is a section further below called "Network support" that talks about the
> need for CONFIG_INET in order to add a network rule.  Do similar restrictions
> apply to the socket rules as well?  Maybe this should be added to the section.

No, socket rules should be supported with default config. The only
restriction which we came to is that socket type and address family
values should fit related ranges [1].

[1] 
https://lore.kernel.org/all/deeada6f-2538-027a-4922-8697fc59c43f@huawei-partners.com/

> 
> Please don't forget -- Tahera Fahimi's "scoped" patches have landed in
> linux-next by now, so we will need to rebase and bump the ABI version one higher
> than before.

yeah, thank you!

> 
> Thanks,
> —Günther

Powered by blists - more mailing lists