Message-ID: <ZvZ_ZjcKJPm5B3_Z@google.com>
Date: Fri, 27 Sep 2024 11:48:22 +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
Subject: Re: [RFC PATCH v3 14/19] selftests/landlock: Test socketpair(2) restriction

On Mon, Sep 23, 2024 at 03:57:47PM +0300, Mikhail Ivanov wrote:
> On 9/18/2024 4:47 PM, Günther Noack wrote:
> > On Wed, Sep 04, 2024 at 06:48:19PM +0800, Mikhail Ivanov wrote:
> > > Add test that checks the restriction on socket creation using
> > > socketpair(2).
> > > 
> > > Add `socket_creation` fixture to configure sandboxing in tests in
> > > which different socket creation actions are tested.
> > > 
> > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
> > > ---
> > >   .../testing/selftests/landlock/socket_test.c  | 101 ++++++++++++++++++
> > >   1 file changed, 101 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> > > index 8fc507bf902a..67db0e1c1121 100644
> > > --- a/tools/testing/selftests/landlock/socket_test.c
> > > +++ b/tools/testing/selftests/landlock/socket_test.c
> > > @@ -738,4 +738,105 @@ TEST_F(packet_protocol, alias_restriction)
> > >   	EXPECT_EQ(0, test_socket_variant(&self->prot_tested));
> > >   }
> > > +static int test_socketpair(int family, int type, int protocol)
> > > +{
> > > +	int fds[2];
> > > +	int err;
> > > +
> > > +	err = socketpair(family, type | SOCK_CLOEXEC, protocol, fds);
> > > +	if (err)
> > > +		return errno;
> > > +	/*
> > > +	 * Mixing error codes from close(2) and socketpair(2) should not lead to
> > > +	 * any (access type) confusion for this test.
> > > +	 */
> > > +	if (close(fds[0]) != 0)
> > > +		return errno;
> > > +	if (close(fds[1]) != 0)
> > > +		return errno;
> > > +	return 0;
> > > +}
> > > +
> > > +FIXTURE(socket_creation)
> > > +{
> > > +	bool sandboxed;
> > > +	bool allowed;
> > > +};
> > > +
> > > +FIXTURE_VARIANT(socket_creation)
> > > +{
> > > +	bool sandboxed;
> > > +	bool allowed;
> > > +};
> > > +
> > > +FIXTURE_SETUP(socket_creation)
> > > +{
> > > +	self->sandboxed = variant->sandboxed;
> > > +	self->allowed = variant->allowed;
> > > +
> > > +	setup_loopback(_metadata);
> > > +};
> > > +
> > > +FIXTURE_TEARDOWN(socket_creation)
> > > +{
> > > +}
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(socket_creation, no_sandbox) {
> > > +	/* clang-format on */
> > > +	.sandboxed = false,
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(socket_creation, sandbox_allow) {
> > > +	/* clang-format on */
> > > +	.sandboxed = true,
> > > +	.allowed = true,
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(socket_creation, sandbox_deny) {
> > > +	/* clang-format on */
> > > +	.sandboxed = true,
> > > +	.allowed = false,
> > > +};
> > > +
> > > +TEST_F(socket_creation, socketpair)
> > > +{
> > > +	const struct landlock_ruleset_attr ruleset_attr = {
> > > +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> > > +	};
> > > +	struct landlock_socket_attr unix_socket_create = {
> > > +		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> > > +		.family = AF_UNIX,
> > > +		.type = SOCK_STREAM,
> > > +	};
> > > +	int ruleset_fd;
> > > +
> > > +	if (self->sandboxed) {
> > > +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> > > +						     sizeof(ruleset_attr), 0);
> > > +		ASSERT_LE(0, ruleset_fd);
> > > +
> > > +		if (self->allowed) {
> > > +			ASSERT_EQ(0, landlock_add_rule(ruleset_fd,
> > > +						       LANDLOCK_RULE_SOCKET,
> > > +						       &unix_socket_create, 0));
> > > +		}
> > > +		enforce_ruleset(_metadata, ruleset_fd);
> > > +		ASSERT_EQ(0, close(ruleset_fd));
> > > +	}
> > > +
> > > +	if (!self->sandboxed || self->allowed) {
> > > +		/*
> > > +		 * Tries to create sockets when ruleset is not established
> > > +		 * or protocol is allowed.
> > > +		 */
> > > +		EXPECT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
> > > +	} else {
> > > +		/* Tries to create sockets when protocol is restricted. */
> > > +		EXPECT_EQ(EACCES, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
> > > +	}
> > 
> > I am torn on whether socketpair() should be denied at all --
> > 
> >    * on one hand, the created sockets are connected to each other
> >      and the creating process can only talk to itself (or pass one of them on),
> >      which seems legitimate and harmless.
> > 
> >    * on the other hand, it *does* create two sockets, and
> >      if they are datagram sockets, it it probably currently possible
> >      to disassociate them with connect(AF_UNSPEC). >
> > What are your thoughts on that?
> 
> Good catch! According to the discussion that you've mentioned [1] (I
> believe I found correct one), you've already discussed socketpair(2)
> control with Mickaël and came to the conclusion that socketpair(2) and
> unnamed pipes do not give access to new resources to the process,
> therefore should not be restricted.
> 
> [1]
> https://lore.kernel.org/all/e7e24682-5da7-3b09-323e-a4f784f10158@digikod.net/
> 
> Therefore, this is more like connect(AF_UNSPEC)-related issue. On
> security summit you've mentioned that it will be useful to implement
> restriction of connection dissociation for sockets. This feature will
> solve the problem of reusage of UNIX sockets that were created with
> socketpair(2).
> 
> If we want such feature to be implemented, I suggest leaving current
> implementation as it is (to prevent vulnerable creation of UNIX dgram
> sockets) and enable socketpair(2) in the patchset dedicated to
> connect(AF_UNSPEC) restriction. Also it will be useful to create a
> dedicated issue on github. WDYT?

Thanks for digging up that discussion, that's exactly the one I meant.

I have a feeling that this may result in compatibility issues later on?  If we
leave the current implementation as it is, then we are *blocking* the creation
of sockets through socketpair(2).  And then we would have users who add it as a
restricted ("handled") operation in their ruleset, and who would expect that
socketpair(2) can not be used.  When that API is already fixed, how do you
imagine that people should in the future allow socketpair(2), but disallow the
"normal" creation of sockets?

In my mind, I would have imagined that the LANDLOCK_ACCESS_SOCKET_CREATE right
only restricts socket(2) invocations and leaves socketpair(2) working, and then
we could introduce a LANDLOCK_ACCESS_SOCKETPAIR_CREATE right in the future to
restrict socketpair(2) as well?

If we wanted to permit socketpair(2), but allow socket(2), would we have to
change the LSM hook interface?  How would that implementation look?


> (Btw I think that disassociation control can be really useful. If
> it were possible to restrict this action for each protocol, we would
> have stricter control over the protocols used.)

In my understanding, the disassociation support is closely intertwined with the
transport layer - the last paragraph of DESCRIPTION in connect(2) is listing
TCP, UDP and Unix Domain sockets in datagram mode. -- The relevant code in in
net/ipv4/af_inet.c in inet_dgram_connect() and __inet_stream_connect(), where
AF_UNSPEC is handled.

I would love to find a way to restrict this independent of the specific
transport protocol as well.

Remark on the side - in af_inet.c in inet_shutdown(), I also found a worrying
scenario where the same sk->sk_prot->disconnect() function is called and
sock->state also gets reset to SS_UNCONNECTED.  I have done a naive attempt to
hit that code path by calling shutdown() on a passive TCP socket, but was not
able to reuse the socket for new connections afterwards. (Have not debugged it
further though.)  I wonder whether this is a scnenario that we also need to
cover?


> > (On a much more technical note; consider replacing self->allowed with
> > self->socketpair_error to directly indicate the expected error? It feels that
> > this could be more straightforward?)
> 
> I've considered this approach and decided that this would
> * negatively affect the readability of conditional for adding Landlock
>   rule,
> * make checking the test_socketpair() error code less explicit.

Fair enough, OK.

—Günther

Powered by blists - more mailing lists