Re: [LTP] [PATCH] Add setsockopt10 TLS ULP UAF CVE-2023-0461

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> +#include "tst_test.h"
>> +
>> +#ifdef HAVE_LINUX_TLS_H
>> +
>> +#include <linux/tls.h>
>> +#include "netinet/in.h"
>> +#include "netinet/tcp.h"
>> +#include "tst_checkpoint.h"
>> +#include "tst_net.h"
>> +#include "tst_safe_net.h"
>> +#include "tst_taint.h"
>> +
>> +static struct tls12_crypto_info_aes_gcm_128 opts = {
>> +	.info = {
>> +		.version = TLS_1_2_VERSION,
>> +		.cipher_type = TLS_CIPHER_AES_GCM_128,
>> +	},
>> +	.iv = { 'i', 'v' },
>> +	.key = { 'k', 'e', 'y' },
>> +	.salt = { 's', 'a', 'l', 't' },
>> +	.rec_seq = { 'r', 'e', 'c', 's' },
>> +};
>> +
>> +static struct sockaddr_in tcp0_addr, tcp1_addr;
>> +static const struct sockaddr unspec_addr = {
>> +	.sa_family = AF_UNSPEC
>> +};
>> +
>> +static int tcp0_sk, tcp1_sk, tcp2_sk, tcp3_sk;
>> +
>> +static void setup(void)
>> +{
>> +	tst_init_sockaddr_inet(&tcp0_addr, "127.0.0.1", 0x7c90);
>> +	tst_init_sockaddr_inet(&tcp1_addr, "127.0.0.1", 0x7c91);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	if (tcp0_sk > 0)
>> +		SAFE_CLOSE(tcp0_sk);
>> +	if (tcp1_sk > 0)
>> +		SAFE_CLOSE(tcp1_sk);
>> +	if (tcp2_sk > 0)
>> +		SAFE_CLOSE(tcp2_sk);
>> +	if (tcp3_sk > 0)
>> +		SAFE_CLOSE(tcp3_sk);
>> +}
>> +
>> +static void child(void)
>> +{
>> +	tst_res(TINFO, "child: Listen for tcp1 connection");
>> +	tcp0_sk = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
>> +	SAFE_BIND(tcp0_sk, (struct sockaddr *)&tcp0_addr, sizeof(tcp0_addr));
>> +	SAFE_LISTEN(tcp0_sk, 1);
>> +	TST_CHECKPOINT_WAKE(0);
>> +
>> +	tcp3_sk = SAFE_ACCEPT(tcp0_sk, NULL, 0);
>> +	TST_CHECKPOINT_WAIT(1);
>> +	SAFE_CLOSE(tcp3_sk);
>> +	SAFE_CLOSE(tcp0_sk);
>> +
>> +	tcp3_sk = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
>> +	TST_CHECKPOINT_WAIT(2);
>> +
>> +	tst_res(TINFO, "child: connect for tcp2 connection");
>> +	TEST(connect(tcp3_sk, (struct sockaddr *)&tcp1_addr, sizeof(tcp1_addr)));
>> +
>> +	if (TST_RET == -1) {
>> +		tst_res(TINFO | TTERRNO, "child: could not connect to tcp1");
>> +		return;
>> +	}
>> +
>> +	TST_CHECKPOINT_WAIT(3);
>> +}
>> +
>> +static void run(void)
>> +{
>> +	const pid_t child_pid = SAFE_FORK();
>> +
>> +	if (child_pid == 0) {
>> +		child();
>> +		return;
>> +	}
>> +
>> +	tcp1_sk = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
>> +	TST_CHECKPOINT_WAIT(0);
>> +
>> +	tst_res(TINFO, "parent: Connect for tcp0 connection");
>> +	SAFE_CONNECT(tcp1_sk, (struct sockaddr *)&tcp0_addr, sizeof(tcp0_addr));
>> +	TEST(setsockopt(tcp1_sk, SOL_TCP, TCP_ULP, "tls", 3));
>> +
>> +	if (TST_RET == -1 && TST_ERR == ENOENT)
>> +		tst_brk(TCONF | TTERRNO, "parent: setsockopt failed: The TLS module is probably not loaded");
>
> Should we set .needs_drivers for the test so that it only attempts to
> run either if TLS is compiled in or could be modprobed as a module?

Yes if it worked :-D, although I would still want to keep the above check. I
tried it and noticed a number of problems.

On NixOS:

$ ./setsockopt10
tst_kconfig.c:87: TINFO: Parsing kernel config '/proc/config.gz'
tst_kernel.c:110: TWARN: expected file /lib/modules/6.5.5/modules.dep does not exist or not a file
tst_kernel.c:110: TWARN: expected file /lib/modules/6.5.5/modules.builtin does not exist or not a file
tst_test.c:1195: TCONF: tls driver not available
$ lsmod | grep tls
tls                   151552  0

We have a special switch for Android which disables the check, but it is
not just Android. Granted NixOS is really wierd by desktop standards,
but we have ALP and embedded systems to think about.

AFAICT the test library does not do a modprobe if the driver is
missing. Some tests (e.g. zram, CAN) do that and then claim to require
modprobe. This is not good, those tests do not need modprobe if the
module is loaded or compiled in.

Perhaps if the check in tst_kernel fails for any reason we could just do
a modprobe (or use the configured kernel usermode helper if it is
set). If that fails we just carry on, like for Android. That's a
seperate patch though IMO. Once we have a solution for that, this test
can have needs_drivers added.

>
>> +	else if (TST_RET == -1)
>> +		tst_brk(TBROK | TTERRNO, "parent: setsockopt failed");
>> +
>> +	SAFE_SETSOCKOPT(tcp1_sk, SOL_TLS, TLS_TX, &opts, sizeof(opts));
>> +	TST_CHECKPOINT_WAKE(1);
>> +
>> +	tst_res(TINFO, "parent: Disconnect by setting unspec address");
>> +	SAFE_CONNECT(tcp1_sk, &unspec_addr, sizeof(unspec_addr));
>> +	SAFE_BIND(tcp1_sk, (struct sockaddr *)&tcp1_addr, sizeof(tcp1_addr));
>> +
>> +	TEST(listen(tcp1_sk, 1));
>> +
>> +	if (TST_RET == -1) {
>> +		if (TST_ERR == EINVAL)
>> +			tst_res(TPASS | TTERRNO, "parent: Can't listen on disconnected TLS socket");
>> +		else
>> + tst_res(TCONF | TTERRNO, "parent: Can't listen on disconnected TLS
>> socket, but the errno is not EINVAL as expected");
>> +
>> +		TST_CHECKPOINT_WAKE(2);
>> +		goto out;
>> +	}
>> +
>> +	tst_res(TINFO, "parent: Can listen on disconnected TLS socket");
>> +	TST_CHECKPOINT_WAKE(2);
>> +
>> +	tcp2_sk = SAFE_ACCEPT(tcp1_sk, NULL, 0);
>> +	SAFE_CLOSE(tcp2_sk);
>> +
>> + tst_res(TINFO, "parent: Attempting double free, because we set
>> cipher options this should result in an crash");
>> +	SAFE_CLOSE(tcp1_sk);
>> +
>> +	TST_CHECKPOINT_WAKE(3);
>> +	usleep(0);
>
> Did you forget this here?

It's supposed to give the kernel chance to propagate the taint or
panic. Which stops it from printing there was no kernel taint in
parallel with the kernel splat. It appears to work on my test system.

>
>> +	if (tst_taint_check())
>> +		tst_res(TFAIL, "Kernel is tainted");
>> +	else
>> +		tst_res(TCONF, "No kernel taint or crash, maybe the kernel can clone the TLS-ULP context now?");
>
> If you set up .taint_check this is going to be redundant since we print
> TFAIL in the test library in that case.

In that case we can unconditionally do TCONF and it'll be overridden by
fail and the resulting panic. Or it will be prevented by the panic.

>
>> +out:
>> +	tst_reap_children();
>> +}
>> +
>> +static struct tst_test test = {
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.test_all = run,
>> +	.forks_child = 1,
>> +	.needs_checkpoints = 1,
>> +	.taint_check = TST_TAINT_W | TST_TAINT_D,
>> +	.needs_kconfigs = (const char *[]) {
>> +		"CONFIG_TLS",
>> +		NULL
>> +	},
>> +	.tags = (const struct tst_tag[]) {
>> +		{"linux-git", "2c02d41d71f90"},
>> +		{"CVE", "2023-0461"},
>> +		{}
>> +	}
>> +};
>> +
>> +#else
>> +
>> +TST_TEST_TCONF("linux/tls.h missing, we assume your system is too old");
>> +
>> +#endif
>> -- 
>> 2.40.1
>> 
>> 
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp