diff mbox

[v4] netfilter: nf_tables: Ensure init attributes are within the bounds

Message ID 20160818160623.GA25544@sonyv
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

nevola Aug. 18, 2016, 4:06 p.m. UTC 
Check for overflow of u8 fields from u32 netlink attributes and maximum
values.

Refer to 4da449ae1df

Signed-off-by: Laura Garcia Liebana <nevola@gmail.com>
---
(was: netfilter: nf_tables: Check for overflow of u8 fields from u32
netlink attributes)

Changes in V4:
	- Define NFT_CMP_MAX

 include/uapi/linux/netfilter/nf_tables.h |  2 ++
 net/netfilter/nft_bitwise.c              |  7 ++++++-
 net/netfilter/nft_byteorder.c            | 13 +++++++++++--
 net/netfilter/nft_cmp.c                  |  9 ++++++++-
 net/netfilter/nft_immediate.c            |  3 +++
 5 files changed, 30 insertions(+), 4 deletions(-)

Comments

Pablo Neira Ayuso Aug. 25, 2016, 11:29 a.m. UTC | #1 
Hi Laura,

On Thu, Aug 18, 2016 at 06:06:26PM +0200, Laura Garcia Liebana wrote:
> Check for overflow of u8 fields from u32 netlink attributes and maximum
> values.

After a closer look, this lack of validation seems more widespread
than I initially expected.

Look, other enums like:

enum nft_set_policies {
        NFT_SET_POL_PERFORMANCE,
        NFT_SET_POL_MEMORY,
};

that has no _MAX definition are suspect, actually looking at
net/netfilter/nf_tables_api.c more specifically at
nft_select_set_ops() you'll notice that the switch there doesn't seem
to reject anything over NFT_SET_POL_MEMORY.

So I would review net/netfilter/nf_tables_api.c too.

BTW, I think it is a good idea to add something like:

	err = nft_parse_u8(ntohl(nla_get_be32(tb[NFTA_BITWISE_LEN]),
                           &priv->len);
        if (err < 0)
                return err;

that we can consistently use all over the code, instead of open
coding:

        len = ...
        if (len > U8_MAX)
                return -ERANGE;

> Refer to 4da449ae1df

Please, use this format instead to refer to patches:

4da449a ("netfilter: nft_exthdr: Add size check on u8 nft_exthdr attributes")

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 0ddefb1..ce12a20 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -528,7 +528,9 @@  enum nft_cmp_ops {
 	NFT_CMP_LTE,
 	NFT_CMP_GT,
 	NFT_CMP_GTE,
+	__NFT_CMP_MAX
 };
+#define NFT_CMP_MAX	(__NFT_CMP_MAX - 1)
 
 /**
  * enum nft_cmp_attributes - nf_tables cmp expression netlink attributes
diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
index d71cc18..6e09b1e 100644
--- a/net/netfilter/nft_bitwise.c
+++ b/net/netfilter/nft_bitwise.c
@@ -53,6 +53,7 @@  static int nft_bitwise_init(const struct nft_ctx *ctx,
 	struct nft_bitwise *priv = nft_expr_priv(expr);
 	struct nft_data_desc d1, d2;
 	int err;
+	u32 len;
 
 	if (tb[NFTA_BITWISE_SREG] == NULL ||
 	    tb[NFTA_BITWISE_DREG] == NULL ||
@@ -61,7 +62,11 @@  static int nft_bitwise_init(const struct nft_ctx *ctx,
 	    tb[NFTA_BITWISE_XOR] == NULL)
 		return -EINVAL;
 
-	priv->len  = ntohl(nla_get_be32(tb[NFTA_BITWISE_LEN]));
+	len  = ntohl(nla_get_be32(tb[NFTA_BITWISE_LEN]));
+	if (len > U8_MAX)
+		return -ERANGE;
+	priv->len = len;
+
 	priv->sreg = nft_parse_register(tb[NFTA_BITWISE_SREG]);
 	err = nft_validate_register_load(priv->sreg, priv->len);
 	if (err < 0)
diff --git a/net/netfilter/nft_byteorder.c b/net/netfilter/nft_byteorder.c
index b78c28b..763cf15 100644
--- a/net/netfilter/nft_byteorder.c
+++ b/net/netfilter/nft_byteorder.c
@@ -100,6 +100,7 @@  static int nft_byteorder_init(const struct nft_ctx *ctx,
 {
 	struct nft_byteorder *priv = nft_expr_priv(expr);
 	int err;
+	u32 len, size;
 
 	if (tb[NFTA_BYTEORDER_SREG] == NULL ||
 	    tb[NFTA_BYTEORDER_DREG] == NULL ||
@@ -117,7 +118,10 @@  static int nft_byteorder_init(const struct nft_ctx *ctx,
 		return -EINVAL;
 	}
 
-	priv->size = ntohl(nla_get_be32(tb[NFTA_BYTEORDER_SIZE]));
+	size = ntohl(nla_get_be32(tb[NFTA_BYTEORDER_SIZE]));
+	if (size > U8_MAX)
+		return -ERANGE;
+	priv->size = size;
 	switch (priv->size) {
 	case 2:
 	case 4:
@@ -128,7 +132,12 @@  static int nft_byteorder_init(const struct nft_ctx *ctx,
 	}
 
 	priv->sreg = nft_parse_register(tb[NFTA_BYTEORDER_SREG]);
-	priv->len  = ntohl(nla_get_be32(tb[NFTA_BYTEORDER_LEN]));
+
+	len  = ntohl(nla_get_be32(tb[NFTA_BYTEORDER_LEN]));
+	if (len > U8_MAX)
+		return -ERANGE;
+	priv->len = len;
+
 	err = nft_validate_register_load(priv->sreg, priv->len);
 	if (err < 0)
 		return err;
diff --git a/net/netfilter/nft_cmp.c b/net/netfilter/nft_cmp.c
index e25b35d..cb9cfab 100644
--- a/net/netfilter/nft_cmp.c
+++ b/net/netfilter/nft_cmp.c
@@ -55,6 +55,8 @@  static void nft_cmp_eval(const struct nft_expr *expr,
 		if (d < 0)
 			goto mismatch;
 		break;
+	default:
+		break;
 	}
 	return;
 
@@ -84,8 +86,13 @@  static int nft_cmp_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 	if (err < 0)
 		return err;
 
-	priv->op  = ntohl(nla_get_be32(tb[NFTA_CMP_OP]));
+	if (desc.len > U8_MAX)
+		return -ERANGE;
 	priv->len = desc.len;
+	priv->op  = ntohl(nla_get_be32(tb[NFTA_CMP_OP]));
+	if (priv->op > NFT_CMP_MAX)
+		return -ERANGE;
+
 	return 0;
 }
 
diff --git a/net/netfilter/nft_immediate.c b/net/netfilter/nft_immediate.c
index db3b746..b5f899c 100644
--- a/net/netfilter/nft_immediate.c
+++ b/net/netfilter/nft_immediate.c
@@ -53,6 +53,9 @@  static int nft_immediate_init(const struct nft_ctx *ctx,
 			    tb[NFTA_IMMEDIATE_DATA]);
 	if (err < 0)
 		return err;
+
+	if (desc.len > U8_MAX)
+		return -ERANGE;
 	priv->dlen = desc.len;
 
 	priv->dreg = nft_parse_register(tb[NFTA_IMMEDIATE_DREG]);