Message-ID: <ZqtlJZMHVf-otlOq@google.com>
Date: Thu, 1 Aug 2024 12:36:21 +0200
From: "Günther Noack" <gnoack@...gle.com>
To: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.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

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 :)


> > 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/


> > 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.)


> > 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