more stable fixes for 4.14

PATCH In review Pablo Neira Ayuso 2 participants 11 messages May 16, 2023 – May 23, 2023

Subsystems (2)

Stable branch netfilter

lore

Share & tools

lore

Pablo Neira Ayuso

author maintainer

2023-05-16 15:16 UTC #0 lore raw

cover more stable fixes for 4.14

Hi Greg, Sasha,

This is second round of -stable backport fixes for 4.14. This batch includes dependency patches which are not currently in the 4.14 branch.

The following list shows the backported patches, I am using original commit IDs for reference:

1) 08a01c11a5bb ("netfilter: nftables: statify nft_parse_register()")

2) 6e1acfa387b9 ("netfilter: nf_tables: validate registers coming from userspace.")

3) 20a1452c3542 ("netfilter: nf_tables: add nft_setelem_parse_key()")

4) fdb9c405e35b ("netfilter: nf_tables: allow up to 64 bytes in the set element data area")

5) 7e6bc1f6cabc ("netfilter: nf_tables: stricter validation of element data")

6) 215a31f19ded ("netfilter: nft_dynset: do not reject set updates with NFT_SET_EVAL")

7) 36d5b2913219 ("netfilter: nf_tables: do not allow RULE_ID to refer to another chain")

8) 470ee20e069a ("netfilter: nf_tables: do not allow SET_ID to refer to another table")

Patches #1, #3 and #4 are dependencies.

Please, apply.
Thanks.

Pablo Neira Ayuso (8):
netfilter: nftables: statify nft_parse_register()
netfilter: nf_tables: validate registers coming from userspace.
netfilter: nf_tables: add nft_setelem_parse_key()
netfilter: nf_tables: allow up to 64 bytes in the set element data area
netfilter: nf_tables: stricter validation of element data
netfilter: nft_dynset: do not reject set updates with NFT_SET_EVAL
netfilter: nf_tables: do not allow RULE_ID to refer to another chain
netfilter: nf_tables: do not allow SET_ID to refer to another table

 include/net/netfilter/nf_tables.h        |   7 +-
 include/uapi/linux/netfilter/nf_tables.h |   2 +-
 net/netfilter/nf_tables_api.c            | 157 ++++++++++++++---------
 net/netfilter/nft_dynset.c               |   4 +-
 4 files changed, 104 insertions(+), 66 deletions(-)

Signature · 1 line

2.30.2

Pablo Neira Ayuso

author maintainer

2023-05-16 15:16 UTC #1 lore raw

PATCH 1/8 netfilter: nftables: statify nft_parse_register()

[ 08a01c11a5bb3de9b0a9c9b2685867e50eda9910 ]

This function is not used anymore by any extension, statify it.

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 include/net/netfilter/nf_tables.h | 1 -
 net/netfilter/nf_tables_api.c     | 3 +--
 2 files changed, 1 insertion(+), 3 deletions(-)

include/net/netfilter/nf_tables.h +0 -1

index e837bf96c6a3..8397f859ff70 100644
@@ -195,7 +195,6 @@ static inline enum nft_registers nft_type_to_reg(enum nft_data_types type)
 }
 
 int nft_parse_u32_check(const struct nlattr *attr, int max, u32 *dest);
-unsigned int nft_parse_register(const struct nlattr *attr);
 int nft_dump_register(struct sk_buff *skb, unsigned int attr, unsigned int reg);
 
 int nft_parse_register_load(const struct nlattr *attr, u8 *sreg, u32 len);

net/netfilter/nf_tables_api.c +1 -2

index 3adefb2a625c..95ca28a732c7 100644
@@ -5624,7 +5624,7 @@ EXPORT_SYMBOL_GPL(nft_parse_u32_check);
  *	Registers used to be 128 bit wide, these register numbers will be
  *	mapped to the corresponding 32 bit register numbers.
  */
-unsigned int nft_parse_register(const struct nlattr *attr)
+static unsigned int nft_parse_register(const struct nlattr *attr)
 {
 	unsigned int reg;

@@ -5636,7 +5636,6 @@ unsigned int nft_parse_register(const struct nlattr *attr)
 		return reg + NFT_REG_SIZE / NFT_REG32_SIZE - NFT_REG32_00;
 	}
 }
-EXPORT_SYMBOL_GPL(nft_parse_register);
 
 /**
  *	nft_dump_register - dump a register value to a netlink attribute

Signature · 1 line

2.30.2

Pablo Neira Ayuso

author maintainer

2023-05-16 15:16 UTC #2 lore raw

PATCH 2/8 netfilter: nf_tables: validate registers coming from userspace.

In reply to Pablo Neira Ayuso

[ 6e1acfa387b9ff82cfc7db8cc3b6959221a95851 ]

Bail out in case userspace uses unsupported registers.

Fixes 49499c3e6e18 ("netfilter: nf_tables: switch registers to 32 bit addressing")

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 net/netfilter/nf_tables_api.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

net/netfilter/nf_tables_api.c +17 ~~-14~~

index 95ca28a732c7..404e0484ddd0 100644
@@ -5615,26 +5615,23 @@ int nft_parse_u32_check(const struct nlattr *attr, int max, u32 *dest)
 }
 EXPORT_SYMBOL_GPL(nft_parse_u32_check);
 
-/**
- *	nft_parse_register - parse a register value from a netlink attribute
- *
- *	@attr: netlink attribute
- *
- *	Parse and translate a register value from a netlink attribute.
- *	Registers used to be 128 bit wide, these register numbers will be
- *	mapped to the corresponding 32 bit register numbers.
- */
-static unsigned int nft_parse_register(const struct nlattr *attr)
+static int nft_parse_register(const struct nlattr *attr, u32 *preg)
 {
 	unsigned int reg;
 
 	reg = ntohl(nla_get_be32(attr));
 	switch (reg) {
 	case NFT_REG_VERDICT...NFT_REG_4:
-		return reg * NFT_REG_SIZE / NFT_REG32_SIZE;
+		*preg = reg * NFT_REG_SIZE / NFT_REG32_SIZE;
+		break;
+	case NFT_REG32_00...NFT_REG32_15:
+		*preg = reg + NFT_REG_SIZE / NFT_REG32_SIZE - NFT_REG32_00;
+		break;
 	default:
-		return reg + NFT_REG_SIZE / NFT_REG32_SIZE - NFT_REG32_00;
+		return -ERANGE;
 	}
+
+	return 0;
 }
 
 /**

@@ -5685,7 +5682,10 @@ int nft_parse_register_load(const struct nlattr *attr, u8 *sreg, u32 len)
 	u32 reg;
 	int err;
 
-	reg = nft_parse_register(attr);
+	err = nft_parse_register(attr, &reg);
+	if (err < 0)
+		return err;
+
 	err = nft_validate_register_load(reg, len);
 	if (err < 0)
 		return err;

@@ -5761,7 +5761,10 @@ int nft_parse_register_store(const struct nft_ctx *ctx,
 	int err;
 	u32 reg;
 
-	reg = nft_parse_register(attr);
+	err = nft_parse_register(attr, &reg);
+	if (err < 0)
+		return err;
+
 	err = nft_validate_register_store(ctx, reg, data, type, len);
 	if (err < 0)
 		return err;

Signature · 1 line

2.30.2

Pablo Neira Ayuso

author maintainer

2023-05-16 15:16 UTC #3 lore raw

PATCH 3/8 netfilter: nf_tables: add nft_setelem_parse_key()

In reply to Pablo Neira Ayuso

[ 20a1452c35425b2cef76f21f8395ef069dfddfa9 ]

Add helper function to parse the set element key netlink attribute.

v4: No changes
v3: New patch

[sbrivio: refactor error paths and labels; use NFT_DATA_VALUE_MAXLEN
instead of sizeof(*key) in helper, value can be longer than that; rebase]

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 net/netfilter/nf_tables_api.c | 78 +++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 36 deletions(-)

net/netfilter/nf_tables_api.c +42 ~~-36~~

index 404e0484ddd0..c49e93c5db9c 100644
@@ -3736,6 +3736,24 @@ static int nf_tables_dump_set_done(struct netlink_callback *cb)
 	return 0;
 }
 
+static int nft_setelem_parse_key(struct nft_ctx *ctx, struct nft_set *set,
+				 struct nft_data *key, struct nlattr *attr)
+{
+	struct nft_data_desc desc;
+	int err;
+
+	err = nft_data_init(ctx, key, NFT_DATA_VALUE_MAXLEN, &desc, attr);
+	if (err < 0)
+		return err;
+
+	if (desc.type != NFT_DATA_VALUE || desc.len != set->klen) {
+		nft_data_release(key, desc.type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int nf_tables_getsetelem(struct net *net, struct sock *nlsk,
 				struct sk_buff *skb, const struct nlmsghdr *nlh,
 				const struct nlattr * const nla[],

@@ -3941,13 +3959,13 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 {
 	struct nlattr *nla[NFTA_SET_ELEM_MAX + 1];
 	u8 genmask = nft_genmask_next(ctx->net);
-	struct nft_data_desc d1, d2;
 	struct nft_set_ext_tmpl tmpl;
 	struct nft_set_ext *ext, *ext2;
 	struct nft_set_elem elem;
 	struct nft_set_binding *binding;
 	struct nft_object *obj = NULL;
 	struct nft_userdata *udata;
+	struct nft_data_desc desc;
 	struct nft_data data;
 	enum nft_registers dreg;
 	struct nft_trans *trans;

@@ -4000,15 +4018,12 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 		timeout = set->timeout;
 	}
 
-	err = nft_data_init(ctx, &elem.key.val, sizeof(elem.key), &d1,
-			    nla[NFTA_SET_ELEM_KEY]);
+	err = nft_setelem_parse_key(ctx, set, &elem.key.val,
+				    nla[NFTA_SET_ELEM_KEY]);
 	if (err < 0)
 		goto err1;
-	err = -EINVAL;
-	if (d1.type != NFT_DATA_VALUE || d1.len != set->klen)
-		goto err2;
 
-	nft_set_ext_add_length(&tmpl, NFT_SET_EXT_KEY, d1.len);
+	nft_set_ext_add_length(&tmpl, NFT_SET_EXT_KEY, set->klen);
 	if (timeout > 0) {
 		nft_set_ext_add(&tmpl, NFT_SET_EXT_EXPIRATION);
 		if (timeout != set->timeout)

@@ -4030,13 +4045,13 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	}
 
 	if (nla[NFTA_SET_ELEM_DATA] != NULL) {
-		err = nft_data_init(ctx, &data, sizeof(data), &d2,
+		err = nft_data_init(ctx, &data, sizeof(data), &desc,
 				    nla[NFTA_SET_ELEM_DATA]);
 		if (err < 0)
 			goto err2;
 
 		err = -EINVAL;
-		if (set->dtype != NFT_DATA_VERDICT && d2.len != set->dlen)
+		if (set->dtype != NFT_DATA_VERDICT && desc.len != set->dlen)
 			goto err3;
 
 		dreg = nft_type_to_reg(set->dtype);

@@ -4053,12 +4068,12 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 
 			err = nft_validate_register_store(&bind_ctx, dreg,
 							  &data,
-							  d2.type, d2.len);
+							  desc.type, desc.len);
 			if (err < 0)
 				goto err3;
 		}
 
-		nft_set_ext_add_length(&tmpl, NFT_SET_EXT_DATA, d2.len);
+		nft_set_ext_add_length(&tmpl, NFT_SET_EXT_DATA, desc.len);
 	}
 
 	/* The full maximum length of userdata can exceed the maximum

@@ -4141,9 +4156,9 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	kfree(elem.priv);
 err3:
 	if (nla[NFTA_SET_ELEM_DATA] != NULL)
-		nft_data_release(&data, d2.type);
+		nft_data_release(&data, desc.type);
 err2:
-	nft_data_release(&elem.key.val, d1.type);
+	nft_data_release(&elem.key.val, NFT_DATA_VALUE);
 err1:
 	return err;
 }

@@ -4241,7 +4256,6 @@ static int nft_del_setelem(struct nft_ctx *ctx, struct nft_set *set,
 {
 	struct nlattr *nla[NFTA_SET_ELEM_MAX + 1];
 	struct nft_set_ext_tmpl tmpl;
-	struct nft_data_desc desc;
 	struct nft_set_elem elem;
 	struct nft_set_ext *ext;
 	struct nft_trans *trans;

@@ -4252,11 +4266,10 @@ static int nft_del_setelem(struct nft_ctx *ctx, struct nft_set *set,
 	err = nla_parse_nested(nla, NFTA_SET_ELEM_MAX, attr,
 			       nft_set_elem_policy, NULL);
 	if (err < 0)
-		goto err1;
+		return err;
 
-	err = -EINVAL;
 	if (nla[NFTA_SET_ELEM_KEY] == NULL)
-		goto err1;
+		return -EINVAL;
 
 	nft_set_ext_prepare(&tmpl);

@@ -4266,37 +4279,31 @@ static int nft_del_setelem(struct nft_ctx *ctx, struct nft_set *set,
 	if (flags != 0)
 		nft_set_ext_add(&tmpl, NFT_SET_EXT_FLAGS);
 
-	err = nft_data_init(ctx, &elem.key.val, sizeof(elem.key), &desc,
-			    nla[NFTA_SET_ELEM_KEY]);
+	err = nft_setelem_parse_key(ctx, set, &elem.key.val,
+				    nla[NFTA_SET_ELEM_KEY]);
 	if (err < 0)
-		goto err1;
-
-	err = -EINVAL;
-	if (desc.type != NFT_DATA_VALUE || desc.len != set->klen)
-		goto err2;
+		return err;
 
-	nft_set_ext_add_length(&tmpl, NFT_SET_EXT_KEY, desc.len);
+	nft_set_ext_add_length(&tmpl, NFT_SET_EXT_KEY, set->klen);
 
 	err = -ENOMEM;
 	elem.priv = nft_set_elem_init(set, &tmpl, elem.key.val.data, NULL, 0,
 				      GFP_KERNEL);
 	if (elem.priv == NULL)
-		goto err2;
+		goto fail_elem;
 
 	ext = nft_set_elem_ext(set, elem.priv);
 	if (flags)
 		*nft_set_ext_flags(ext) = flags;
 
 	trans = nft_trans_elem_alloc(ctx, NFT_MSG_DELSETELEM, set);
-	if (trans == NULL) {
-		err = -ENOMEM;
-		goto err3;
-	}
+	if (trans == NULL)
+		goto fail_trans;
 
 	priv = set->ops->deactivate(ctx->net, set, &elem);
 	if (priv == NULL) {
 		err = -ENOENT;
-		goto err4;
+		goto fail_ops;
 	}
 	kfree(elem.priv);
 	elem.priv = priv;

@@ -4307,13 +4314,12 @@ static int nft_del_setelem(struct nft_ctx *ctx, struct nft_set *set,
 	list_add_tail(&trans->list, &ctx->net->nft.commit_list);
 	return 0;
 
-err4:
+fail_ops:
 	kfree(trans);
-err3:
+fail_trans:
 	kfree(elem.priv);
-err2:
-	nft_data_release(&elem.key.val, desc.type);
-err1:
+fail_elem:
+	nft_data_release(&elem.key.val, NFT_DATA_VALUE);
 	return err;
 }

Signature · 1 line

2.30.2

Pablo Neira Ayuso

author maintainer

2023-05-16 15:16 UTC #4 lore raw

PATCH 4/8 netfilter: nf_tables: allow up to 64 bytes in the set element data area

In reply to Pablo Neira Ayuso

[ fdb9c405e35bdc6e305b9b4e20ebc141ed14fc81 ]

So far, the set elements could store up to 128-bits in the data area.

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 include/net/netfilter/nf_tables.h |  4 ++++
 net/netfilter/nf_tables_api.c     | 35 ++++++++++++++++++++++---------
 2 files changed, 29 insertions(+), 10 deletions(-)

include/net/netfilter/nf_tables.h +4 -0

index 8397f859ff70..4eb90800fc2e 100644
@@ -229,6 +229,10 @@ struct nft_set_elem {
 		u32		buf[NFT_DATA_VALUE_MAXLEN / sizeof(u32)];
 		struct nft_data	val;
 	} key;
+	union {
+		u32		buf[NFT_DATA_VALUE_MAXLEN / sizeof(u32)];
+		struct nft_data val;
+	} data;
 	void			*priv;
 };

net/netfilter/nf_tables_api.c +25 ~~-10~~

index c49e93c5db9c..1cd49af7d375 100644
@@ -3954,6 +3954,25 @@ static int nft_setelem_parse_flags(const struct nft_set *set,
 	return 0;
 }
 
+static int nft_setelem_parse_data(struct nft_ctx *ctx, struct nft_set *set,
+				  struct nft_data_desc *desc,
+				  struct nft_data *data,
+				  struct nlattr *attr)
+{
+	int err;
+
+	err = nft_data_init(ctx, data, NFT_DATA_VALUE_MAXLEN, desc, attr);
+	if (err < 0)
+		return err;
+
+	if (desc->type != NFT_DATA_VERDICT && desc->len != set->dlen) {
+		nft_data_release(data, desc->type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 			    const struct nlattr *attr, u32 nlmsg_flags)
 {

@@ -3966,7 +3985,6 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	struct nft_object *obj = NULL;
 	struct nft_userdata *udata;
 	struct nft_data_desc desc;
-	struct nft_data data;
 	enum nft_registers dreg;
 	struct nft_trans *trans;
 	u32 flags = 0;

@@ -4045,15 +4063,11 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	}
 
 	if (nla[NFTA_SET_ELEM_DATA] != NULL) {
-		err = nft_data_init(ctx, &data, sizeof(data), &desc,
-				    nla[NFTA_SET_ELEM_DATA]);
+		err = nft_setelem_parse_data(ctx, set, &desc, &elem.data.val,
+					     nla[NFTA_SET_ELEM_DATA]);
 		if (err < 0)
 			goto err2;
 
-		err = -EINVAL;
-		if (set->dtype != NFT_DATA_VERDICT && desc.len != set->dlen)
-			goto err3;
-
 		dreg = nft_type_to_reg(set->dtype);
 		list_for_each_entry(binding, &set->bindings, list) {
 			struct nft_ctx bind_ctx = {

@@ -4067,7 +4081,7 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 				continue;
 
 			err = nft_validate_register_store(&bind_ctx, dreg,
-							  &data,
+							  &elem.data.val,
 							  desc.type, desc.len);
 			if (err < 0)
 				goto err3;

@@ -4089,7 +4103,8 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	}
 
 	err = -ENOMEM;
-	elem.priv = nft_set_elem_init(set, &tmpl, elem.key.val.data, data.data,
+	elem.priv = nft_set_elem_init(set, &tmpl, elem.key.val.data,
+				      elem.data.val.data,
 				      timeout, GFP_KERNEL);
 	if (elem.priv == NULL)
 		goto err3;

@@ -4156,7 +4171,7 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	kfree(elem.priv);
 err3:
 	if (nla[NFTA_SET_ELEM_DATA] != NULL)
-		nft_data_release(&data, desc.type);
+		nft_data_release(&elem.data.val, desc.type);
 err2:
 	nft_data_release(&elem.key.val, NFT_DATA_VALUE);
 err1:

Signature · 1 line

2.30.2

Pablo Neira Ayuso

author maintainer

2023-05-16 15:16 UTC #5 lore raw

PATCH 5/8 netfilter: nf_tables: stricter validation of element data

In reply to Pablo Neira Ayuso

[ 7e6bc1f6cabcd30aba0b11219d8e01b952eacbb6 ]

Make sure element data type and length do not mismatch the one specified by the set declaration.

Fixes 7d7402642eaf ("netfilter: nf_tables: variable sized set element keys / data")

Reported-by Hugues ANGUELKOV <[email protected]>

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 net/netfilter/nf_tables_api.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

net/netfilter/nf_tables_api.c +8 -1

index 1cd49af7d375..9caaac459ca9 100644
@@ -3959,13 +3959,20 @@ static int nft_setelem_parse_data(struct nft_ctx *ctx, struct nft_set *set,
 				  struct nft_data *data,
 				  struct nlattr *attr)
 {
+	u32 dtype;
 	int err;
 
 	err = nft_data_init(ctx, data, NFT_DATA_VALUE_MAXLEN, desc, attr);
 	if (err < 0)
 		return err;
 
-	if (desc->type != NFT_DATA_VERDICT && desc->len != set->dlen) {
+	if (set->dtype == NFT_DATA_VERDICT)
+		dtype = NFT_DATA_VERDICT;
+	else
+		dtype = NFT_DATA_VALUE;
+
+	if (dtype != desc->type ||
+	    set->dlen != desc->len) {
 		nft_data_release(data, desc->type);
 		return -EINVAL;
 	}

Signature · 1 line

2.30.2

Pablo Neira Ayuso

author maintainer

2023-05-16 15:16 UTC #6 lore raw

PATCH 6/8 netfilter: nft_dynset: do not reject set updates with NFT_SET_EVAL

In reply to Pablo Neira Ayuso

[ 215a31f19dedd4e92a67cf5a9717ee898d012b3a ]

NFT_SET_EVAL is signalling the kernel that this sets can be updated from the evaluation path, even if there are no expressions attached to the element. Otherwise, set updates with no expressions fail. Update description to describe the right semantics.

Fixes 22fe54d5fefc ("netfilter: nf_tables: add support for dynamic set updates")

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 include/uapi/linux/netfilter/nf_tables.h | 2 +-
 net/netfilter/nft_dynset.c               | 4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)

include/uapi/linux/netfilter/nf_tables.h +1 -1

index 49b6997c3255..c7bb18ea4962 100644
@@ -258,7 +258,7 @@ enum nft_rule_compat_attributes {
  * @NFT_SET_INTERVAL: set contains intervals
  * @NFT_SET_MAP: set is used as a dictionary
  * @NFT_SET_TIMEOUT: set uses timeouts
- * @NFT_SET_EVAL: set contains expressions for evaluation
+ * @NFT_SET_EVAL: set can be updated from the evaluation path
  * @NFT_SET_OBJECT: set contains stateful objects
  */
 enum nft_set_flags {

net/netfilter/nft_dynset.c +1 -3

index f174a66bbc4b..d1dc5c8937a5 100644
@@ -190,9 +190,7 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
 		priv->expr = nft_expr_init(ctx, tb[NFTA_DYNSET_EXPR]);
 		if (IS_ERR(priv->expr))
 			return PTR_ERR(priv->expr);
-
-	} else if (set->flags & NFT_SET_EVAL)
-		return -EINVAL;
+	}
 
 	nft_set_ext_prepare(&priv->tmpl);
 	nft_set_ext_add_length(&priv->tmpl, NFT_SET_EXT_KEY, set->klen);

Signature · 1 line

2.30.2

Pablo Neira Ayuso

author maintainer

2023-05-16 15:16 UTC #7 lore raw

PATCH 7/8 netfilter: nf_tables: do not allow RULE_ID to refer to another chain

In reply to Pablo Neira Ayuso

[ 36d5b2913219ac853908b0f1c664345e04313856 ]

When doing lookups for rules on the same batch by using its ID, a rule from a different chain can be used. If a rule is added to a chain but tries to be positioned next to a rule from a different chain, it will be linked to chain2, but the use counter on chain1 would be the one to be incremented.

When looking for rules by ID, use the chain that was used for the lookup by name. The chain used in the context copied to the transaction needs to match that same chain. That way, struct nft_rule does not need to get enlarged with another member.

Fixes 1a94e38d254b ("netfilter: nf_tables: add NFTA_RULE_ID attribute")

Fixes 75dd48e2e420 ("netfilter: nf_tables: Support RULE_ID reference in new rule")

Signed-off-by Thadeu Lima de Souza Cascardo <[email protected]>

Cc <[email protected]>

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 net/netfilter/nf_tables_api.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

net/netfilter/nf_tables_api.c +3 -1

index 9caaac459ca9..86913d53eead 100644
@@ -2475,6 +2475,7 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk,
 }
 
 static struct nft_rule *nft_rule_lookup_byid(const struct net *net,
+					     const struct nft_chain *chain,
 					     const struct nlattr *nla)
 {
 	u32 id = ntohl(nla_get_be32(nla));

@@ -2484,6 +2485,7 @@ static struct nft_rule *nft_rule_lookup_byid(const struct net *net,
 		struct nft_rule *rule = nft_trans_rule(trans);
 
 		if (trans->msg_type == NFT_MSG_NEWRULE &&
+		    trans->ctx.chain == chain &&
 		    id == nft_trans_rule_id(trans))
 			return rule;
 	}

@@ -2530,7 +2532,7 @@ static int nf_tables_delrule(struct net *net, struct sock *nlsk,
 
 			err = nft_delrule(&ctx, rule);
 		} else if (nla[NFTA_RULE_ID]) {
-			rule = nft_rule_lookup_byid(net, nla[NFTA_RULE_ID]);
+			rule = nft_rule_lookup_byid(net, chain, nla[NFTA_RULE_ID]);
 			if (IS_ERR(rule))
 				return PTR_ERR(rule);

Signature · 1 line

2.30.2

Pablo Neira Ayuso

author maintainer

2023-05-16 15:16 UTC #8 lore raw

PATCH 8/8 netfilter: nf_tables: do not allow SET_ID to refer to another table

In reply to Pablo Neira Ayuso

[ 470ee20e069a6d05ae549f7d0ef2bdbcee6a81b2 ]

When doing lookups for sets on the same batch by using its ID, a set from a different table can be used.

Then, when the table is removed, a reference to the set may be kept after the set is freed, leading to a potential use-after-free.

When looking for sets by ID, use the table that was used for the lookup by name, and only return sets belonging to that same table.

This fixes CVE-2022-2586, also reported as ZDI-CAN-17470.

Reported-by Team Orca of Sea Security (@seasecresponse)

Fixes 958bee14d071 ("netfilter: nf_tables: use new transaction infrastructure to handle sets")

Signed-off-by Thadeu Lima de Souza Cascardo <[email protected]>

Cc <[email protected]>

Signed-off-by Pablo Neira Ayuso <[email protected]>

---
 include/net/netfilter/nf_tables.h | 2 ++
 net/netfilter/nf_tables_api.c     | 7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

include/net/netfilter/nf_tables.h +2 -0

index 4eb90800fc2e..0d625ff7841a 100644
@@ -381,6 +381,7 @@ void nft_unregister_set(struct nft_set_type *type);
  *
  *	@list: table set list node
  *	@bindings: list of set bindings
+ *	@table: table this set belongs to
  * 	@name: name of the set
  * 	@ktype: key type (numeric type defined by userspace, not used in the kernel)
  * 	@dtype: data type (verdict or numeric type defined by userspace)

@@ -404,6 +405,7 @@ void nft_unregister_set(struct nft_set_type *type);
 struct nft_set {
 	struct list_head		list;
 	struct list_head		bindings;
+	struct nft_table		*table;
 	char				*name;
 	u32				ktype;
 	u32				dtype;

net/netfilter/nf_tables_api.c +5 -2

index 86913d53eead..345fa29f34b9 100644
@@ -2746,6 +2746,7 @@ static struct nft_set *nf_tables_set_lookup(const struct nft_table *table,
 }
 
 static struct nft_set *nf_tables_set_lookup_byid(const struct net *net,
+						 const struct nft_table *table,
 						 const struct nlattr *nla,
 						 u8 genmask)
 {

@@ -2757,6 +2758,7 @@ static struct nft_set *nf_tables_set_lookup_byid(const struct net *net,
 			struct nft_set *set = nft_trans_set(trans);
 
 			if (id == nft_trans_set_id(trans) &&
+			    set->table == table &&
 			    nft_active_genmask(set, genmask))
 				return set;
 		}

@@ -2777,7 +2779,7 @@ struct nft_set *nft_set_lookup(const struct net *net,
 		if (!nla_set_id)
 			return set;
 
-		set = nf_tables_set_lookup_byid(net, nla_set_id, genmask);
+		set = nf_tables_set_lookup_byid(net, table, nla_set_id, genmask);
 	}
 	return set;
 }

@@ -3272,6 +3274,7 @@ static int nf_tables_newset(struct net *net, struct sock *nlsk,
 	}
 
 	INIT_LIST_HEAD(&set->bindings);
+	set->table = table;
 	set->ops   = ops;
 	set->ktype = ktype;
 	set->klen  = desc.klen;

@@ -4209,7 +4212,7 @@ static int nf_tables_newsetelem(struct net *net, struct sock *nlsk,
 				   genmask);
 	if (IS_ERR(set)) {
 		if (nla[NFTA_SET_ELEM_LIST_SET_ID]) {
-			set = nf_tables_set_lookup_byid(net,
+			set = nf_tables_set_lookup_byid(net, ctx.table,
 					nla[NFTA_SET_ELEM_LIST_SET_ID],
 					genmask);
 		}

Signature · 1 line

2.30.2

Sasha Levin

maintainer

2023-05-22 17:55 UTC #9 lore raw

In reply to Pablo Neira Ayuso

Quoted 16 lines · Pablo Neira Ayuso

Hi Greg, Sasha,
This is second round of -stable backport fixes for 4.14. This batch includes dependency patches which are not currently in the 4.14 branch.
The following list shows the backported patches, I am using original commit IDs for reference:
1) 08a01c11a5bb ("netfilter: nftables: statify nft_parse_register()")
2) 6e1acfa387b9 ("netfilter: nf_tables: validate registers coming from userspace.")
3) 20a1452c3542 ("netfilter: nf_tables: add nft_setelem_parse_key()")
4) fdb9c405e35b ("netfilter: nf_tables: allow up to 64 bytes in the set element data area")

Quoted 7 lines

5) 7e6bc1f6cabc ("netfilter: nf_tables: stricter validation of element data")
6) 215a31f19ded ("netfilter: nft_dynset: do not reject set updates with NFT_SET_EVAL")
7) 36d5b2913219 ("netfilter: nf_tables: do not allow RULE_ID to refer to another chain")
8) 470ee20e069a ("netfilter: nf_tables: do not allow SET_ID to refer to another table")

I've applied the 5.4 and 4.19 series, but it looks like patch #1 here fails to apply. Could you please re-send the 4.14 series?

Signature · 2 lines

Thanks,
Sasha

Pablo Neira Ayuso

author maintainer

2023-05-23 08:42 UTC #10 lore raw

Quoted 19 lines · Sasha Levin

Quoted 18 lines · Pablo Neira Ayuso
Hi Greg, Sasha,
This is second round of -stable backport fixes for 4.14. This batch includes dependency patches which are not currently in the 4.14 branch.
The following list shows the backported patches, I am using original commit IDs for reference:
1) 08a01c11a5bb ("netfilter: nftables: statify nft_parse_register()")
2) 6e1acfa387b9 ("netfilter: nf_tables: validate registers coming from userspace.")
3) 20a1452c3542 ("netfilter: nf_tables: add nft_setelem_parse_key()")
4) fdb9c405e35b ("netfilter: nf_tables: allow up to 64 bytes in the set element data area")
5) 7e6bc1f6cabc ("netfilter: nf_tables: stricter validation of element data")

Quoted 8 lines

Quoted 5 lines
6) 215a31f19ded ("netfilter: nft_dynset: do not reject set updates with NFT_SET_EVAL")
7) 36d5b2913219 ("netfilter: nf_tables: do not allow RULE_ID to refer to another chain")
8) 470ee20e069a ("netfilter: nf_tables: do not allow SET_ID to refer to another table")
I've applied the 5.4 and 4.19 series, but it looks like patch #1 here fails to apply. Could you please re-send the 4.14 series?

Sure, I'll rebase and resend v2. Thanks.