[libnftnl PATCH 0/7] A bunch of covscan detected fixes

[libnftnl PATCH 0/7] A bunch of covscan detected fixes

[Phil Sutter]

The following series aims at fixing a number of issues identified by
Coverity tool. Due to limited familiarity with the whole code layout, I
am not sure all of them are really valid, but I tried my best to verify
the concerns are legitimate and worth fixing.

Phil Sutter (7):
  set: prevent memleak in nftnl_jansson_parse_set_info()
  ruleset: Prevent memleak in nftnl_ruleset_snprintf_*() functions
  expr/ct: prevent array index overrun in ctkey2str()
  expr/limit: Drop unreachable code in limit_to_type()
  common: Avoid integer overflow in nftnl_batch_is_supported()
  Avoid returning uninitialized data
  ruleset: Initialize ctx.flags before calling nftnl_ruleset_ctx_set()

 src/chain.c      |  2 +-
 src/common.c     |  3 +++
 src/expr/ct.c    |  2 +-
 src/expr/limit.c |  1 -
 src/rule.c       |  2 +-
 src/ruleset.c    | 18 ++++++++++++++++++
 src/set.c        | 12 ++++++------
 src/table.c      |  2 +-
 8 files changed, 31 insertions(+), 11 deletions(-)

-- 
2.8.2

[libnftnl PATCH 1/7] set: prevent memleak in nftnl_jansson_parse_set_info()

[Phil Sutter]

From: Phil Sutter <psutter@redhat.com>

During list populating, in error case the function returns without
freeing the newly allocated 'elem' object, thereby losing any references
to it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/set.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/set.c b/src/set.c
index 5f7245855db53..7a41e8caeb3c0 100644
--- a/src/set.c
+++ b/src/set.c
@@ -610,12 +610,12 @@ static int nftnl_jansson_parse_set_info(struct nftnl_set *s, json_t *tree,
 				return -1;
 
 			json_elem = json_array_get(array, i);
-			if (json_elem == NULL)
-				return -1;
-
-			if (nftnl_jansson_set_elem_parse(elem,
-						       json_elem, err) < 0)
+			if (json_elem == NULL ||
+			    nftnl_jansson_set_elem_parse(elem,
+							 json_elem, err) < 0) {
+				free(elem);
 				return -1;
+			}
 
 			list_add_tail(&elem->head, &s->element_list);
 		}
-- 
2.8.2

[libnftnl PATCH 2/7] ruleset: Prevent memleak in nftnl_ruleset_snprintf_*() functions

[Phil Sutter]

From: Phil Sutter <psutter@redhat.com>

This is an ugly aspect of the SNPRINTF_BUFFER_SIZE() macro: it contains
a return statement and if that triggers, the function returns without
freeing the iterator object. Therefore duplicate the 'ret < 0' check
before calling it, freeing the iterator knowing that we will bail out
immediately afterwards anyway.

Cc: Arturo Borrero <arturo.borrero.glez@gmail.com>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/ruleset.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/ruleset.c b/src/ruleset.c
index 666bcc7a246b6..93cf95ab61e15 100644
--- a/src/ruleset.c
+++ b/src/ruleset.c
@@ -888,12 +888,16 @@ nftnl_ruleset_snprintf_table(char *buf, size_t size,
 	t = nftnl_table_list_iter_next(ti);
 	while (t != NULL) {
 		ret = nftnl_table_snprintf(buf+offset, len, t, type, flags);
+		if (ret < 0)
+			nftnl_table_list_iter_destroy(ti);
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
 		t = nftnl_table_list_iter_next(ti);
 
 		ret = snprintf(buf+offset, len, "%s",
 			       nftnl_ruleset_o_separator(t, type));
+		if (ret < 0)
+			nftnl_table_list_iter_destroy(ti);
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 	}
 	nftnl_table_list_iter_destroy(ti);
@@ -917,12 +921,16 @@ nftnl_ruleset_snprintf_chain(char *buf, size_t size,
 	c = nftnl_chain_list_iter_next(ci);
 	while (c != NULL) {
 		ret = nftnl_chain_snprintf(buf+offset, len, c, type, flags);
+		if (ret < 0)
+			nftnl_chain_list_iter_destroy(ci);
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
 		c = nftnl_chain_list_iter_next(ci);
 
 		ret = snprintf(buf+offset, len, "%s",
 			       nftnl_ruleset_o_separator(c, type));
+		if (ret < 0)
+			nftnl_chain_list_iter_destroy(ci);
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 	}
 	nftnl_chain_list_iter_destroy(ci);
@@ -946,12 +954,16 @@ nftnl_ruleset_snprintf_set(char *buf, size_t size,
 	s = nftnl_set_list_iter_next(si);
 	while (s != NULL) {
 		ret = nftnl_set_snprintf(buf+offset, len, s, type, flags);
+		if (ret < 0)
+			nftnl_set_list_iter_destroy(si);
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
 		s = nftnl_set_list_iter_next(si);
 
 		ret = snprintf(buf+offset, len, "%s",
 			       nftnl_ruleset_o_separator(s, type));
+		if (ret < 0)
+			nftnl_set_list_iter_destroy(si);
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 	}
 	nftnl_set_list_iter_destroy(si);
@@ -975,12 +987,16 @@ nftnl_ruleset_snprintf_rule(char *buf, size_t size,
 	r = nftnl_rule_list_iter_next(ri);
 	while (r != NULL) {
 		ret = nftnl_rule_snprintf(buf+offset, len, r, type, flags);
+		if (ret < 0)
+			nftnl_rule_list_iter_destroy(ri);
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
 		r = nftnl_rule_list_iter_next(ri);
 
 		ret = snprintf(buf+offset, len, "%s",
 			       nftnl_ruleset_o_separator(r, type));
+		if (ret < 0)
+			nftnl_rule_list_iter_destroy(ri);
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 	}
 	nftnl_rule_list_iter_destroy(ri);
-- 
2.8.2

[libnftnl PATCH 3/7] expr/ct: prevent array index overrun in ctkey2str()

[Phil Sutter]

The array has NFT_CT_MAX fields, so indices must be less than that
number.

Fixes: 977b7a1dbe1bd ("ct: xml: use key names instead of numbers")
Cc: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/expr/ct.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/expr/ct.c b/src/expr/ct.c
index 7d96df4e1d5b0..1a53b49fde0ad 100644
--- a/src/expr/ct.c
+++ b/src/expr/ct.c
@@ -173,7 +173,7 @@ static const char *ctkey2str_array[NFT_CT_MAX] = {
 
 static const char *ctkey2str(uint32_t ctkey)
 {
-	if (ctkey > NFT_CT_MAX)
+	if (ctkey >= NFT_CT_MAX)
 		return "unknown";
 
 	return ctkey2str_array[ctkey];
-- 
2.8.2

[libnftnl PATCH 4/7] expr/limit: Drop unreachable code in limit_to_type()

[Phil Sutter]

The function returns from inside the switch() in any case, so the final
return statement is never reached.

Fixes: 7769cbd9dfe69 ("expr: limit: add per-byte limiting support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/expr/limit.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/expr/limit.c b/src/expr/limit.c
index 4bd096e8e921c..cdff81d620caa 100644
--- a/src/expr/limit.c
+++ b/src/expr/limit.c
@@ -259,7 +259,6 @@ static const char *limit_to_type(enum nft_limit_type type)
 	case NFT_LIMIT_PKT_BYTES:
 		return "bytes";
 	}
-	return "unknown";
 }
 
 static int nftnl_expr_limit_snprintf_default(char *buf, size_t len,
-- 
2.8.2

[libnftnl PATCH 5/7] common: Avoid integer overflow in nftnl_batch_is_supported()

[Phil Sutter]

time() may return -1 which is then assigned to an unsigned integer type
and used as sequence number. The following code increments that number
multiple times, so it may overflow and get libmnl confused. To avoid
this, fall back to a starting sequence number of zero in case the call
to time() failed.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/common.c b/src/common.c
index bf4176ceb76eb..2189cc8a3e882 100644
--- a/src/common.c
+++ b/src/common.c
@@ -192,6 +192,9 @@ int nftnl_batch_is_supported(void)
 	uint32_t seq = time(NULL), req_seq;
 	int ret;
 
+	if (seq == (uint32_t)-1)
+		seq = 0;
+
 	nl = mnl_socket_open(NETLINK_NETFILTER);
 	if (nl == NULL)
 		return -1;
-- 
2.8.2

[libnftnl PATCH 6/7] Avoid returning uninitialized data

[Phil Sutter]

Although the 'err' pointer should be interesting for users only if the
parser returned non-zero, having it point to uninitialized data is
generally a bad thing.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/chain.c | 2 +-
 src/rule.c  | 2 +-
 src/set.c   | 2 +-
 src/table.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/chain.c b/src/chain.c
index ff59f7f705783..dfe0cf23b0f51 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -816,7 +816,7 @@ static int nftnl_chain_do_parse(struct nftnl_chain *c, enum nftnl_parse_type typ
 			      enum nftnl_parse_input input)
 {
 	int ret;
-	struct nftnl_parse_err perr;
+	struct nftnl_parse_err perr = {};
 
 	switch (type) {
 	case NFTNL_PARSE_XML:
diff --git a/src/rule.c b/src/rule.c
index dada00e1cc99f..8aeefbe19a9ed 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -700,7 +700,7 @@ static int nftnl_rule_do_parse(struct nftnl_rule *r, enum nftnl_parse_type type,
 			     enum nftnl_parse_input input)
 {
 	int ret;
-	struct nftnl_parse_err perr;
+	struct nftnl_parse_err perr = {};
 
 	switch (type) {
 	case NFTNL_PARSE_XML:
diff --git a/src/set.c b/src/set.c
index 7a41e8caeb3c0..47b6ef46a9c34 100644
--- a/src/set.c
+++ b/src/set.c
@@ -781,7 +781,7 @@ static int nftnl_set_do_parse(struct nftnl_set *s, enum nftnl_parse_type type,
 			    enum nftnl_parse_input input)
 {
 	int ret;
-	struct nftnl_parse_err perr;
+	struct nftnl_parse_err perr = {};
 
 	switch (type) {
 	case NFTNL_PARSE_XML:
diff --git a/src/table.c b/src/table.c
index bb46716950b98..845fd68168797 100644
--- a/src/table.c
+++ b/src/table.c
@@ -360,7 +360,7 @@ static int nftnl_table_do_parse(struct nftnl_table *t, enum nftnl_parse_type typ
 			      enum nftnl_parse_input input)
 {
 	int ret;
-	struct nftnl_parse_err perr;
+	struct nftnl_parse_err perr = {};
 
 	switch (type) {
 	case NFTNL_PARSE_XML:
-- 
2.8.2

[libnftnl PATCH 7/7] ruleset: Initialize ctx.flags before calling nftnl_ruleset_ctx_set()

[Phil Sutter]

The called function otherwise accesses uninitialized data.

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

diff --git a/src/ruleset.c b/src/ruleset.c
index 93cf95ab61e15..a2d67cb550179 100644
--- a/src/ruleset.c
+++ b/src/ruleset.c
@@ -551,6 +551,7 @@ static int nftnl_ruleset_json_parse(const void *json,
 
 	ctx.cb = cb;
 	ctx.format = type;
+	ctx.flags = 0;
 
 	ctx.set_list = nftnl_set_list_alloc();
 	if (ctx.set_list == NULL)
@@ -682,6 +683,7 @@ static int nftnl_ruleset_xml_parse(const void *xml, struct nftnl_parse_err *err,
 
 	ctx.cb = cb;
 	ctx.format = type;
+	ctx.flags = 0;
 
 	ctx.set_list = nftnl_set_list_alloc();
 	if (ctx.set_list == NULL)
-- 
2.8.2

Re: [libnftnl PATCH 2/7] ruleset: Prevent memleak in nftnl_ruleset_snprintf_*() functions

[Pablo Neira Ayuso]

On Fri, Aug 12, 2016 at 01:33:34AM +0200, Phil Sutter wrote:
> From: Phil Sutter <psutter@redhat.com>
> 
> This is an ugly aspect of the SNPRINTF_BUFFER_SIZE() macro: it contains
> a return statement and if that triggers, the function returns without
> freeing the iterator object. Therefore duplicate the 'ret < 0' check
> before calling it, freeing the iterator knowing that we will bail out
> immediately afterwards anyway.
> 
> Cc: Arturo Borrero <arturo.borrero.glez@gmail.com>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/ruleset.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/src/ruleset.c b/src/ruleset.c
> index 666bcc7a246b6..93cf95ab61e15 100644
> --- a/src/ruleset.c
> +++ b/src/ruleset.c
> @@ -888,12 +888,16 @@ nftnl_ruleset_snprintf_table(char *buf, size_t size,
>  	t = nftnl_table_list_iter_next(ti);
>  	while (t != NULL) {
>  		ret = nftnl_table_snprintf(buf+offset, len, t, type, flags);
> +		if (ret < 0)
> +			nftnl_table_list_iter_destroy(ti);
>  		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);

Better get rid of the obscure if (ret < 0) hidden in
SNPRINT_BUFFER_SIZE.

Or simply set:

        if (ret < 0)
                ret = 0;

in SNPRINT_BUFFER_SIZE.

Re: [libnftnl PATCH 5/7] common: Avoid integer overflow in nftnl_batch_is_supported()

[Pablo Neira Ayuso]

On Fri, Aug 12, 2016 at 01:33:37AM +0200, Phil Sutter wrote:
> time() may return -1 which is then assigned to an unsigned integer type
> and used as sequence number. The following code increments that number
> multiple times, so it may overflow and get libmnl confused. To avoid
> this, fall back to a starting sequence number of zero in case the call
> to time() failed.

(uint32_t)-1 should be fine for netlink as sequence number.

Re: [libnftnl PATCH 0/7] A bunch of covscan detected fixes

[Pablo Neira Ayuso]

On Fri, Aug 12, 2016 at 01:33:32AM +0200, Phil Sutter wrote:
> The following series aims at fixing a number of issues identified by
> Coverity tool. Due to limited familiarity with the whole code layout, I
> am not sure all of them are really valid, but I tried my best to verify
> the concerns are legitimate and worth fixing.

Applied, 1/7, 3/7, 4/7, 5/7, 7/7.

Kept back 2/7 and 6/7.

I think 2/7 you can send a new version.

Thanks Phil.

Re: [libnftnl PATCH 0/7] A bunch of covscan detected fixes

[Pablo Neira Ayuso]

On Fri, Aug 12, 2016 at 01:58:17AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Aug 12, 2016 at 01:33:32AM +0200, Phil Sutter wrote:
> > The following series aims at fixing a number of issues identified by
> > Coverity tool. Due to limited familiarity with the whole code layout, I
> > am not sure all of them are really valid, but I tried my best to verify
> > the concerns are legitimate and worth fixing.
> 
> Applied, 1/7, 3/7, 4/7, 5/7, 7/7.
> 
> Kept back 2/7 and 6/7.

Sorry, I mean I kept back 5/7.

> I think 2/7 you can send a new version.
> 
> Thanks Phil.

Re: [libnftnl PATCH 2/7] ruleset: Prevent memleak in nftnl_ruleset_snprintf_*() functions

[Phil Sutter]

On Fri, Aug 12, 2016 at 01:42:02AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Aug 12, 2016 at 01:33:34AM +0200, Phil Sutter wrote:
> > From: Phil Sutter <psutter@redhat.com>
> > 
> > This is an ugly aspect of the SNPRINTF_BUFFER_SIZE() macro: it contains
> > a return statement and if that triggers, the function returns without
> > freeing the iterator object. Therefore duplicate the 'ret < 0' check
> > before calling it, freeing the iterator knowing that we will bail out
> > immediately afterwards anyway.
> > 
> > Cc: Arturo Borrero <arturo.borrero.glez@gmail.com>
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  src/ruleset.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/src/ruleset.c b/src/ruleset.c
> > index 666bcc7a246b6..93cf95ab61e15 100644
> > --- a/src/ruleset.c
> > +++ b/src/ruleset.c
> > @@ -888,12 +888,16 @@ nftnl_ruleset_snprintf_table(char *buf, size_t size,
> >  	t = nftnl_table_list_iter_next(ti);
> >  	while (t != NULL) {
> >  		ret = nftnl_table_snprintf(buf+offset, len, t, type, flags);
> > +		if (ret < 0)
> > +			nftnl_table_list_iter_destroy(ti);
> >  		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> 
> Better get rid of the obscure if (ret < 0) hidden in
> SNPRINT_BUFFER_SIZE.
> 
> Or simply set:
> 
>         if (ret < 0)
>                 ret = 0;
> 
> in SNPRINT_BUFFER_SIZE.

Hmm. This means we will lose error propagation. Given how widely this
macro is being used (grep says 200 calls), this needs a good second
thought.

Thanks, Phil

Re: [libnftnl PATCH 0/7] A bunch of covscan detected fixes

[Phil Sutter]

On Fri, Aug 12, 2016 at 02:05:54AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Aug 12, 2016 at 01:58:17AM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Aug 12, 2016 at 01:33:32AM +0200, Phil Sutter wrote:
> > > The following series aims at fixing a number of issues identified by
> > > Coverity tool. Due to limited familiarity with the whole code layout, I
> > > am not sure all of them are really valid, but I tried my best to verify
> > > the concerns are legitimate and worth fixing.
> > 
> > Applied, 1/7, 3/7, 4/7, 5/7, 7/7.
> > 
> > Kept back 2/7 and 6/7.
> 
> Sorry, I mean I kept back 5/7.

Actually, you didn't. Patch 5/7 slipped through (d26feca2c9c19). Feel
free to revert if it's useless.

Thanks, Phil

Re: [libnftnl PATCH 2/7] ruleset: Prevent memleak in nftnl_ruleset_snprintf_*() functions

[Pablo Neira Ayuso]

On Fri, Aug 12, 2016 at 02:44:58AM +0200, Phil Sutter wrote:
> On Fri, Aug 12, 2016 at 01:42:02AM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Aug 12, 2016 at 01:33:34AM +0200, Phil Sutter wrote:
> > > From: Phil Sutter <psutter@redhat.com>
> > > 
> > > This is an ugly aspect of the SNPRINTF_BUFFER_SIZE() macro: it contains
> > > a return statement and if that triggers, the function returns without
> > > freeing the iterator object. Therefore duplicate the 'ret < 0' check
> > > before calling it, freeing the iterator knowing that we will bail out
> > > immediately afterwards anyway.
> > > 
> > > Cc: Arturo Borrero <arturo.borrero.glez@gmail.com>
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  src/ruleset.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/src/ruleset.c b/src/ruleset.c
> > > index 666bcc7a246b6..93cf95ab61e15 100644
> > > --- a/src/ruleset.c
> > > +++ b/src/ruleset.c
> > > @@ -888,12 +888,16 @@ nftnl_ruleset_snprintf_table(char *buf, size_t size,
> > >  	t = nftnl_table_list_iter_next(ti);
> > >  	while (t != NULL) {
> > >  		ret = nftnl_table_snprintf(buf+offset, len, t, type, flags);
> > > +		if (ret < 0)
> > > +			nftnl_table_list_iter_destroy(ti);
> > >  		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> > 
> > Better get rid of the obscure if (ret < 0) hidden in
> > SNPRINT_BUFFER_SIZE.
> > 
> > Or simply set:
> > 
> >         if (ret < 0)
> >                 ret = 0;
> > 
> > in SNPRINT_BUFFER_SIZE.
> 
> Hmm. This means we will lose error propagation. Given how widely this
> macro is being used (grep says 200 calls), this needs a good second
> thought.

The usual suggested idiom to deal with this looks like:

    snprintf(cp, blen, "AF=%d ", sau->soa.sa_family)
    blen -= strlen(cp);
    cp += strlen(cp);

See: https://goo.gl/AUGE5m

No need to second thought, we can just ignore the -1 case.

Re: [libnftnl PATCH 0/7] A bunch of covscan detected fixes

[Pablo Neira Ayuso]

On Fri, Aug 12, 2016 at 02:47:46AM +0200, Phil Sutter wrote:
> On Fri, Aug 12, 2016 at 02:05:54AM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Aug 12, 2016 at 01:58:17AM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Aug 12, 2016 at 01:33:32AM +0200, Phil Sutter wrote:
> > > > The following series aims at fixing a number of issues identified by
> > > > Coverity tool. Due to limited familiarity with the whole code layout, I
> > > > am not sure all of them are really valid, but I tried my best to verify
> > > > the concerns are legitimate and worth fixing.
> > > 
> > > Applied, 1/7, 3/7, 4/7, 5/7, 7/7.
> > > 
> > > Kept back 2/7 and 6/7.
> > 
> > Sorry, I mean I kept back 5/7.
> 
> Actually, you didn't. Patch 5/7 slipped through (d26feca2c9c19). Feel
> free to revert if it's useless.

Done.