Message-ID: <526a70a2-b0bc-f29a-6558-022ca12a6430@huawei.com>
Date:   Mon, 30 Jan 2023 13:03:49 +0300
From:   "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>
To:     Mickaël Salaün <mic@...ikod.net>,
        Günther Noack <gnoack3000@...il.com>
CC:     <willemdebruijn.kernel@...il.com>,
        <linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
        <netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>,
        <artem.kuzin@...wei.com>
Subject: Re: [PATCH v9 12/12] landlock: Document Landlock's network support



1/27/2023 9:22 PM, Mickaël Salaün пишет:
> 
> On 23/01/2023 10:38, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 1/22/2023 2:07 AM, Günther Noack пишет:
> 
> [...]
> 
>>>> @@ -143,10 +157,24 @@ 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.
>>>>   
>>>> -We now have a ruleset with one rule allowing read access to ``/usr`` while
>>>> -denying all other handled accesses for the filesystem.  The next step is to
>>>> -restrict the current thread from gaining more privileges (e.g. thanks to a SUID
>>>> -binary).
>>>> +For network access-control, we can add a set of rules that allow to use a port
>>>> +number for a specific action. All ports values must be defined in network byte
>>>> +order.
>>>
>>> What is the point of asking user space to convert this to network byte
>>> order? It seems to me that the kernel would be able to convert it to
>>> network byte order very easily internally and in a single place -- why
>>> ask all of the users to deal with that complexity? Am I overlooking
>>> something?
>> 
>>    I had a discussion about this issue with Mickaёl.
>>    Please check these threads:
>>    1.
>> https://lore.kernel.org/netdev/49391484-7401-e7c7-d909-3bd6bd024731@digikod.net/
>>    2.
>> https://lore.kernel.org/netdev/1ed20e34-c252-b849-ab92-78c82901c979@huawei.com/
> 
> I'm definitely not sure if this is the right solution, or if there is
> one. The rationale is to make it close to the current (POSIX) API. We
> didn't get many opinion about that but I'd really like to have a
> discussion about port endianness for this Landlock API.

   As for me, the kernel should take care about port converting. This 
work should be done under the hood.

   Any thoughts?

> 
> I looked at some code (e.g. see [1]) and it seems that using htons()
> might make application patching more complex after all. What do you
> think? Is there some network (syscall) API that don't use this convention?
> 
> [1] https://github.com/landlock-lsm/tuto-lighttpd
> 
>>>
>>>> +
>>>> +.. code-block:: c
>>>> +
>>>> +    struct landlock_net_service_attr net_service = {
>>>> +        .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
>>>> +        .port = htons(8080),
>>>> +    };
>>>
>>> This is a more high-level comment:
>>>
>>> The notion of a 16-bit "port" seems to be specific to TCP and UDP --
>>> how do you envision this struct to evolve if other protocols need to
>>> be supported in the future?
>> 
>>     When TCP restrictions land into Linux, we need to think about UDP
>> support. Then other protocols will be on the road. Anyway you are right
>> this struct will be evolving in long term, but I don't have a particular
>> envision now. Thanks for the question - we need to think about it.
>>>
>>> Should this struct and the associated constants have "TCP" in its
>>> name, and other protocols use a separate struct in the future?
> 
> Other protocols such as AF_VSOCK uses a 32-bit port. We could use a
> 32-bits port field or ever a 64-bit one. The later could make more sense
> because each field would eventually be aligned on 64-bit. Picking a
> 16-bit value was to help developers (and compilers/linters) with the
> "correct" type (for TCP).
> 
> If we think about protocols other than TCP and UDP (e.g. AF_VSOCK), it
> could make sense to have a dedicated attr struct specifying other
> properties (e.g. CID). Anyway, the API is flexible but it would be nice
> to not mess with it too much. What do you think?
> 
> 
>>>
>>>> +
>>>> +    err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
>>>> +                            &net_service, 0);
>>>> +
>>>> +The next step is to restrict the current thread from gaining more privileges
>>>> +(e.g. thanks to a SUID binary). We now have a ruleset with the first rule allowing
>>>            ^^^^^^
>>>            "through" a SUID binary? "thanks to" sounds like it's desired
>>>            to do that, while we're actually trying to prevent it here?
>> 
>>     This is Mickaёl's part. Let's ask his opinion here.
>> 
>>     Mickaёl, any thoughts?
> 
> Yep, "through" looks better.
> .

Powered by blists - more mailing lists