Skip to contentSkip to conversation

Netfilter fixes for net

PATCH In review Pablo Neira Ayuso 2 participants 7 messages Mar 12, 2018

Subsystems (4)
lore
Share & tools
lore

Pablo Neira Ayuso

author maintainer
#0 lore raw

cover Netfilter fixes for net

Hi David,

The following patchset contains Netfilter fixes for your net tree, they are:

1) Fixed hashtable representation doesn't support timeout flag, skip it otherwise rules to add elements from the packet fail bogusly fail with EOPNOTSUPP.

2) Fix bogus error with 32-bits ebtables userspace and 64-bits kernel, patch from Florian Westphal.

3) Sanitize proc names in several x_tables extensions, also from Florian.

4) Add sanitization to ebt_among wormhash logic, from Florian.

5) Missing release of hook array in flowtable.

You can pull these changes from:

git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git

Thanks!

----------------------------------------------------------------

The following changes since commit ce380619fab99036f5e745c7a865b21c59f005f6:

Merge tag 'please-pull-ia64_misc' of git://git.kernel.org/pub/scm/linux/kernel/git/aegl/linux (2018-03-05 20:31:14 -0800)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git HEAD

for you to fetch changes up to c04a3f730021c304c7cc4bc30ee57ee70ad98d57:

netfilter: nf_tables: release flowtable hooks (2018-03-11 21:24:56 +0100)

----------------------------------------------------------------
Florian Westphal (3):
netfilter: ebtables: fix erroneous reject of last rule
netfilter: x_tables: add and use xt_check_proc_name
netfilter: bridge: ebt_among: add more missing match size checks

Pablo Neira Ayuso (2):
netfilter: nft_set_hash: skip fixed hash if timeout is specified
netfilter: nf_tables: release flowtable hooks

 include/linux/netfilter/x_tables.h |  2 ++
 net/bridge/netfilter/ebt_among.c   | 34 ++++++++++++++++++++++++++++++++++
 net/bridge/netfilter/ebtables.c    |  6 +++++-
 net/netfilter/nf_tables_api.c      |  1 +
 net/netfilter/nft_set_hash.c       |  2 +-
 net/netfilter/x_tables.c           | 30 ++++++++++++++++++++++++++++++
 net/netfilter/xt_hashlimit.c       | 16 ++++++++++------
 net/netfilter/xt_recent.c          |  6 +++---
 8 files changed, 86 insertions(+), 11 deletions(-)
author maintainer
#1 lore raw

PATCH 1/5 netfilter: nft_set_hash: skip fixed hash if timeout is specified

Fixed hash supports to timeouts, so skip it. Otherwise, userspace hits EOPNOTSUPP.

Fixes 6c03ae210ce3 ("netfilter: nft_set_hash: add non-resizable hashtable implementation")
---
 net/netfilter/nft_set_hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/netfilter/nft_set_hash.c +1 -1
index 3f1624ee056f..d40591fe1b2f 100644
@@ -674,7 +674,7 @@ static const struct nft_set_ops *
 nft_hash_select_ops(const struct nft_ctx *ctx, const struct nft_set_desc *desc,
 		    u32 flags)
 {
-	if (desc->size) {
+	if (desc->size && !(flags & NFT_SET_TIMEOUT)) {
 		switch (desc->klen) {
 		case 4:
 			return &nft_hash_fast_ops;
Signature · 1 line
2.11.0
author maintainer
#2 lore raw

PATCH 2/5 netfilter: ebtables: fix erroneous reject of last rule

In reply to Pablo Neira Ayuso

From: Florian Westphal <[email protected]>

The last rule in the blob has next_entry offset that is same as total size. This made "ebtables32 -A OUTPUT -d de:ad:be:ef:01:02" fail on 64 bit kernel.

Fixes b71812168571fa ("netfilter: ebtables: CONFIG_COMPAT: don't trust userland offsets")
---
 net/bridge/netfilter/ebtables.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
net/bridge/netfilter/ebtables.c +5 -1
index 254ef9f49567..a94d23b0a9af 100644
@@ -2119,8 +2119,12 @@ static int size_entry_mwt(struct ebt_entry *entry, const unsigned char *base,
 	 * offsets are relative to beginning of struct ebt_entry (i.e., 0).
 	 */
 	for (i = 0; i < 4 ; ++i) {
-		if (offsets[i] >= *total)
+		if (offsets[i] > *total)
 			return -EINVAL;
+
+		if (i < 3 && offsets[i] == *total)
+			return -EINVAL;
+
 		if (i == 0)
 			continue;
 		if (offsets[i-1] > offsets[i])
Signature · 1 line
2.11.0
author maintainer
#3 lore raw

PATCH 3/5 netfilter: x_tables: add and use xt_check_proc_name

In reply to Pablo Neira Ayuso

From: Florian Westphal <[email protected]>

recent and hashlimit both create /proc files, but only check that name is 0 terminated.

This can trigger WARN() from procfs when name is "" or "/". Add helper for this and then use it for both.

---
 include/linux/netfilter/x_tables.h |  2 ++
 net/netfilter/x_tables.c           | 30 ++++++++++++++++++++++++++++++
 net/netfilter/xt_hashlimit.c       | 16 ++++++++++------
 net/netfilter/xt_recent.c          |  6 +++---
 4 files changed, 45 insertions(+), 9 deletions(-)
include/linux/netfilter/x_tables.h +2 -0
index 1313b35c3ab7..14529511c4b8 100644
@@ -285,6 +285,8 @@ unsigned int *xt_alloc_entry_offsets(unsigned int size);
 bool xt_find_jump_offset(const unsigned int *offsets,
 			 unsigned int target, unsigned int size);
 
+int xt_check_proc_name(const char *name, unsigned int size);
+
 int xt_check_match(struct xt_mtchk_param *, unsigned int size, u_int8_t proto,
 		   bool inv_proto);
 int xt_check_target(struct xt_tgchk_param *, unsigned int size, u_int8_t proto,
net/netfilter/x_tables.c +30 -0
index fa1655aff8d3..4aa01c90e9d1 100644
@@ -423,6 +423,36 @@ textify_hooks(char *buf, size_t size, unsigned int mask, uint8_t nfproto)
 	return buf;
 }
 
+/**
+ * xt_check_proc_name - check that name is suitable for /proc file creation
+ *
+ * @name: file name candidate
+ * @size: length of buffer
+ *
+ * some x_tables modules wish to create a file in /proc.
+ * This function makes sure that the name is suitable for this
+ * purpose, it checks that name is NUL terminated and isn't a 'special'
+ * name, like "..".
+ *
+ * returns negative number on error or 0 if name is useable.
+ */
+int xt_check_proc_name(const char *name, unsigned int size)
+{
+	if (name[0] == '\0')
+		return -EINVAL;
+
+	if (strnlen(name, size) == size)
+		return -ENAMETOOLONG;
+
+	if (strcmp(name, ".") == 0 ||
+	    strcmp(name, "..") == 0 ||
+	    strchr(name, '/'))
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL(xt_check_proc_name);
+
 int xt_check_match(struct xt_mtchk_param *par,
 		   unsigned int size, u_int8_t proto, bool inv_proto)
 {
net/netfilter/xt_hashlimit.c +10 -6
index 66f5aca62a08..3360f13dc208 100644
@@ -917,8 +917,9 @@ static int hashlimit_mt_check_v1(const struct xt_mtchk_param *par)
 	struct hashlimit_cfg3 cfg = {};
 	int ret;
 
-	if (info->name[sizeof(info->name) - 1] != '\0')
-		return -EINVAL;
+	ret = xt_check_proc_name(info->name, sizeof(info->name));
+	if (ret)
+		return ret;
 
 	ret = cfg_copy(&cfg, (void *)&info->cfg, 1);
 
@@ -935,8 +936,9 @@ static int hashlimit_mt_check_v2(const struct xt_mtchk_param *par)
 	struct hashlimit_cfg3 cfg = {};
 	int ret;
 
-	if (info->name[sizeof(info->name) - 1] != '\0')
-		return -EINVAL;
+	ret = xt_check_proc_name(info->name, sizeof(info->name));
+	if (ret)
+		return ret;
 
 	ret = cfg_copy(&cfg, (void *)&info->cfg, 2);
 
@@ -950,9 +952,11 @@ static int hashlimit_mt_check_v2(const struct xt_mtchk_param *par)
 static int hashlimit_mt_check(const struct xt_mtchk_param *par)
 {
 	struct xt_hashlimit_mtinfo3 *info = par->matchinfo;
+	int ret;
 
-	if (info->name[sizeof(info->name) - 1] != '\0')
-		return -EINVAL;
+	ret = xt_check_proc_name(info->name, sizeof(info->name));
+	if (ret)
+		return ret;
 
 	return hashlimit_mt_check_common(par, &info->hinfo, &info->cfg,
 					 info->name, 3);
net/netfilter/xt_recent.c +3 -3
index 6d232d18faff..81ee1d6543b2 100644
@@ -361,9 +361,9 @@ static int recent_mt_check(const struct xt_mtchk_param *par,
 				    info->hit_count, XT_RECENT_MAX_NSTAMPS - 1);
 		return -EINVAL;
 	}
-	if (info->name[0] == '\0' ||
-	    strnlen(info->name, XT_RECENT_NAME_LEN) == XT_RECENT_NAME_LEN)
-		return -EINVAL;
+	ret = xt_check_proc_name(info->name, sizeof(info->name));
+	if (ret)
+		return ret;
 
 	if (ip_pkt_list_tot && info->hit_count < ip_pkt_list_tot)
 		nstamp_mask = roundup_pow_of_two(ip_pkt_list_tot) - 1;
Signature · 1 line
2.11.0
author maintainer
#4 lore raw

PATCH 4/5 netfilter: bridge: ebt_among: add more missing match size checks

In reply to Pablo Neira Ayuso

From: Florian Westphal <[email protected]>

ebt_among is special, it has a dynamic match size and is exempt from the central size checks.

commit c4585a2823edf ("bridge: ebt_among: add missing match size checks") added validation for pool size, but missed fact that the macros ebt_among_wh_src/dst can already return out-of-bound result because they do not check value of wh_src/dst_ofs (an offset) vs. the size of the match that userspace gave to us.

v2:
check that offset has correct alignment.
Paolo Abeni points out that we should also check that src/dst wormhash arrays do not overlap, and src + length lines up with start of dst (or vice versa).
v3: compact wormhash_sizes_valid() part

NB: Fixes tag is intentionally wrong, this bug exists from day one when match was added for 2.6 kernel. Tag is there so stable maintainers will notice this one too.

Tested with same rules from the earlier patch.

Fixes c4585a2823edf ("bridge: ebt_among: add missing match size checks")
---
 net/bridge/netfilter/ebt_among.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
net/bridge/netfilter/ebt_among.c +34 -0
index c5afb4232ecb..620e54f08296 100644
@@ -177,6 +177,28 @@ static bool poolsize_invalid(const struct ebt_mac_wormhash *w)
 	return w && w->poolsize >= (INT_MAX / sizeof(struct ebt_mac_wormhash_tuple));
 }
 
+static bool wormhash_offset_invalid(int off, unsigned int len)
+{
+	if (off == 0) /* not present */
+		return false;
+
+	if (off < (int)sizeof(struct ebt_among_info) ||
+	    off % __alignof__(struct ebt_mac_wormhash))
+		return true;
+
+	off += sizeof(struct ebt_mac_wormhash);
+
+	return off > len;
+}
+
+static bool wormhash_sizes_valid(const struct ebt_mac_wormhash *wh, int a, int b)
+{
+	if (a == 0)
+		a = sizeof(struct ebt_among_info);
+
+	return ebt_mac_wormhash_size(wh) + a == b;
+}
+
 static int ebt_among_mt_check(const struct xt_mtchk_param *par)
 {
 	const struct ebt_among_info *info = par->matchinfo;
@@ -189,6 +211,10 @@ static int ebt_among_mt_check(const struct xt_mtchk_param *par)
 	if (expected_length > em->match_size)
 		return -EINVAL;
 
+	if (wormhash_offset_invalid(info->wh_dst_ofs, em->match_size) ||
+	    wormhash_offset_invalid(info->wh_src_ofs, em->match_size))
+		return -EINVAL;
+
 	wh_dst = ebt_among_wh_dst(info);
 	if (poolsize_invalid(wh_dst))
 		return -EINVAL;
@@ -201,6 +227,14 @@ static int ebt_among_mt_check(const struct xt_mtchk_param *par)
 	if (poolsize_invalid(wh_src))
 		return -EINVAL;
 
+	if (info->wh_src_ofs < info->wh_dst_ofs) {
+		if (!wormhash_sizes_valid(wh_src, info->wh_src_ofs, info->wh_dst_ofs))
+			return -EINVAL;
+	} else {
+		if (!wormhash_sizes_valid(wh_dst, info->wh_dst_ofs, info->wh_src_ofs))
+			return -EINVAL;
+	}
+
 	expected_length += ebt_mac_wormhash_size(wh_src);
 
 	if (em->match_size != EBT_ALIGN(expected_length)) {
Signature · 1 line
2.11.0
author maintainer
#5 lore raw

PATCH 5/5 netfilter: nf_tables: release flowtable hooks

In reply to Pablo Neira Ayuso

Otherwise we leak this array.

---
 net/netfilter/nf_tables_api.c | 1 +
 1 file changed, 1 insertion(+)
net/netfilter/nf_tables_api.c +1 -0
index 558593e6a0a3..c4acc7340eb1 100644
@@ -5423,6 +5423,7 @@ static void nf_tables_flowtable_notify(struct nft_ctx *ctx,
 static void nf_tables_flowtable_destroy(struct nft_flowtable *flowtable)
 {
 	cancel_delayed_work_sync(&flowtable->data.gc_work);
+	kfree(flowtable->ops);
 	kfree(flowtable->name);
 	flowtable->data.type->free(&flowtable->data);
 	rhashtable_destroy(&flowtable->data.rhashtable);
Signature · 1 line
2.11.0
maintainer
#6 lore raw
In reply to Pablo Neira Ayuso

From: Pablo Neira Ayuso <[email protected]>
Date: Mon, 12 Mar 2018 17:15:59 +0100

Quoted 6 lines

The following patchset contains Netfilter fixes for your net tree, they are:

1) Fixed hashtable representation doesn't support timeout flag, skip it otherwise rules to add elements from the packet fail bogusly fail with EOPNOTSUPP.

Quoted 8 lines

2) Fix bogus error with 32-bits ebtables userspace and 64-bits kernel, patch from Florian Westphal.

3) Sanitize proc names in several x_tables extensions, also from Florian.

4) Add sanitization to ebt_among wormhash logic, from Florian.

5) Missing release of hook array in flowtable.

Pulled, thank you.