Re: [LTP] [PATCH v3 4/4] Add test for CVE 2023-31248

Petr Vorel
2023-11-21T13:19:57Z

Hi Martin, Souta,

nice work, few very minor nits below.

> Fixes #1058

> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> Co-Developed-by: Souta Kawahara <souta.kawahara@miraclelinux.com>
> ---

> Changes since v1: New patch
> Changes since v2:
> - Use netfilter GOTO rule jumping into the deleted chain
> - Check for ENOENT error instead of kernel taint

> The test does not use any external utilities so I've decided not to add it
> to the net.tcp_cmds runfile.

Sure.

> runtest/cve | 1 +
> testcases/network/iptables/.gitignore | 1 +
> testcases/network/iptables/Makefile | 2 +-
> testcases/network/iptables/nft02.c | 213 ++++++++++++++++++++++++++
> 4 files changed, 216 insertions(+), 1 deletion(-)
> create mode 100644 testcases/network/iptables/.gitignore
> create mode 100644 testcases/network/iptables/nft02.c

> diff --git a/runtest/cve b/runtest/cve
> index 569558af2..1d1d87597 100644
> --- a/runtest/cve
> +++ b/runtest/cve
> @@ -86,6 +86,7 @@ cve-2022-2590 dirtyc0w_shmem
> cve-2022-23222 bpf_prog07
> cve-2023-1829 tcindex01
> cve-2023-0461 setsockopt10
> +cve-2023-31248 nft02
> # Tests below may cause kernel memory leak
> cve-2020-25704 perf_event_open03
> cve-2022-0185 fsconfig03
> diff --git a/testcases/network/iptables/.gitignore b/testcases/network/iptables/.gitignore
> new file mode 100644
> index 000000000..0f47a7313
> --- /dev/null
> +++ b/testcases/network/iptables/.gitignore
> @@ -0,0 +1 @@
> +nft02
> diff --git a/testcases/network/iptables/Makefile b/testcases/network/iptables/Makefile
> index 1b42f25db..02e228cbc 100644
> --- a/testcases/network/iptables/Makefile
> +++ b/testcases/network/iptables/Makefile
> @@ -5,7 +5,7 @@

> top_srcdir ?= ../../..

> -include $(top_srcdir)/include/mk/env_pre.mk
> +include $(top_srcdir)/include/mk/testcases.mk

> INSTALL_TARGETS := *.sh

> diff --git a/testcases/network/iptables/nft02.c b/testcases/network/iptables/nft02.c
> new file mode 100644
> index 000000000..a6e795af3
> --- /dev/null
> +++ b/testcases/network/iptables/nft02.c
> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 SUSE LLC
> + * Author: Marcos Paulo de Souza <mpdesouza@suse.com>
> + * LTP port: Martin Doucha <mdoucha@suse.cz>
> + */
> +
> +/*\
We usually add [Description] here. Although it looks bogus to me, I can add it
before merge.

> + * CVE-2023-31248
> + *
> + * Test for use-after-free when adding a new rule to a chain deleted
> + * in the same netlink message batch.
> + *
> + * Kernel bug fixed in:
> + *
> + * commit 515ad530795c118f012539ed76d02bacfd426d89
> + * Author: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> + * Date: Wed Jul 5 09:12:55 2023 -0300
> + *
> + * netfilter: nf_tables: do not ignore genmask when looking up chain by id
> + */
> +
> +#include <linux/netlink.h>
> +#include <linux/tcp.h>
> +#include <arpa/inet.h>
> +#include <linux/netfilter.h>
> +#include "lapi/nf_tables.h"
> +#include <linux/netfilter/nfnetlink.h>
> +#include "tst_test.h"
> +#include "tst_netlink.h"
> +
> +#define TABNAME "ltp_table1"
> +#define SRCCHAIN "ltp_chain_src"
> +#define DESTCHAIN "ltp_chain_dest"
> +
> +static uint32_t chain_id;
> +static uint32_t imm_dreg, imm_verdict;
> +static struct tst_netlink_context *ctx;
> +
> +/* Table creation config */
> +static const struct tst_netlink_attr_list table_config[] = {
> + {NFTA_TABLE_NAME, TABNAME, strlen(TABNAME) + 1, NULL},
> + {0, NULL, -1, NULL}
> +};
> +
> +/* Chain creation and deletion config */
> +static const struct tst_netlink_attr_list destchain_config[] = {
> + {NFTA_TABLE_NAME, TABNAME, strlen(TABNAME) + 1, NULL},
> + {NFTA_CHAIN_NAME, DESTCHAIN, strlen(DESTCHAIN) + 1, NULL},
> + {NFTA_CHAIN_ID, &chain_id, sizeof(chain_id), NULL},
> + {0, NULL, -1, NULL}
> +};
> +
> +static const struct tst_netlink_attr_list delchain_config[] = {
> + {NFTA_TABLE_NAME, TABNAME, strlen(TABNAME) + 1, NULL},
> + {NFTA_CHAIN_NAME, DESTCHAIN, strlen(DESTCHAIN) + 1, NULL},
> + {0, NULL, -1, NULL}
> +};
> +
> +static const struct tst_netlink_attr_list srcchain_config[] = {
> + {NFTA_TABLE_NAME, TABNAME, strlen(TABNAME) + 1, NULL},
> + {NFTA_CHAIN_NAME, SRCCHAIN, strlen(SRCCHAIN) + 1, NULL},
> + {0, NULL, -1, NULL}
> +};
> +
> +/* Rule creation config */
> +static const struct tst_netlink_attr_list rule_verdict_config[] = {
> + {NFTA_VERDICT_CODE, &imm_verdict, sizeof(imm_verdict), NULL},
> + {NFTA_VERDICT_CHAIN_ID, &chain_id, sizeof(chain_id), NULL},
> + {0, NULL, -1, NULL}
> +};
> +
> +static const struct tst_netlink_attr_list rule_data_config[] = {
> + {NFTA_IMMEDIATE_DREG, &imm_dreg, sizeof(imm_dreg), NULL},
> + {NFTA_IMMEDIATE_DATA, NULL, 0, (const struct tst_netlink_attr_list[]) {
> + {NFTA_DATA_VERDICT, NULL, 0, rule_verdict_config},
> + {0, NULL, -1, NULL}
> + }},
> + {0, NULL, -1, NULL}
> +};
> +
> +static const struct tst_netlink_attr_list rule_expr_config[] = {
> + {NFTA_LIST_ELEM, NULL, 0, (const struct tst_netlink_attr_list[]) {
> + {NFTA_EXPR_NAME, "immediate", 10, NULL},
> + {NFTA_EXPR_DATA, NULL, 0, rule_data_config},
> + {0, NULL, -1, NULL}
> + }},
> + {0, NULL, -1, NULL}
> +};
> +
> +static const struct tst_netlink_attr_list rule_config[] = {
> + {NFTA_RULE_EXPRESSIONS, NULL, 0, rule_expr_config},
> + {NFTA_RULE_TABLE, TABNAME, strlen(TABNAME) + 1, NULL},
> + {NFTA_RULE_CHAIN, SRCCHAIN, strlen(SRCCHAIN) + 1, NULL},
> + {0, NULL, -1, NULL}
> +};
> +
> +static void setup(void)
> +{
> + tst_setup_netns();
> +
> + chain_id = htonl(77);
nit: Although it's obvious that ID chain_id is some random number I would define
77 also above.

> + imm_dreg = htonl(NFT_REG_VERDICT);
> + imm_verdict = htonl(NFT_GOTO);
> +}
> +
> +static void run(void)
> +{
> + int ret;
> + struct nlmsghdr header;
> + struct nfgenmsg nfpayload;
> +
> + memset(&header, 0, sizeof(header));
> + memset(&nfpayload, 0, sizeof(nfpayload));
> + nfpayload.version = NFNETLINK_V0;
> +
> + ctx = NETLINK_CREATE_CONTEXT(NETLINK_NETFILTER);
> +
> + /* Start netfilter batch */
> + header.nlmsg_type = NFNL_MSG_BATCH_BEGIN;
> + header.nlmsg_flags = NLM_F_REQUEST;
> + nfpayload.nfgen_family = AF_UNSPEC;
> + nfpayload.res_id = htons(NFNL_SUBSYS_NFTABLES);
> + NETLINK_ADD_MESSAGE(ctx, &header, &nfpayload, sizeof(nfpayload));
> +
> + /* Add table */
> + header.nlmsg_type = (NFNL_SUBSYS_NFTABLES << 8) | NFT_MSG_NEWTABLE;
> + header.nlmsg_flags = NLM_F_REQUEST | NLM_F_CREATE;
> + nfpayload.nfgen_family = NFPROTO_IPV4;
> + nfpayload.res_id = htons(0);
> + NETLINK_ADD_MESSAGE(ctx, &header, &nfpayload, sizeof(nfpayload));
> + NETLINK_ADD_ATTR_LIST(ctx, table_config);
> +
> + /* Add destination chain */
> + header.nlmsg_type = (NFNL_SUBSYS_NFTABLES << 8) | NFT_MSG_NEWCHAIN;
> + header.nlmsg_flags = NLM_F_REQUEST | NLM_F_CREATE;
> + nfpayload.nfgen_family = NFPROTO_IPV4;
> + nfpayload.res_id = htons(0);
> + NETLINK_ADD_MESSAGE(ctx, &header, &nfpayload, sizeof(nfpayload));
> + NETLINK_ADD_ATTR_LIST(ctx, destchain_config);
> +
> + /* Delete destination chain */
> + header.nlmsg_type = (NFNL_SUBSYS_NFTABLES << 8) | NFT_MSG_DELCHAIN;
> + header.nlmsg_flags = NLM_F_REQUEST;
> + nfpayload.nfgen_family = NFPROTO_IPV4;
> + nfpayload.res_id = htons(0);
> + NETLINK_ADD_MESSAGE(ctx, &header, &nfpayload, sizeof(nfpayload));
> + NETLINK_ADD_ATTR_LIST(ctx, delchain_config);
> +
> + /* Add destination chain */
nit: this looks to be source chain
Out of curriosity I'm looking at the reproducer
(https://bugzilla.suse.com/attachment.cgi?id=868806)
and it needs just single chain.
But test needs both for some reason.
Anyway, nice work, kernel oops printed to dmesg on older kernel.

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Tested-by: Petr Vorel <pvorel@suse.cz>

> + header.nlmsg_type = (NFNL_SUBSYS_NFTABLES << 8) | NFT_MSG_NEWCHAIN;
> + header.nlmsg_flags = NLM_F_REQUEST | NLM_F_CREATE;
> + nfpayload.nfgen_family = NFPROTO_IPV4;
> + nfpayload.res_id = htons(0);
> + NETLINK_ADD_MESSAGE(ctx, &header, &nfpayload, sizeof(nfpayload));
> + NETLINK_ADD_ATTR_LIST(ctx, srcchain_config);
> +
> + /* Add rule to source chain. Require ACK and check for ENOENT error. */
> + header.nlmsg_type = (NFNL_SUBSYS_NFTABLES << 8) | NFT_MSG_NEWRULE;
> + header.nlmsg_flags = NLM_F_REQUEST | NLM_F_APPEND | NLM_F_CREATE |
> + NLM_F_ACK;
> + nfpayload.nfgen_family = NFPROTO_IPV4;
> + nfpayload.res_id = htons(0);
> + NETLINK_ADD_MESSAGE(ctx, &header, &nfpayload, sizeof(nfpayload));
> + NETLINK_ADD_ATTR_LIST(ctx, rule_config);
> +
> + /* End batch */
> + header.nlmsg_type = NFNL_MSG_BATCH_END;
> + header.nlmsg_flags = NLM_F_REQUEST;
> + nfpayload.nfgen_family = AF_UNSPEC;
> + nfpayload.res_id = htons(NFNL_SUBSYS_NFTABLES);
> + NETLINK_ADD_MESSAGE(ctx, &header, &nfpayload, sizeof(nfpayload));
> +
> + ret = NETLINK_SEND_VALIDATE(ctx);
> + TST_ERR = tst_netlink_errno;
> + NETLINK_DESTROY_CONTEXT(ctx);
> + ctx = NULL;
> +
> + if (ret)
> + tst_res(TFAIL, "Netfilter chain list is corrupted");
> + else if (TST_ERR == ENOENT)
> + tst_res(TPASS, "Deleted netfilter chain cannot be referenced");
> + else if (TST_ERR == EOPNOTSUPP || TST_ERR == EINVAL)
> + tst_brk(TCONF, "Test requires unavailable netfilter features");
> + else
> + tst_brk(TBROK | TTERRNO, "Unknown nfnetlink error");
> +}
> +
> +static void cleanup(void)
> +{
> + NETLINK_DESTROY_CONTEXT(ctx);
> +}
> +
> +static struct tst_test test = {
> + .test_all = run,
> + .setup = setup,
> + .cleanup = cleanup,
> + .taint_check = TST_TAINT_W | TST_TAINT_D,
> + .needs_kconfigs = (const char *[]) {
> + "CONFIG_USER_NS=y",
> + "CONFIG_NF_TABLES",
> + NULL
> + },
> + .save_restore = (const struct tst_path_val[]) {
> + {"/proc/sys/user/max_user_namespaces", "1024", TST_SR_SKIP},
Out of curiosity, why this?

CVE mentions "Exploiting it requires CAP_NET_ADMIN in any user or network
namespace.", but how is it related to changing max_user_namespaces value?

Also, vulnerable kernel reproducers with any max_user_namespaces value, or
without setting max_user_namespaces at all.

I can fix all the typos (only) before merge or you send v4 (whatever you prefer).

Kind regards,
Petr

> + {}
> + },
> + .tags = (const struct tst_tag[]) {
> + {"linux-git", "515ad530795c"},
> + {"CVE", "2023-31248"},
> + {}
> + }
> +};