Message-ID: <51b6b614-66d7-1acc-a676-b7302537e1fb@huawei-partners.com>
Date: Thu, 1 Aug 2024 14:45:04 +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>, <alx@...nel.org>
Subject: Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

8/1/2024 1:36 PM, Günther Noack wrote:
> On Wed, Jul 31, 2024 at 08:20:41PM +0300, Mikhail Ivanov wrote:
>> 7/30/2024 11:24 AM, Günther Noack wrote:
>>> On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
>>>> LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
>>>> ports to forbid a malicious sandboxed process to impersonate a legitimate
>>>> server process. However, bind(2) might be used by (TCP) clients to set the
>>>> source port to a (legitimate) value. Controlling the ports that can be
>>>> used for listening would allow (TCP) clients to explicitly bind to ports
>>>> that are forbidden for listening.
>>>>
>>>> Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
>>>> access right that restricts listening on undesired ports with listen(2).
>>>
>>> Nit: I would turn around the first two commit message paragraphs and describe
>>> your changes first, before explaining the problems in the bind(2) support.  I
>>> was initially a bit confused that the description started talking about
>>> LANDLOCK_ACCESS_NET_BIND_TCP.
>>>
>>> General recommendations at:
>>> https://www.kernel.org/doc/html/v6.10/process/submitting-patches.html#describe-your-changes
>>
>> I consider the first paragraph as a problem statement for this patch.
>> According to linux recommendations problem should be established before
>> the description of changes. Do you think that the changes part should
>> stand before the problem anyway?
> 
> Up to you. To be fair, I'm sold on the approach in this patchset anyway :)

Nice :)

> 
> 
>>> When we have the documentation wording finalized,
>>> please send an update to the man pages as well,
>>> for this and other documentation updates.
>>
>> Should I send it after this patchset would be accepted?
> 
> Yes, that would be the normal process which we have been following so far.
> 
> (I don't like the process much either, because it decouples feature development
> so far from documentation writing, but it's what we have for now.)
> 
> An example patch which does that for the network bind(2) and connect(2) features
> (and where I would still like a review from Konstantin) is:
> https://lore.kernel.org/all/20240723101917.90918-1-gnoack@google.com/

got it

> 
> 
>>> Small remarks on what I've done here:
>>>
>>> * I am avoiding the word "binding" when referring to the automatic assignment to
>>>     an ephemeral port - IMHO, this is potentially confusing, since bind(2) is not
>>>     explicitly called.
>>> * I am also dropping the "It should be noted" / "Note that" phrase, which is
>>>     frowned upon in man pages.
>>
>> Didn't know that, thanks
> 
> Regarding "note that", see
> https://lore.kernel.org/all/0aafcdd6-4ac7-8501-c607-9a24a98597d7@gmail.com/
> https://lore.kernel.org/linux-man/20210729223535.qvyomfqvvahzmu5w@localhost.localdomain/
> https://lore.kernel.org/linux-man/20230105225235.6cjtz6orjzxzvo6v@illithid/
> (The "Kemper notectomy")
> 
> This came up in man page reviews, but we'll have an easier time keeping the
> kernel and man page documentation in sync if we adhere to man page style
> directly.  (The man page style is documented in man-pages(7) and contains some
> groff-independent wording advice as well.)

Ok, such phrases should be really avoided in kernel as well.

> 
> 
>>> If I understand correctly, these are cases where we use TCP on top of protocols
>>> that are not IP (or have an additional layer in the middle, like TLS?).  This
>>> can not be recognized through the socket family or type?
>>
>> ULP can be used in the context of TCP protocols as an additional layer
>> (currently supported only by IP and MPTCP), so it cannot be recognized
>> with family or type. You can check this test [1] in which TCP IP socket
>> is created with ULP control hook.
>>
>> [1] https://lore.kernel.org/all/20240728002602.3198398-8-ivanov.mikhail1@huawei-partners.com/
> 
> Thanks, this is helpful.
> 
> For reference, it seems that ULP were introduced in
> https://lore.kernel.org/all/20170614183714.GA80310@davejwatson-mba.dhcp.thefacebook.com/
> 
> 
>>> Do we have cases where we can run TCP on top of something else than plain IPv4
>>> or IPv6, where the clone method exists?
>>
>> Yeah, MPTCP protocol for example (see net/mptcp/subflow.c). ULP control
>> hook is supported only by IP and MPTCP, and in both cases
>> clone method is checked during listen(2) execution.
> 
> 
>>> Aren't the socket type and family checks duplicated with existing logic that we
>>> have for the connect(2) and bind(2) support?  Should it be deduplicated, or is
>>> that too messy?
>>
>> bind(2) and connect(2) hooks also support AF_UNSPEC family, so I think
>> such helper is gonna complicate code a little bit. Also it can
>> complicate switch in current_check_access_socket().
> 
> OK, sounds good. 👍
> 
> —Günther

Powered by blists - more mailing lists