[nft PATCH v2 0/4] A round of covscan indicated fixes

[nft PATCH v2 0/4] A round of covscan indicated fixes

[Phil Sutter]

As for libnftnl, this series aims at fixing a number of issues
identified by covscan. And again, due to my limited overview of the
code-base, some of them might as well be invalid although I tried to
verify the issues as best as I can.

Changes since v1:
- Rebased onto current upstream master.
- Reviewed and improved every patch (for details see each patch's
  changelog).

Phil Sutter (4):
  evaluate: Fix datalen checks in expr_evaluate_string()
  netlink_delinearize: Avoid potential null pointer deref
  stmt_evaluate_reset: Have a generic fix for missing network context
  evaluate: Avoid undefined behaviour in concat_subtype_id()

 src/evaluate.c            | 12 +++++++-----
 src/netlink_delinearize.c |  6 ++++++
 2 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.8.2

[nft PATCH v2 1/4] evaluate: Fix datalen checks in expr_evaluate_string()

[Phil Sutter]

I have been told that the flex scanner won't return empty strings, so
strlen(data) should always be greater 0. To avoid a hard to debug issue
though, add an assert() to make sure this is always the case before
risking an unsigned variable underrun.

A real issue though is the check for 'datalen - 1 >= 0', which will
never fail due to datalen being unsigned. Fix this by incrementing both
sides by one, hence checking 'datalen >= 1'.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Based on the assumption that strlen(data) should never be less than 1,
  just add an assert() to be on the safe side.
- By modifying the '>= 0' check a bit, we can get by without making
  datalen a signed variable.
---
 src/evaluate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 7eb28f2c4b72d..fb9b82534d520 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -221,6 +221,7 @@ static int expr_evaluate_string(struct eval_ctx *ctx, struct expr **exprp)
 	memset(data + len, 0, data_len - len);
 	mpz_export_data(data, expr->value, BYTEORDER_HOST_ENDIAN, len);
 
+	assert(strlen(data) > 0);
 	datalen = strlen(data) - 1;
 	if (data[datalen] != '*') {
 		/* We need to reallocate the constant expression with the right
@@ -234,7 +235,7 @@ static int expr_evaluate_string(struct eval_ctx *ctx, struct expr **exprp)
 		return 0;
 	}
 
-	if (datalen - 1 >= 0 &&
+	if (datalen >= 1 &&
 	    data[datalen - 1] == '\\') {
 		char unescaped_str[data_len];
 
-- 
2.8.2

Re: [nft PATCH v2 1/4] evaluate: Fix datalen checks in expr_evaluate_string()

[Pablo Neira Ayuso]

On Tue, Aug 30, 2016 at 07:39:49PM +0200, Phil Sutter wrote:
> I have been told that the flex scanner won't return empty strings, so
> strlen(data) should always be greater 0. To avoid a hard to debug issue
> though, add an assert() to make sure this is always the case before
> risking an unsigned variable underrun.
> 
> A real issue though is the check for 'datalen - 1 >= 0', which will
> never fail due to datalen being unsigned. Fix this by incrementing both
> sides by one, hence checking 'datalen >= 1'.

Applied, thanks Phil.

[nft PATCH v2 2/4] netlink_delinearize: Avoid potential null pointer deref

[Phil Sutter]

As netlink_get_register() may return NULL, we must not pass the returned
data unchecked to expr_set_type() as that will dereference it. Since the
parser has failed at that point anyway, by returning early we can skip
the useless statement allocation that follows in
netlink_parse_ct_stmt().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Fix for the same problem in netlink_parse_meta_stmt(), too.
- Print error message instead of silently failing.
- Align coding style with other post netlink_get_register() checks.
---
 src/netlink_delinearize.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 1a1cfbded4e1e..e3846a700ca8e 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -517,6 +517,9 @@ static void netlink_parse_meta_stmt(struct netlink_parse_ctx *ctx,
 
 	sreg = netlink_parse_register(nle, NFTNL_EXPR_META_SREG);
 	expr = netlink_get_register(ctx, loc, sreg);
+	if (expr == NULL)
+		return netlink_error(ctx, loc,
+				     "No expression in register %u", sreg);
 
 	key  = nftnl_expr_get_u32(nle, NFTNL_EXPR_META_KEY);
 	stmt = meta_stmt_alloc(loc, key, expr);
@@ -562,6 +565,9 @@ static void netlink_parse_ct_stmt(struct netlink_parse_ctx *ctx,
 
 	sreg = netlink_parse_register(nle, NFTNL_EXPR_CT_SREG);
 	expr = netlink_get_register(ctx, loc, sreg);
+	if (expr == NULL)
+		return netlink_error(ctx, loc,
+				     "No expression in register %u", sreg);
 
 	key  = nftnl_expr_get_u32(nle, NFTNL_EXPR_CT_KEY);
 	stmt = ct_stmt_alloc(loc, key, expr);
-- 
2.8.2

Re: [nft PATCH v2 2/4] netlink_delinearize: Avoid potential null pointer deref

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]

On Tue, Aug 30, 2016 at 07:39:50PM +0200, Phil Sutter wrote:
> As netlink_get_register() may return NULL, we must not pass the returned
> data unchecked to expr_set_type() as that will dereference it. Since the
> parser has failed at that point anyway, by returning early we can skip
> the useless statement allocation that follows in
> netlink_parse_ct_stmt().

I found a couple more spots, such as the payload stmt that was not
covered by this patch.

Attaching a new one based on this, looks good to you?

Anyway, this is very unlikely to happen: Only if we ever get more
registers in the kernel, given that expl_clone() relies on the
xzalloc() function that just stops execution under OOM.

Actually this brings an interesting issue that is that we need to
provide a way to describe the vm capabilities so we can extend things
in the future without breaking userspace.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 2366 bytes --]

commit 098dae9e0cf2237b4cb3cf4c1ee89fbf9f9fb5e9
Author: Phil Sutter <phil@nwl.cc>
Date:   Tue Aug 30 19:39:50 2016 +0200

    netlink_delinearize: Avoid potential null pointer deref
    
    As netlink_get_register() may return NULL, we must not pass the returned
    data unchecked to expr_set_type() as that will dereference it. Since the
    parser has failed at that point anyway, by returning early we can skip
    the useless statement allocation that follows in
    netlink_parse_ct_stmt().
    
    Signed-off-by: Phil Sutter <phil@nwl.cc>
    Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 1a1cfbd..cddbfa6 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -428,6 +428,10 @@ static void netlink_parse_payload_stmt(struct netlink_parse_ctx *ctx,
 
 	sreg = netlink_parse_register(nle, NFTNL_EXPR_PAYLOAD_SREG);
 	val  = netlink_get_register(ctx, loc, sreg);
+	if (val == NULL)
+		return netlink_error(ctx, loc,
+				     "payload statement has no expression");
+
 	stmt = payload_stmt_alloc(loc, expr, val);
 
 	list_add_tail(&stmt->list, &ctx->rule->stmts);
@@ -473,6 +477,9 @@ static void netlink_parse_hash(struct netlink_parse_ctx *ctx,
 
 	sreg = netlink_parse_register(nle, NFTNL_EXPR_HASH_SREG);
 	hexpr = netlink_get_register(ctx, loc, sreg);
+	if (hexpr == NULL)
+		return netlink_error(ctx, loc,
+				     "hash statement has no expression");
 
 	seed = nftnl_expr_get_u32(nle, NFTNL_EXPR_HASH_SEED);
 	mod  = nftnl_expr_get_u32(nle, NFTNL_EXPR_HASH_MODULUS);
@@ -517,6 +524,9 @@ static void netlink_parse_meta_stmt(struct netlink_parse_ctx *ctx,
 
 	sreg = netlink_parse_register(nle, NFTNL_EXPR_META_SREG);
 	expr = netlink_get_register(ctx, loc, sreg);
+	if (expr == NULL)
+		return netlink_error(ctx, loc,
+				     "meta statement has no expression");
 
 	key  = nftnl_expr_get_u32(nle, NFTNL_EXPR_META_KEY);
 	stmt = meta_stmt_alloc(loc, key, expr);
@@ -562,6 +572,9 @@ static void netlink_parse_ct_stmt(struct netlink_parse_ctx *ctx,
 
 	sreg = netlink_parse_register(nle, NFTNL_EXPR_CT_SREG);
 	expr = netlink_get_register(ctx, loc, sreg);
+	if (expr == NULL)
+		return netlink_error(ctx, loc,
+				     "ct statement has no expression");
 
 	key  = nftnl_expr_get_u32(nle, NFTNL_EXPR_CT_KEY);
 	stmt = ct_stmt_alloc(loc, key, expr);

Re: [nft PATCH v2 2/4] netlink_delinearize: Avoid potential null pointer deref

[Phil Sutter]

Hi,

On Mon, Sep 05, 2016 at 06:52:43PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 30, 2016 at 07:39:50PM +0200, Phil Sutter wrote:
> > As netlink_get_register() may return NULL, we must not pass the returned
> > data unchecked to expr_set_type() as that will dereference it. Since the
> > parser has failed at that point anyway, by returning early we can skip
> > the useless statement allocation that follows in
> > netlink_parse_ct_stmt().
> 
> I found a couple more spots, such as the payload stmt that was not
> covered by this patch.

OK. The reason I left those out was that they don't call expr_set_type()
and therefore the potential NULL pointer dereference doesn't happen
there.

> Attaching a new one based on this, looks good to you?

Acked-by: Phil Sutter <phil@nwl.cc>

> Anyway, this is very unlikely to happen: Only if we ever get more
> registers in the kernel, given that expl_clone() relies on the
> xzalloc() function that just stops execution under OOM.

Yes. The thing with Covscan is it will find very theoretical issues and
practical relevance is usually questionable (as they would likely have
been found already).

> Actually this brings an interesting issue that is that we need to
> provide a way to describe the vm capabilities so we can extend things
> in the future without breaking userspace.

Not my cup of tea luckily. :)

Thanks, Phil

[nft PATCH v2 3/4] stmt_evaluate_reset: Have a generic fix for missing network context

[Phil Sutter]

Commit 17b495957b29e ("evaluate: reject: fix crash if we have transport
protocol conflict from inet") took care of a crash when using inet or
bridge families, but since then netdev family has been added which also
does not implicitly define the network context. Therefore the crash can
be reproduced again using the following example:

nft add rule netdev filter e1000-ingress \
		meta l4proto udp reject with tcp reset

In order to fix this in a more generic way, have stmt_evaluate_reset()
fall back to the generic proto_inet_service irrespective of the actual
proto context.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Found the real cause for the problem after some more code and git
  history research, so rewrote the patch from scratch.
---
 src/evaluate.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index fb9b82534d520..194a03495b5fd 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2167,9 +2167,7 @@ static int stmt_evaluate_reset(struct eval_ctx *ctx, struct stmt *stmt)
 		return 0;
 
 	base = pctx->protocol[PROTO_BASE_NETWORK_HDR].desc;
-	if (base == NULL &&
-	    (ctx->pctx.family == NFPROTO_INET ||
-	     ctx->pctx.family == NFPROTO_BRIDGE))
+	if (base == NULL)
 		base = &proto_inet_service;
 
 	protonum = proto_find_num(base, desc);
-- 
2.8.2

Re: [nft PATCH v2 3/4] stmt_evaluate_reset: Have a generic fix for missing network context

[Pablo Neira Ayuso]

On Tue, Aug 30, 2016 at 07:39:51PM +0200, Phil Sutter wrote:
> Commit 17b495957b29e ("evaluate: reject: fix crash if we have transport
> protocol conflict from inet") took care of a crash when using inet or
> bridge families, but since then netdev family has been added which also
> does not implicitly define the network context. Therefore the crash can
> be reproduced again using the following example:
> 
> nft add rule netdev filter e1000-ingress \
> 		meta l4proto udp reject with tcp reset
> 
> In order to fix this in a more generic way, have stmt_evaluate_reset()
> fall back to the generic proto_inet_service irrespective of the actual
> proto context.

Applied, thanks.

This reminds me that the reject code needs care, it is a bit tangled.
This was made by a GSoC student.

[nft PATCH v2 4/4] evaluate: Avoid undefined behaviour in concat_subtype_id()

[Phil Sutter]

For the left side of a concat expression, dtype is NULL and therefore
off is 0. In that case the code expects to get a datatype of
TYPE_INVALID, but this is fragile as the output of concat_subtype_id()
is undefined for n > 32 / TYPE_BITS.

To fix this, call datatype_lookup() directly passing the expected
TYPE_INVALID as argument if off is 0.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Understood (and reproduced) when the situation causing the undefined
  behaviour happens.
- Understood what the code is supposed to do in that situation.
- Based on the information at hand, implemented a solution which avoids
  undefined behaviour.
---
 src/evaluate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 194a03495b5fd..c1ee6b1929551 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -962,7 +962,10 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
 						 "expressions",
 						 i->dtype->name);
 
-		tmp = concat_subtype_lookup(type, --off);
+		if (dtype == NULL)
+			tmp = datatype_lookup(TYPE_INVALID);
+		else
+			tmp = concat_subtype_lookup(type, --off);
 		expr_set_context(&ctx->ectx, tmp, tmp->size);
 
 		if (list_member_evaluate(ctx, &i) < 0)
-- 
2.8.2

Re: [nft PATCH v2 4/4] evaluate: Avoid undefined behaviour in concat_subtype_id()

[Pablo Neira Ayuso]

On Tue, Aug 30, 2016 at 07:39:52PM +0200, Phil Sutter wrote:
> For the left side of a concat expression, dtype is NULL and therefore
> off is 0. In that case the code expects to get a datatype of
> TYPE_INVALID, but this is fragile as the output of concat_subtype_id()
> is undefined for n > 32 / TYPE_BITS.
> 
> To fix this, call datatype_lookup() directly passing the expected
> TYPE_INVALID as argument if off is 0.

Also applied, thanks!