Skip to contentSkip to conversation

netfilter: nf_tables: Check for overflow of u8 fields from u32 netlink attributes

PATCH In review Laura Garcia Liebana 2 participants 2 messages Aug 14, 2016 – Aug 17, 2016

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Laura Garcia Liebana

author
#0 lore raw

PATCH v2 1/1 netfilter: nf_tables: Check for overflow of u8 fields from u32 netlink attributes

Fix the direct assignment from u32 data input into an attribute with a size of u8.

Refer to 4da449ae1df

Signed-off-by Laura Garcia Liebana <[email protected]>

---
Changes in V2:
- Collapse the 5 independent patches in just one
- Change description and subject
- Add bug link

 net/netfilter/nft_bitwise.c   |  7 ++++++-
 net/netfilter/nft_byteorder.c | 13 +++++++++++--
 net/netfilter/nft_cmp.c       |  5 ++++-
 net/netfilter/nft_immediate.c |  3 +++
 net/netfilter/nft_nat.c       |  2 ++
 5 files changed, 26 insertions(+), 4 deletions(-)
net/netfilter/nft_bitwise.c +6 -1
index d71cc18..2c49f69 100644
@@ -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 -EINVAL;
+	priv->len = len;
+
 	priv->sreg = nft_parse_register(tb[NFTA_BITWISE_SREG]);
 	err = nft_validate_register_load(priv->sreg, priv->len);
 	if (err < 0)
net/netfilter/nft_byteorder.c +11 -2
index b78c28b..fdd23d5 100644
@@ -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 -EINVAL;
+	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 -EINVAL;
+	priv->len = len;
+
 	err = nft_validate_register_load(priv->sreg, priv->len);
 	if (err < 0)
 		return err;
net/netfilter/nft_cmp.c +4 -1
index e25b35d..ca247e5 100644
@@ -84,8 +84,11 @@ 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 -EINVAL;
 	priv->len = desc.len;
+	priv->op  = ntohl(nla_get_be32(tb[NFTA_CMP_OP]));
+
 	return 0;
 }
net/netfilter/nft_immediate.c +3 -0
index db3b746..6de590c 100644
@@ -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 -EINVAL;
 	priv->dlen = desc.len;
 
 	priv->dreg = nft_parse_register(tb[NFTA_IMMEDIATE_DREG]);
net/netfilter/nft_nat.c +2 -0
index ee2d717..74f8293 100644
@@ -148,6 +148,8 @@ static int nft_nat_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 	family = ntohl(nla_get_be32(tb[NFTA_NAT_FAMILY]));
 	if (family != ctx->afi->family)
 		return -EOPNOTSUPP;
+	if (family > U8_MAX)
+		return -EINVAL;
 
 	switch (family) {
 	case NFPROTO_IPV4:
Signature · 1 line
2.8.1
maintainer
#1 lore raw

net/netfilter/nft_cmp.c static int nft_cmp_init(const struct nft_ctx *ctx, const struct nft_expr *expr, · line 88

Quoted 11 lines · Laura Garcia Liebana
net/netfilter/nft_cmp.c +2 -1
index e25b35d..ca247e5 100644
@@ -84,8 +84,11 @@ 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 -EINVAL;

I suggest we use return -ERANGE instead of -EINVAL.

net/netfilter/nft_cmp.c static int nft_cmp_init(const struct nft_ctx *ctx, const struct nft_expr *expr, · line 90

Quoted 2 lines
net/netfilter/nft_cmp.c +1 -0
 	priv->len = desc.len;
+	priv->op  = ntohl(nla_get_be32(tb[NFTA_CMP_OP]));

Hm, I just noticed this priv->op is not validated either?

If so, we should add NFT_CMP_MAX to enum nft_cmp_ops and check if this is:

        if (priv->op >= NFT_CMP_MAX)
                return -ERANGE;

You can send this in a follow up patch given that this doesn't match the description, or you rework the title and description to make this all fit in one logical change.

Quoted 19 lines
net/netfilter/nft_immediate.c +3 -0
index db3b746..6de590c 100644
@@ -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 -EINVAL;
 	priv->dlen = desc.len;
 
 	priv->dreg = nft_parse_register(tb[NFTA_IMMEDIATE_DREG]);
net/netfilter/nft_nat.c +0 -0
index ee2d717..74f8293 100644

net/netfilter/nft_nat.c static int nft_nat_init(const struct nft_ctx *ctx, const struct nft_expr *expr, · line 152

Quoted 5 lines
net/netfilter/nft_nat.c +2 -0
@@ -148,6 +148,8 @@ static int nft_nat_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 	family = ntohl(nla_get_be32(tb[NFTA_NAT_FAMILY]));
 	if (family != ctx->afi->family)
 		return -EOPNOTSUPP;
+	if (family > U8_MAX)
+		return -EINVAL;

I think we don't need this, because we check later on:

        switch (family) {
        case NFPROTO_IPV4:
                ...
        case NFPROTO_IPV6:
                ...
        default:
                return -EAFNOSUPPORT;
        }

So this check is superfluous, you can remove it.