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

[nft PATCH 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.

Phil Sutter (4):
  evaluate: Fix datalen checks in expr_evaluate_string()
  netlink_delinearize: Avoid potential null pointer deref
  proto_find_num: Avoid potential null pointer dereference
  evaluate: Avoid undefined behaviour in concat_subtype_id()

 src/evaluate.c            | 8 +++++---
 src/netlink_delinearize.c | 2 ++
 src/proto.c               | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.8.2

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

[Phil Sutter]

This part of the code is pretty weird due to suboptimal variable name
choice: 'data', 'len', 'datalen', 'data_len'.

But even without understanding all of it, the code checking 'datalen - 1
>= 0' assumes 'datalen - 1' may actually become negative, which is not
true since it is unsigned. So make 'datalen' a signed integer instead.

Another issue is the check for "data[datalen] != '*'" which will access
unallocated memory if 'strlen(data) == 0'. So make sure 'datalen >= 0'
before using it as array index.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/evaluate.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 87f5a6d77d485..523eedabe84ac 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -211,8 +211,9 @@ static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr)
 static int expr_evaluate_string(struct eval_ctx *ctx, struct expr **exprp)
 {
 	struct expr *expr = *exprp;
-	unsigned int len = div_round_up(expr->len, BITS_PER_BYTE), datalen;
+	unsigned int len = div_round_up(expr->len, BITS_PER_BYTE);
 	struct expr *value, *prefix;
+	int datalen;
 	int data_len = ctx->ectx.len > 0 ? ctx->ectx.len : len + 1;
 	char data[data_len];
 
@@ -228,7 +229,8 @@ static int expr_evaluate_string(struct eval_ctx *ctx, struct expr **exprp)
 	mpz_export_data(data, expr->value, BYTEORDER_HOST_ENDIAN, len);
 
 	datalen = strlen(data) - 1;
-	if (data[datalen] != '*') {
+	if (datalen >= 0 &&
+	    data[datalen] != '*') {
 		/* We need to reallocate the constant expression with the right
 		 * expression length to avoid problems on big endian.
 		 */
-- 
2.8.2

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

[Pablo Neira Ayuso]

On Fri, Aug 12, 2016 at 06:00:07PM +0200, Phil Sutter wrote:
> This part of the code is pretty weird due to suboptimal variable name
> choice: 'data', 'len', 'datalen', 'data_len'.
> 
> But even without understanding all of it, the code checking 'datalen - 1
> >= 0' assumes 'datalen - 1' may actually become negative, which is not
> true since it is unsigned. So make 'datalen' a signed integer instead.
> 
> Another issue is the check for "data[datalen] != '*'" which will access
> unallocated memory if 'strlen(data) == 0'. So make sure 'datalen >= 0'
> before using it as array index.

We don't allow empty strings from our flex scanner as string, so we
assume the string is at least 1.

You can probably add an assert() here instead.

[nft PATCH 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>
---
 src/netlink_delinearize.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 12d0b4a277795..6ac2e9690fd39 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -518,6 +518,8 @@ 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)
+		return;
 
 	key  = nftnl_expr_get_u32(nle, NFTNL_EXPR_CT_KEY);
 	stmt = ct_stmt_alloc(loc, key, expr);
-- 
2.8.2

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

[Pablo Neira Ayuso]

On Fri, Aug 12, 2016 at 06:00:08PM +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().
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/netlink_delinearize.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> index 12d0b4a277795..6ac2e9690fd39 100644
> --- a/src/netlink_delinearize.c
> +++ b/src/netlink_delinearize.c
> @@ -518,6 +518,8 @@ 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)
> +		return;

I would add a netlink_error() by here too.

I can see more spots with this problem, would you send us a patch to
resolve the multiple reincarnations of this problem in one go?

Thanks!

[nft PATCH 3/4] proto_find_num: Avoid potential null pointer dereference

[Phil Sutter]

When being called from stmt_evaluate_reset(), it seems that 'base' might
actually be NULL, so better make sure it is not in proto_find_num().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
This might be invalid in that if 'base' is NULL, ctx->pctx.family is
always either NFPROTO_INET or NFPROTO_BRIDGE. But if so, the
corresponding check in stmt_evaluate_reset() may be simplified to just
having 'base' falling back to &proto_inet_service.
---
 src/proto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/proto.c b/src/proto.c
index 4c12977cef082..d9210afeaf256 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -75,7 +75,7 @@ int proto_find_num(const struct proto_desc *base,
 {
 	unsigned int i;
 
-	for (i = 0; i < array_size(base->protocols); i++) {
+	for (i = 0; base && i < array_size(base->protocols); i++) {
 		if (base->protocols[i].desc == desc)
 			return base->protocols[i].num;
 	}
-- 
2.8.2

Re: [nft PATCH 3/4] proto_find_num: Avoid potential null pointer dereference

[Pablo Neira Ayuso]

On Fri, Aug 12, 2016 at 06:00:09PM +0200, Phil Sutter wrote:
> When being called from stmt_evaluate_reset(), it seems that 'base' might
> actually be NULL, so better make sure it is not in proto_find_num().

I would suggest you address this from stmt_evaluate_reset().

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

[Phil Sutter]

Looking at expr_evaluate_concat(), 'off' might be zero and the error
checks not triggering (by having dtype != NULL and i->dtype->size > 0).
Decrementing it will then lead to casting -1 to unsigned during the call
to concat_subtype_lookup() will lead to bit-shifting in
concat_subtype_id() by a value bigger than the number of bits in 'type'
(which is 32bit).

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
This patch is just an ugly sanitization hack and should probably be
substituted by  an additional error check in expr_evaluate_concat()
giving an explanation of what went wrong.
---
 src/evaluate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 523eedabe84ac..c8568690f6338 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -950,7 +950,7 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
 						 "expressions",
 						 i->dtype->name);
 
-		tmp = concat_subtype_lookup(type, --off);
+		tmp = concat_subtype_lookup(type, off > 0 ? --off : 0);
 		expr_set_context(&ctx->ectx, tmp, tmp->size);
 
 		if (list_member_evaluate(ctx, &i) < 0)
-- 
2.8.2

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

[Pablo Neira Ayuso]

On Fri, Aug 12, 2016 at 06:00:10PM +0200, Phil Sutter wrote:
> Looking at expr_evaluate_concat(), 'off' might be zero and the error
> checks not triggering (by having dtype != NULL and i->dtype->size > 0).
> Decrementing it will then lead to casting -1 to unsigned during the call
> to concat_subtype_lookup() will lead to bit-shifting in
> concat_subtype_id() by a value bigger than the number of bits in 'type'
> (which is 32bit).
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> This patch is just an ugly sanitization hack and should probably be
> substituted by  an additional error check in expr_evaluate_concat()
> giving an explanation of what went wrong.

I agree. It would be good to look back and see what condition can
indeed trigger this instead of papering the problem with this check.

Thanks!

>  src/evaluate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 523eedabe84ac..c8568690f6338 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -950,7 +950,7 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
>  						 "expressions",
>  						 i->dtype->name);
>  
> -		tmp = concat_subtype_lookup(type, --off);
> +		tmp = concat_subtype_lookup(type, off > 0 ? --off : 0);
>  		expr_set_context(&ctx->ectx, tmp, tmp->size);
>  
>  		if (list_member_evaluate(ctx, &i) < 0)
> -- 
> 2.8.2
>