linux-kernel - Re: [PATCH v2] net/tls: support maximum record size limit

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 for Android: free password hash cracker in your pocket

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

Message-ID: <aLf6j73xSGGLAhQv@krikkit>
Date: Wed, 3 Sep 2025 10:21:35 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Wilfred Mallawa <wilfred.mallawa@....com>
Cc: "corbet@....net" <corbet@....net>,
	"dlemoal@...nel.org" <dlemoal@...nel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"john.fastabend@...il.com" <john.fastabend@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Alistair Francis <Alistair.Francis@....com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"horms@...nel.org" <horms@...nel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v2] net/tls: support maximum record size limit

2025-09-02, 22:50:53 +0000, Wilfred Mallawa wrote:
> On Tue, 2025-09-02 at 18:07 +0200, Sabrina Dubroca wrote:
> > 2025-09-02, 13:38:10 +1000, Wilfred Mallawa wrote:
> > > From: Wilfred Mallawa <wilfred.mallawa@....com>
> Hey Sabrina,
> > A selftest would be nice (tools/testing/selftests/net/tls.c), but I'm
> > not sure what we could do on the "RX" side to check that we are
> > respecting the size restriction. Use a basic TCP socket and try to
> > parse (and then discard without decrypting) records manually out of
> > the stream and see if we got the length we wanted?
> > 
> So far I have just been using an NVMe TCP Target with TLS enabled and
> checking that the targets RX record sizes are <= negotiated size in
> tls_rx_one_record(). I didn't check for this patch and the bug below
> got through...my bad!
> 
> Is it possible to get the exact record length into the testing layer?

Not really, unless we come up with some mechanism using probes. I
wouldn't go that route unless we don't have any other choice.

> Wouldn't the socket just return N bytes received which doesn't
> necessarily correlate to a record size?

Yes. That's why I suggested only using ktls on one side of the test,
and parsing the records out of the raw stream of bytes on the RX side.

Actually, control records don't get aggregated on read, so sending a
large non-data buffer should result in separate limit-sized reads. But
this makes me wonder if this limit is supposed to apply to control
records, and how the userspace library/application is supposed to deal
with the possible splitting of those records?


Here's a rough example of what I had in mind. The hardcoded cipher
overhead is a bit ugly but I don't see a way around it. Sanity check
at the end is probably not needed. I didn't write the loop because I
haven't had enough coffee yet to get that right :)


TEST(tx_record_size)
{
	struct tls_crypto_info_keys tls12;
	int cfd, ret, fd, len, overhead;
	char buf[1000], buf2[2000];
	__u16 limit = 100;
	bool notls;

	tls_crypto_info_init(TLS_1_2_VERSION, TLS_CIPHER_AES_CCM_128,
			     &tls12, 0);

	ulp_sock_pair(_metadata, &fd, &cfd, &notls);

	if (notls)
		exit(KSFT_SKIP);

	/* Don't install keys on fd, we'll parse raw records */
	ret = setsockopt(cfd, SOL_TLS, TLS_TX, &tls12, tls12.len);
	ASSERT_EQ(ret, 0);

	ret = setsockopt(cfd, SOL_TLS, TLS_TX_RECORD_SIZE_LIM, &limit, sizeof(limit));
	ASSERT_EQ(ret, 0);

	EXPECT_EQ(send(cfd, buf, sizeof(buf), 0), sizeof(buf));
	close(cfd);

	ret = recv(fd, buf2, sizeof(buf2), 0);
	memcpy(&len, buf2 + 3, 2);
	len = htons(len);

	/* 16B tag + 8B IV -- record header (5B) is not counted but we'll need it to walk the record stream */
	overhead = 16 + 8;

	// TODO should be <= limit since we may not have filled every
	// record (especially the last one), and loop over all the
	// records we got
	// next record starts at buf2 + (limit + overhead + 5)
	ASSERT_EQ(len, limit + overhead);
	/* sanity check that it's a TLS header for application data */
	ASSERT_EQ(buf2[0], 23);
	ASSERT_EQ(buf2[1], 0x3);
	ASSERT_EQ(buf2[2], 0x3);

	close(fd);
}


-- 
Sabrina