netdev - Re: [PATCH net 4/5] vsock/test: Add test for UAF due to socket unbinding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <34yqhvkemx27yoxwodfjdc7rwvuyr6sq2e2nlpyfhzvyscvccq@du25v6ljrebj>
Date: Mon, 20 Jan 2025 11:20:05 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Michal Luczaj <mhal@...x.co>
Cc: "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	George Zhang <georgezhang@...are.com>, Dmitry Torokhov <dtor@...are.com>, Andy King <acking@...are.com>, 
	netdev@...r.kernel.org
Subject: Re: [PATCH net 4/5] vsock/test: Add test for UAF due to socket
 unbinding

On Fri, Jan 17, 2025 at 10:59:44PM +0100, Michal Luczaj wrote:
>Fail the autobind, then trigger a transport reassign. Socket might get
>unbound from unbound_sockets, which then leads to a reference count
>underflow.
>
>Signed-off-by: Michal Luczaj <mhal@...x.co>
>---
> tools/testing/vsock/vsock_test.c | 67 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 67 insertions(+)
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 28a5083bbfd600cf84a1a85cec2f272ce6912dd3..7f1916e23858b5ba407c34742a05b7bd6cfdcc10 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -1458,6 +1458,68 @@ static void test_stream_cred_upd_on_set_rcvlowat(const struct test_opts *opts)
> 	test_stream_credit_update_test(opts, false);
> }
>
>+#define MAX_PORT_RETRIES	24	/* net/vmw_vsock/af_vsock.c */
>+#define VMADDR_CID_NONEXISTING	42

I would suggest a slightly bigger and weirder CID, this might actually
exist, (e.g. 4242424242)


>+
>+/* Test attempts to trigger a transport release for an unbound socket. This can
>+ * lead to a reference count mishandling.
>+ */
>+static void test_seqpacket_transport_uaf_client(const struct test_opts *opts)
>+{
>+	int sockets[MAX_PORT_RETRIES];
>+	struct sockaddr_vm addr;
>+	int s, i, alen;
>+
>+	s = vsock_bind(VMADDR_CID_LOCAL, VMADDR_PORT_ANY, SOCK_SEQPACKET);

In my suite this test failed because I have instances where I run it
without vsock_loopback loaded.

26 - connectible transport release use-after-free...bind: Cannot assign requested address

Is it important to use VMADDR_CID_LOCAL or can we use VMADDR_CID_ANY?

>+
>+	alen = sizeof(addr);
>+	if (getsockname(s, (struct sockaddr *)&addr, &alen)) {
>+		perror("getsockname");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	for (i = 0; i < MAX_PORT_RETRIES; ++i)
>+		sockets[i] = vsock_bind(VMADDR_CID_ANY, ++addr.svm_port,
>+					SOCK_SEQPACKET);
>+
>+	close(s);
>+	s = socket(AF_VSOCK, SOCK_SEQPACKET, 0);
>+	if (s < 0) {
>+		perror("socket");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	if (!connect(s, (struct sockaddr *)&addr, alen)) {
>+		fprintf(stderr, "Unexpected connect() #1 success\n");
>+		exit(EXIT_FAILURE);
>+	}
>+	/* connect() #1 failed: transport set, sk in unbound list. */
>+
>+	addr.svm_cid = VMADDR_CID_NONEXISTING;
>+	if (!connect(s, (struct sockaddr *)&addr, alen)) {
>+		fprintf(stderr, "Unexpected connect() #2 success\n");
>+		exit(EXIT_FAILURE);
>+	}
>+	/* connect() #2 failed: transport unset, sk ref dropped? */
>+
>+	addr.svm_cid = VMADDR_CID_LOCAL;

Ditto.

>+	addr.svm_port = VMADDR_PORT_ANY;
>+
>+	/* Vulnerable system may crash now. */
>+	bind(s, (struct sockaddr *)&addr, alen);

Should we check the return of this function or do we not care whether
it fails or not?

>+
>+	close(s);
>+	while (i--)
>+		close(sockets[i]);
>+
>+	control_writeln("DONE");
>+}
>+
>+static void test_seqpacket_transport_uaf_server(const struct test_opts *opts)
>+{
>+	control_expectln("DONE");
>+}
>+
> static struct test_case test_cases[] = {
> 	{
> 		.name = "SOCK_STREAM connection reset",
>@@ -1588,6 +1650,11 @@ static struct test_case test_cases[] = {
> 		.run_client = test_seqpacket_unsent_bytes_client,
> 		.run_server = test_seqpacket_unsent_bytes_server,
> 	},
>+	{
>+		.name = "connectible transport release use-after-free",

If it doesn't matter that it is SOCK_STREAM or SOCK_SEQPACKET, I would
rather test SOCK_STREAM because it is more common.

Anyway, here I would specify which of the two we are testing for
accordance.

"SOCK_STREAM transport release use-after-free".

Tanks for adding this test!
Stefano

>+		.run_client = test_seqpacket_transport_uaf_client,
>+		.run_server = test_seqpacket_transport_uaf_server,
>+	},
> 	{},
> };
>
>
>-- 
>2.47.1
>