[libnftnl PATCH 0/6] A series of covscan-indicated fixes

[libnftnl PATCH 0/6] A series of covscan-indicated fixes

[Phil Sutter]

Fix (potential) issues identified by Coverity tool.

Phil Sutter (6):
  obj: ct_timeout: Check return code of mnl_attr_parse_nested()
  set_elem: Fix return code of nftnl_set_elem_set()
  set_elem: Validate nftnl_set_elem_set() parameters
  set: Don't bypass checks in nftnl_set_set_u{32,64}()
  obj/ct_timeout: Avoid array overrun in timeout_parse_attr_data()
  obj/tunnel: Fix for undefined behaviour

 include/libnftnl/set.h |  2 ++
 include/utils.h        |  8 ++++++++
 src/obj/ct_timeout.c   | 11 +++++++----
 src/obj/tunnel.c       |  6 +++---
 src/set.c              |  5 +++--
 src/set_elem.c         | 12 +++++++++++-
 6 files changed, 34 insertions(+), 10 deletions(-)

-- 
2.23.0

[libnftnl PATCH 1/6] obj: ct_timeout: Check return code of mnl_attr_parse_nested()

[Phil Sutter]

Don't ignore nested attribute parsing errors, this may hide bugs in
users' code.

Fixes: 0adceeab1597a ("src: add ct timeout support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/obj/ct_timeout.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/obj/ct_timeout.c b/src/obj/ct_timeout.c
index e2e99917de7ae..a439432deee18 100644
--- a/src/obj/ct_timeout.c
+++ b/src/obj/ct_timeout.c
@@ -116,7 +116,7 @@ parse_timeout_attr_policy_cb(const struct nlattr *attr, void *data)
 	return MNL_CB_OK;
 }
 
-static void
+static int
 timeout_parse_attr_data(struct nftnl_obj *e,
 			const struct nlattr *nest)
 {
@@ -131,7 +131,8 @@ timeout_parse_attr_data(struct nftnl_obj *e,
 
 	memset(tb, 0, sizeof(struct nlattr *) * attr_max);
 
-	mnl_attr_parse_nested(nest, parse_timeout_attr_policy_cb, &cnt);
+	if (mnl_attr_parse_nested(nest, parse_timeout_attr_policy_cb, &cnt) < 0)
+		return -1;
 
 	for (i = 1; i <= attr_max; i++) {
 		if (tb[i]) {
@@ -139,6 +140,7 @@ timeout_parse_attr_data(struct nftnl_obj *e,
 				ntohl(mnl_attr_get_u32(tb[i])));
 		}
 	}
+	return 0;
 }
 
 static int nftnl_obj_ct_timeout_set(struct nftnl_obj *e, uint16_t type,
@@ -248,7 +250,8 @@ nftnl_obj_ct_timeout_parse(struct nftnl_obj *e, struct nlattr *attr)
 		e->flags |= (1 << NFTNL_OBJ_CT_TIMEOUT_L4PROTO);
 	}
 	if (tb[NFTA_CT_TIMEOUT_DATA]) {
-		timeout_parse_attr_data(e, tb[NFTA_CT_TIMEOUT_DATA]);
+		if (timeout_parse_attr_data(e, tb[NFTA_CT_TIMEOUT_DATA]) < 0)
+			return -1;
 		e->flags |= (1 << NFTNL_OBJ_CT_TIMEOUT_ARRAY);
 	}
 	return 0;
-- 
2.23.0

[libnftnl PATCH 2/6] set_elem: Fix return code of nftnl_set_elem_set()

[Phil Sutter]

The function returned -1 on success.

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

diff --git a/src/set_elem.c b/src/set_elem.c
index 47965249691dd..3794f12594079 100644
--- a/src/set_elem.c
+++ b/src/set_elem.c
@@ -149,7 +149,7 @@ int nftnl_set_elem_set(struct nftnl_set_elem *s, uint16_t attr,
 		break;
 	}
 	s->flags |= (1 << attr);
-	return -1;
+	return 0;
 }
 
 EXPORT_SYMBOL(nftnl_set_elem_set_u32);
-- 
2.23.0

[libnftnl PATCH 3/6] set_elem: Validate nftnl_set_elem_set() parameters

[Phil Sutter]

Copying from nftnl_table_set_data(), validate input to
nftnl_set_elem_set() as well. Given that for some attributes the
function assumes passed data size, this seems necessary.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/libnftnl/set.h |  2 ++
 src/set_elem.c         | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/libnftnl/set.h b/include/libnftnl/set.h
index 6640ad929f346..2ea2e9a56ce4f 100644
--- a/include/libnftnl/set.h
+++ b/include/libnftnl/set.h
@@ -104,7 +104,9 @@ enum {
 	NFTNL_SET_ELEM_USERDATA,
 	NFTNL_SET_ELEM_EXPR,
 	NFTNL_SET_ELEM_OBJREF,
+	__NFTNL_SET_ELEM_MAX
 };
+#define NFTNL_SET_ELEM_MAX (__NFTNL_SET_ELEM_MAX - 1)
 
 struct nftnl_set_elem;
 
diff --git a/src/set_elem.c b/src/set_elem.c
index 3794f12594079..4225a96ee5a0a 100644
--- a/src/set_elem.c
+++ b/src/set_elem.c
@@ -96,10 +96,20 @@ void nftnl_set_elem_unset(struct nftnl_set_elem *s, uint16_t attr)
 	s->flags &= ~(1 << attr);
 }
 
+static uint32_t nftnl_set_elem_validate[NFTNL_SET_ELEM_MAX + 1] = {
+	[NFTNL_SET_ELEM_FLAGS]		= sizeof(uint32_t),
+	[NFTNL_SET_ELEM_VERDICT]	= sizeof(int), /* FIXME: data.verdict is int?! */
+	[NFTNL_SET_ELEM_TIMEOUT]	= sizeof(uint64_t),
+	[NFTNL_SET_ELEM_EXPIRATION]	= sizeof(uint64_t),
+};
+
 EXPORT_SYMBOL(nftnl_set_elem_set);
 int nftnl_set_elem_set(struct nftnl_set_elem *s, uint16_t attr,
 		       const void *data, uint32_t data_len)
 {
+	nftnl_assert_attr_exists(attr, NFTNL_SET_ELEM_MAX);
+	nftnl_assert_validate(data, nftnl_set_elem_validate, attr, data_len);
+
 	switch(attr) {
 	case NFTNL_SET_ELEM_FLAGS:
 		memcpy(&s->set_elem_flags, data, sizeof(s->set_elem_flags));
-- 
2.23.0

[libnftnl PATCH 4/6] set: Don't bypass checks in nftnl_set_set_u{32,64}()

[Phil Sutter]

By calling nftnl_set_set(), any data size checks are effectively
bypassed. Better call nftnl_set_set_data() directly, passing the real
size for validation.

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

diff --git a/src/set.c b/src/set.c
index e6db7258cc224..b1ffe7e6de975 100644
--- a/src/set.c
+++ b/src/set.c
@@ -195,6 +195,7 @@ int nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data,
 	return 0;
 }
 
+/* XXX: Deprecate this, it is simply unsafe */
 EXPORT_SYMBOL(nftnl_set_set);
 int nftnl_set_set(struct nftnl_set *s, uint16_t attr, const void *data)
 {
@@ -204,13 +205,13 @@ int nftnl_set_set(struct nftnl_set *s, uint16_t attr, const void *data)
 EXPORT_SYMBOL(nftnl_set_set_u32);
 void nftnl_set_set_u32(struct nftnl_set *s, uint16_t attr, uint32_t val)
 {
-	nftnl_set_set(s, attr, &val);
+	nftnl_set_set_data(s, attr, &val, sizeof(uint32_t));
 }
 
 EXPORT_SYMBOL(nftnl_set_set_u64);
 void nftnl_set_set_u64(struct nftnl_set *s, uint16_t attr, uint64_t val)
 {
-	nftnl_set_set(s, attr, &val);
+	nftnl_set_set_data(s, attr, &val, sizeof(uint64_t));
 }
 
 EXPORT_SYMBOL(nftnl_set_set_str);
-- 
2.23.0

[libnftnl PATCH 5/6] obj/ct_timeout: Avoid array overrun in timeout_parse_attr_data()

[Phil Sutter]

Array 'tb' has only 'attr_max' elements, the loop overstepped its
boundary by one. Copy array_size() macro from include/utils.h in
nftables.git to make sure code does the right thing.

Fixes: 0adceeab1597a ("src: add ct timeout support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/utils.h      | 8 ++++++++
 src/obj/ct_timeout.c | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/utils.h b/include/utils.h
index 3cc659652fe2e..91fbebb1956fd 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -58,6 +58,14 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
 		ret = remain;				\
 	remain -= ret;					\
 
+
+#define BUILD_BUG_ON_ZERO(e)	(sizeof(char[1 - 2 * !!(e)]) - 1)
+
+#define __must_be_array(a) \
+	BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
+
+#define array_size(arr)		(sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
+
 const char *nftnl_family2str(uint32_t family);
 int nftnl_str2family(const char *family);
 
diff --git a/src/obj/ct_timeout.c b/src/obj/ct_timeout.c
index a439432deee18..a09e25ae5d44f 100644
--- a/src/obj/ct_timeout.c
+++ b/src/obj/ct_timeout.c
@@ -134,7 +134,7 @@ timeout_parse_attr_data(struct nftnl_obj *e,
 	if (mnl_attr_parse_nested(nest, parse_timeout_attr_policy_cb, &cnt) < 0)
 		return -1;
 
-	for (i = 1; i <= attr_max; i++) {
+	for (i = 1; i < array_size(tb); i++) {
 		if (tb[i]) {
 			nftnl_timeout_policy_attr_set_u32(e, i-1,
 				ntohl(mnl_attr_get_u32(tb[i])));
-- 
2.23.0

[libnftnl PATCH 6/6] obj/tunnel: Fix for undefined behaviour

[Phil Sutter]

Cppcheck complains: Shifting signed 32-bit value by 31 bits is undefined
behaviour.

Indeed, NFTNL_OBJ_TUNNEL_ERSPAN_V2_DIR enum value is 31. Make sure
behaviour is as intended by shifting unsigned 1.

Fixes: ea63a05272f54 ("obj: add tunnel support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/obj/tunnel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/obj/tunnel.c b/src/obj/tunnel.c
index 7ffade8c46ae7..100aa099c6e97 100644
--- a/src/obj/tunnel.c
+++ b/src/obj/tunnel.c
@@ -227,7 +227,7 @@ nftnl_obj_tunnel_build(struct nlmsghdr *nlh, const struct nftnl_obj *e)
 	if (e->flags & (1 << NFTNL_OBJ_TUNNEL_ERSPAN_VERSION) &&
 	    (e->flags & (1 << NFTNL_OBJ_TUNNEL_ERSPAN_V1_INDEX) ||
 	     (e->flags & (1 << NFTNL_OBJ_TUNNEL_ERSPAN_V2_HWID) &&
-	      e->flags & (1 << NFTNL_OBJ_TUNNEL_ERSPAN_V2_DIR)))) {
+	      e->flags & (1u << NFTNL_OBJ_TUNNEL_ERSPAN_V2_DIR)))) {
 		struct nlattr *nest_inner;
 
 		nest = mnl_attr_nest_start(nlh, NFTA_TUNNEL_KEY_OPTS);
@@ -240,7 +240,7 @@ nftnl_obj_tunnel_build(struct nlmsghdr *nlh, const struct nftnl_obj *e)
 		if (e->flags & (1 << NFTNL_OBJ_TUNNEL_ERSPAN_V2_HWID))
 			mnl_attr_put_u8(nlh, NFTA_TUNNEL_KEY_ERSPAN_V2_HWID,
 					tun->u.tun_erspan.u.v2.hwid);
-		if (e->flags & (1 << NFTNL_OBJ_TUNNEL_ERSPAN_V2_DIR))
+		if (e->flags & (1u << NFTNL_OBJ_TUNNEL_ERSPAN_V2_DIR))
 			mnl_attr_put_u8(nlh, NFTA_TUNNEL_KEY_ERSPAN_V2_DIR,
 					tun->u.tun_erspan.u.v2.dir);
 		mnl_attr_nest_end(nlh, nest_inner);
@@ -430,7 +430,7 @@ nftnl_obj_tunnel_parse_erspan(struct nftnl_obj *e, struct nlattr *attr,
 	if (tb[NFTA_TUNNEL_KEY_ERSPAN_V2_DIR]) {
 		tun->u.tun_erspan.u.v2.dir =
 			mnl_attr_get_u8(tb[NFTA_TUNNEL_KEY_ERSPAN_V2_DIR]);
-		e->flags |= (1 << NFTNL_OBJ_TUNNEL_ERSPAN_V2_DIR);
+		e->flags |= (1u << NFTNL_OBJ_TUNNEL_ERSPAN_V2_DIR);
 	}
 
 	return 0;
-- 
2.23.0

Re: [libnftnl PATCH 1/6] obj: ct_timeout: Check return code of mnl_attr_parse_nested()

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 04:16:53PM +0200, Phil Sutter wrote:
> Don't ignore nested attribute parsing errors, this may hide bugs in
> users' code.
> 
> Fixes: 0adceeab1597a ("src: add ct timeout support")
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Re: [libnftnl PATCH 2/6] set_elem: Fix return code of nftnl_set_elem_set()

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 04:16:54PM +0200, Phil Sutter wrote:
> The function returned -1 on success.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Re: [libnftnl PATCH 3/6] set_elem: Validate nftnl_set_elem_set() parameters

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 04:16:55PM +0200, Phil Sutter wrote:
> Copying from nftnl_table_set_data(), validate input to
> nftnl_set_elem_set() as well. Given that for some attributes the
> function assumes passed data size, this seems necessary.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Before pushing out this, see below.

> ---
>  include/libnftnl/set.h |  2 ++
>  src/set_elem.c         | 10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/include/libnftnl/set.h b/include/libnftnl/set.h
> index 6640ad929f346..2ea2e9a56ce4f 100644
> --- a/include/libnftnl/set.h
> +++ b/include/libnftnl/set.h
> @@ -104,7 +104,9 @@ enum {
>  	NFTNL_SET_ELEM_USERDATA,
>  	NFTNL_SET_ELEM_EXPR,
>  	NFTNL_SET_ELEM_OBJREF,
> +	__NFTNL_SET_ELEM_MAX
>  };
> +#define NFTNL_SET_ELEM_MAX (__NFTNL_SET_ELEM_MAX - 1)
>  
>  struct nftnl_set_elem;
>  
> diff --git a/src/set_elem.c b/src/set_elem.c
> index 3794f12594079..4225a96ee5a0a 100644
> --- a/src/set_elem.c
> +++ b/src/set_elem.c
> @@ -96,10 +96,20 @@ void nftnl_set_elem_unset(struct nftnl_set_elem *s, uint16_t attr)
>  	s->flags &= ~(1 << attr);
>  }
>  
> +static uint32_t nftnl_set_elem_validate[NFTNL_SET_ELEM_MAX + 1] = {
> +	[NFTNL_SET_ELEM_FLAGS]		= sizeof(uint32_t),
> +	[NFTNL_SET_ELEM_VERDICT]	= sizeof(int), /* FIXME: data.verdict is int?! */

This is uint32_t, update this before pushing out this.

> +	[NFTNL_SET_ELEM_TIMEOUT]	= sizeof(uint64_t),
> +	[NFTNL_SET_ELEM_EXPIRATION]	= sizeof(uint64_t),
> +};
> +
>  EXPORT_SYMBOL(nftnl_set_elem_set);
>  int nftnl_set_elem_set(struct nftnl_set_elem *s, uint16_t attr,
>  		       const void *data, uint32_t data_len)
>  {
> +	nftnl_assert_attr_exists(attr, NFTNL_SET_ELEM_MAX);
> +	nftnl_assert_validate(data, nftnl_set_elem_validate, attr, data_len);
> +
>  	switch(attr) {
>  	case NFTNL_SET_ELEM_FLAGS:
>  		memcpy(&s->set_elem_flags, data, sizeof(s->set_elem_flags));
> -- 
> 2.23.0
> 

Re: [libnftnl PATCH 4/6] set: Don't bypass checks in nftnl_set_set_u{32,64}()

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 04:16:56PM +0200, Phil Sutter wrote:
> By calling nftnl_set_set(), any data size checks are effectively
> bypassed. Better call nftnl_set_set_data() directly, passing the real
> size for validation.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Probably attribute((deprecated)) is better so we don't forget. Anyway,
we can probably nuke this function in the next release.

> ---
>  src/set.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/set.c b/src/set.c
> index e6db7258cc224..b1ffe7e6de975 100644
> --- a/src/set.c
> +++ b/src/set.c
> @@ -195,6 +195,7 @@ int nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data,
>  	return 0;
>  }
>  
> +/* XXX: Deprecate this, it is simply unsafe */
>  EXPORT_SYMBOL(nftnl_set_set);
>  int nftnl_set_set(struct nftnl_set *s, uint16_t attr, const void *data)
>  {
> @@ -204,13 +205,13 @@ int nftnl_set_set(struct nftnl_set *s, uint16_t attr, const void *data)
>  EXPORT_SYMBOL(nftnl_set_set_u32);
>  void nftnl_set_set_u32(struct nftnl_set *s, uint16_t attr, uint32_t val)
>  {
> -	nftnl_set_set(s, attr, &val);
> +	nftnl_set_set_data(s, attr, &val, sizeof(uint32_t));
>  }
>  
>  EXPORT_SYMBOL(nftnl_set_set_u64);
>  void nftnl_set_set_u64(struct nftnl_set *s, uint16_t attr, uint64_t val)
>  {
> -	nftnl_set_set(s, attr, &val);
> +	nftnl_set_set_data(s, attr, &val, sizeof(uint64_t));
>  }
>  
>  EXPORT_SYMBOL(nftnl_set_set_str);
> -- 
> 2.23.0
> 

Re: [libnftnl PATCH 5/6] obj/ct_timeout: Avoid array overrun in timeout_parse_attr_data()

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 04:16:57PM +0200, Phil Sutter wrote:
> Array 'tb' has only 'attr_max' elements, the loop overstepped its
> boundary by one. Copy array_size() macro from include/utils.h in
> nftables.git to make sure code does the right thing.
> 
> Fixes: 0adceeab1597a ("src: add ct timeout support")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  include/utils.h      | 8 ++++++++
>  src/obj/ct_timeout.c | 2 +-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/include/utils.h b/include/utils.h
> index 3cc659652fe2e..91fbebb1956fd 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -58,6 +58,14 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
>  		ret = remain;				\
>  	remain -= ret;					\
>  
> +
> +#define BUILD_BUG_ON_ZERO(e)	(sizeof(char[1 - 2 * !!(e)]) - 1)
> +
> +#define __must_be_array(a) \
> +	BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
> +
> +#define array_size(arr)		(sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> +
>  const char *nftnl_family2str(uint32_t family);
>  int nftnl_str2family(const char *family);
>  
> diff --git a/src/obj/ct_timeout.c b/src/obj/ct_timeout.c
> index a439432deee18..a09e25ae5d44f 100644
> --- a/src/obj/ct_timeout.c
> +++ b/src/obj/ct_timeout.c
> @@ -134,7 +134,7 @@ timeout_parse_attr_data(struct nftnl_obj *e,
>  	if (mnl_attr_parse_nested(nest, parse_timeout_attr_policy_cb, &cnt) < 0)
>  		return -1;
>  
> -	for (i = 1; i <= attr_max; i++) {
> +	for (i = 1; i < array_size(tb); i++) {

Are you sure this is correct?

array use NFTNL_CTTIMEOUT_* while tb uses netlink NFTA_* attributes.

>  		if (tb[i]) {
>  			nftnl_timeout_policy_attr_set_u32(e, i-1,
>  				ntohl(mnl_attr_get_u32(tb[i])));
> -- 
> 2.23.0
> 

Re: [libnftnl PATCH 6/6] obj/tunnel: Fix for undefined behaviour

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 04:16:58PM +0200, Phil Sutter wrote:
> Cppcheck complains: Shifting signed 32-bit value by 31 bits is undefined
> behaviour.
> 
> Indeed, NFTNL_OBJ_TUNNEL_ERSPAN_V2_DIR enum value is 31. Make sure
> behaviour is as intended by shifting unsigned 1.
> 
> Fixes: ea63a05272f54 ("obj: add tunnel support")
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Re: [libnftnl PATCH 3/6] set_elem: Validate nftnl_set_elem_set() parameters

[Phil Sutter]

On Tue, Oct 15, 2019 at 05:52:44PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 15, 2019 at 04:16:55PM +0200, Phil Sutter wrote:
[...]
> > diff --git a/src/set_elem.c b/src/set_elem.c
> > index 3794f12594079..4225a96ee5a0a 100644
> > --- a/src/set_elem.c
> > +++ b/src/set_elem.c
> > @@ -96,10 +96,20 @@ void nftnl_set_elem_unset(struct nftnl_set_elem *s, uint16_t attr)
> >  	s->flags &= ~(1 << attr);
> >  }
> >  
> > +static uint32_t nftnl_set_elem_validate[NFTNL_SET_ELEM_MAX + 1] = {
> > +	[NFTNL_SET_ELEM_FLAGS]		= sizeof(uint32_t),
> > +	[NFTNL_SET_ELEM_VERDICT]	= sizeof(int), /* FIXME: data.verdict is int?! */
> 
> This is uint32_t, update this before pushing out this.

Oh, sorry. I missed this note to myself.

So, should we change union nftnl_data_reg accordingly then?

Thanks, Phil

Re: [libnftnl PATCH 3/6] set_elem: Validate nftnl_set_elem_set() parameters

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 06:02:55PM +0200, Phil Sutter wrote:
> On Tue, Oct 15, 2019 at 05:52:44PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Oct 15, 2019 at 04:16:55PM +0200, Phil Sutter wrote:
> [...]
> > > diff --git a/src/set_elem.c b/src/set_elem.c
> > > index 3794f12594079..4225a96ee5a0a 100644
> > > --- a/src/set_elem.c
> > > +++ b/src/set_elem.c
> > > @@ -96,10 +96,20 @@ void nftnl_set_elem_unset(struct nftnl_set_elem *s, uint16_t attr)
> > >  	s->flags &= ~(1 << attr);
> > >  }
> > >  
> > > +static uint32_t nftnl_set_elem_validate[NFTNL_SET_ELEM_MAX + 1] = {
> > > +	[NFTNL_SET_ELEM_FLAGS]		= sizeof(uint32_t),
> > > +	[NFTNL_SET_ELEM_VERDICT]	= sizeof(int), /* FIXME: data.verdict is int?! */
> > 
> > This is uint32_t, update this before pushing out this.
> 
> Oh, sorry. I missed this note to myself.
> 
> So, should we change union nftnl_data_reg accordingly then?

I'm seeing this is being used from nftables.git as...

        nftnl_set_elem_set_u32(nlse, NFTNL_SET_ELEM_VERDICT, ...

Re: [libnftnl PATCH 4/6] set: Don't bypass checks in nftnl_set_set_u{32,64}()

[Phil Sutter]

Hi,

On Tue, Oct 15, 2019 at 05:53:46PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 15, 2019 at 04:16:56PM +0200, Phil Sutter wrote:
> > By calling nftnl_set_set(), any data size checks are effectively
> > bypassed. Better call nftnl_set_set_data() directly, passing the real
> > size for validation.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> 
> Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> Probably attribute((deprecated)) is better so we don't forget. Anyway,
> we can probably nuke this function in the next release.

But if we drop it, we break ABI, no? Sadly, nftables use(d) the symbol,
so we would break older nftables versions with the new libnftnl release.

Should I send a v2 setting attribute((deprecated))? I think it's worth
doing it.

Thanks, Phil

Re: [libnftnl PATCH 5/6] obj/ct_timeout: Avoid array overrun in timeout_parse_attr_data()

[Phil Sutter]

Hi,

On Tue, Oct 15, 2019 at 05:57:16PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 15, 2019 at 04:16:57PM +0200, Phil Sutter wrote:
> > Array 'tb' has only 'attr_max' elements, the loop overstepped its
> > boundary by one. Copy array_size() macro from include/utils.h in
> > nftables.git to make sure code does the right thing.
> > 
> > Fixes: 0adceeab1597a ("src: add ct timeout support")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  include/utils.h      | 8 ++++++++
> >  src/obj/ct_timeout.c | 2 +-
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/utils.h b/include/utils.h
> > index 3cc659652fe2e..91fbebb1956fd 100644
> > --- a/include/utils.h
> > +++ b/include/utils.h
> > @@ -58,6 +58,14 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
> >  		ret = remain;				\
> >  	remain -= ret;					\
> >  
> > +
> > +#define BUILD_BUG_ON_ZERO(e)	(sizeof(char[1 - 2 * !!(e)]) - 1)
> > +
> > +#define __must_be_array(a) \
> > +	BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
> > +
> > +#define array_size(arr)		(sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> > +
> >  const char *nftnl_family2str(uint32_t family);
> >  int nftnl_str2family(const char *family);
> >  
> > diff --git a/src/obj/ct_timeout.c b/src/obj/ct_timeout.c
> > index a439432deee18..a09e25ae5d44f 100644
> > --- a/src/obj/ct_timeout.c
> > +++ b/src/obj/ct_timeout.c
> > @@ -134,7 +134,7 @@ timeout_parse_attr_data(struct nftnl_obj *e,
> >  	if (mnl_attr_parse_nested(nest, parse_timeout_attr_policy_cb, &cnt) < 0)
> >  		return -1;
> >  
> > -	for (i = 1; i <= attr_max; i++) {
> > +	for (i = 1; i < array_size(tb); i++) {
> 
> Are you sure this is correct?
> 
> array use NFTNL_CTTIMEOUT_* while tb uses netlink NFTA_* attributes.

The old code can't be correct. Basically it was:

| struct nlattr *tb[attr_max];
[...]
| for (i = 1; i <= attr_max; i++) {
|   if (tb[i]) {
[...]

So in the last round, it accesses 'tb[attr_max]' which is out of bounds.

Regarding the question of whether the array is big enough at all, I had
a look at values in 'timeout_protocol' array struct field values
'attr_max': either NFTNL_CTTIMEOUT_TCP_MAX or NFTNL_CTTIMEOUT_UDP_MAX.
Both are last items in unions so serve only for defining array sizes.
Without checking differences between NFTNL_CTTIMEOUT_* and respective
NFTA_* symbols, I'd bet the array is large enough! :)

Cheers, Phil

Re: [libnftnl PATCH 3/6] set_elem: Validate nftnl_set_elem_set() parameters

[Phil Sutter]

Hi,

On Tue, Oct 15, 2019 at 06:09:13PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 15, 2019 at 06:02:55PM +0200, Phil Sutter wrote:
> > On Tue, Oct 15, 2019 at 05:52:44PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Oct 15, 2019 at 04:16:55PM +0200, Phil Sutter wrote:
> > [...]
> > > > diff --git a/src/set_elem.c b/src/set_elem.c
> > > > index 3794f12594079..4225a96ee5a0a 100644
> > > > --- a/src/set_elem.c
> > > > +++ b/src/set_elem.c
> > > > @@ -96,10 +96,20 @@ void nftnl_set_elem_unset(struct nftnl_set_elem *s, uint16_t attr)
> > > >  	s->flags &= ~(1 << attr);
> > > >  }
> > > >  
> > > > +static uint32_t nftnl_set_elem_validate[NFTNL_SET_ELEM_MAX + 1] = {
> > > > +	[NFTNL_SET_ELEM_FLAGS]		= sizeof(uint32_t),
> > > > +	[NFTNL_SET_ELEM_VERDICT]	= sizeof(int), /* FIXME: data.verdict is int?! */
> > > 
> > > This is uint32_t, update this before pushing out this.
> > 
> > Oh, sorry. I missed this note to myself.
> > 
> > So, should we change union nftnl_data_reg accordingly then?
> 
> I'm seeing this is being used from nftables.git as...
> 
>         nftnl_set_elem_set_u32(nlse, NFTNL_SET_ELEM_VERDICT, ...

Well, there's no nftnl_set_elem_set_int() so it naturally uses that. My
question was whether 'verdict' field in union nftnl_data_reg should be
changed to uint32_t type as well. Currently it is just int, which
doesn't make a difference unless one tries to run nftables on a 16bit
machine. :)

Cheers, Phil

Re: [libnftnl PATCH 4/6] set: Don't bypass checks in nftnl_set_set_u{32,64}()

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 06:11:34PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Tue, Oct 15, 2019 at 05:53:46PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Oct 15, 2019 at 04:16:56PM +0200, Phil Sutter wrote:
> > > By calling nftnl_set_set(), any data size checks are effectively
> > > bypassed. Better call nftnl_set_set_data() directly, passing the real
> > > size for validation.
> > > 
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > 
> > Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > 
> > Probably attribute((deprecated)) is better so we don't forget. Anyway,
> > we can probably nuke this function in the next release.
> 
> But if we drop it, we break ABI, no? Sadly, nftables use(d) the symbol,
> so we would break older nftables versions with the new libnftnl release.
>
> Should I send a v2 setting attribute((deprecated))? I think it's worth
> doing it.

OK.

Re: [libnftnl PATCH 5/6] obj/ct_timeout: Avoid array overrun in timeout_parse_attr_data()

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 06:21:30PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Tue, Oct 15, 2019 at 05:57:16PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Oct 15, 2019 at 04:16:57PM +0200, Phil Sutter wrote:
> > > Array 'tb' has only 'attr_max' elements, the loop overstepped its
> > > boundary by one. Copy array_size() macro from include/utils.h in
> > > nftables.git to make sure code does the right thing.
> > > 
> > > Fixes: 0adceeab1597a ("src: add ct timeout support")
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  include/utils.h      | 8 ++++++++
> > >  src/obj/ct_timeout.c | 2 +-
> > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/utils.h b/include/utils.h
> > > index 3cc659652fe2e..91fbebb1956fd 100644
> > > --- a/include/utils.h
> > > +++ b/include/utils.h
> > > @@ -58,6 +58,14 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
> > >  		ret = remain;				\
> > >  	remain -= ret;					\
> > >  
> > > +
> > > +#define BUILD_BUG_ON_ZERO(e)	(sizeof(char[1 - 2 * !!(e)]) - 1)
> > > +
> > > +#define __must_be_array(a) \
> > > +	BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
> > > +
> > > +#define array_size(arr)		(sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> > > +
> > >  const char *nftnl_family2str(uint32_t family);
> > >  int nftnl_str2family(const char *family);
> > >  
> > > diff --git a/src/obj/ct_timeout.c b/src/obj/ct_timeout.c
> > > index a439432deee18..a09e25ae5d44f 100644
> > > --- a/src/obj/ct_timeout.c
> > > +++ b/src/obj/ct_timeout.c
> > > @@ -134,7 +134,7 @@ timeout_parse_attr_data(struct nftnl_obj *e,
> > >  	if (mnl_attr_parse_nested(nest, parse_timeout_attr_policy_cb, &cnt) < 0)
> > >  		return -1;
> > >  
> > > -	for (i = 1; i <= attr_max; i++) {
> > > +	for (i = 1; i < array_size(tb); i++) {
> > 
> > Are you sure this is correct?
> > 
> > array use NFTNL_CTTIMEOUT_* while tb uses netlink NFTA_* attributes.
> 
> The old code can't be correct. Basically it was:
> 
> | struct nlattr *tb[attr_max];
> [...]
> | for (i = 1; i <= attr_max; i++) {
> |   if (tb[i]) {
> [...]
> 
> So in the last round, it accesses 'tb[attr_max]' which is out of bounds.

I see, thanks for explaining.

> Regarding the question of whether the array is big enough at all, I had
> a look at values in 'timeout_protocol' array struct field values
> 'attr_max': either NFTNL_CTTIMEOUT_TCP_MAX or NFTNL_CTTIMEOUT_UDP_MAX.
> Both are last items in unions so serve only for defining array sizes.
> Without checking differences between NFTNL_CTTIMEOUT_* and respective
> NFTA_* symbols, I'd bet the array is large enough! :)

OK!

Re: [libnftnl PATCH 5/6] obj/ct_timeout: Avoid array overrun in timeout_parse_attr_data()

[Phil Sutter]

On Tue, Oct 15, 2019 at 06:33:46PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 15, 2019 at 06:21:30PM +0200, Phil Sutter wrote:
> > Hi,
> > 
> > On Tue, Oct 15, 2019 at 05:57:16PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Oct 15, 2019 at 04:16:57PM +0200, Phil Sutter wrote:
> > > > Array 'tb' has only 'attr_max' elements, the loop overstepped its
> > > > boundary by one. Copy array_size() macro from include/utils.h in
> > > > nftables.git to make sure code does the right thing.
> > > > 
> > > > Fixes: 0adceeab1597a ("src: add ct timeout support")
> > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > ---
> > > >  include/utils.h      | 8 ++++++++
> > > >  src/obj/ct_timeout.c | 2 +-
> > > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/utils.h b/include/utils.h
> > > > index 3cc659652fe2e..91fbebb1956fd 100644
> > > > --- a/include/utils.h
> > > > +++ b/include/utils.h
> > > > @@ -58,6 +58,14 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
> > > >  		ret = remain;				\
> > > >  	remain -= ret;					\
> > > >  
> > > > +
> > > > +#define BUILD_BUG_ON_ZERO(e)	(sizeof(char[1 - 2 * !!(e)]) - 1)
> > > > +
> > > > +#define __must_be_array(a) \
> > > > +	BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
> > > > +
> > > > +#define array_size(arr)		(sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> > > > +
> > > >  const char *nftnl_family2str(uint32_t family);
> > > >  int nftnl_str2family(const char *family);
> > > >  
> > > > diff --git a/src/obj/ct_timeout.c b/src/obj/ct_timeout.c
> > > > index a439432deee18..a09e25ae5d44f 100644
> > > > --- a/src/obj/ct_timeout.c
> > > > +++ b/src/obj/ct_timeout.c
> > > > @@ -134,7 +134,7 @@ timeout_parse_attr_data(struct nftnl_obj *e,
> > > >  	if (mnl_attr_parse_nested(nest, parse_timeout_attr_policy_cb, &cnt) < 0)
> > > >  		return -1;
> > > >  
> > > > -	for (i = 1; i <= attr_max; i++) {
> > > > +	for (i = 1; i < array_size(tb); i++) {
> > > 
> > > Are you sure this is correct?
> > > 
> > > array use NFTNL_CTTIMEOUT_* while tb uses netlink NFTA_* attributes.
> > 
> > The old code can't be correct. Basically it was:
> > 
> > | struct nlattr *tb[attr_max];
> > [...]
> > | for (i = 1; i <= attr_max; i++) {
> > |   if (tb[i]) {
> > [...]
> > 
> > So in the last round, it accesses 'tb[attr_max]' which is out of bounds.
> 
> I see, thanks for explaining.
> 
> > Regarding the question of whether the array is big enough at all, I had
> > a look at values in 'timeout_protocol' array struct field values
> > 'attr_max': either NFTNL_CTTIMEOUT_TCP_MAX or NFTNL_CTTIMEOUT_UDP_MAX.
> > Both are last items in unions so serve only for defining array sizes.
> > Without checking differences between NFTNL_CTTIMEOUT_* and respective
> > NFTA_* symbols, I'd bet the array is large enough! :)
> 
> OK!

Is that a synonym for ACK? ;)

Re: [libnftnl PATCH 3/6] set_elem: Validate nftnl_set_elem_set() parameters

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 06:25:59PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Tue, Oct 15, 2019 at 06:09:13PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Oct 15, 2019 at 06:02:55PM +0200, Phil Sutter wrote:
> > > On Tue, Oct 15, 2019 at 05:52:44PM +0200, Pablo Neira Ayuso wrote:
> > > > On Tue, Oct 15, 2019 at 04:16:55PM +0200, Phil Sutter wrote:
> > > [...]
> > > > > diff --git a/src/set_elem.c b/src/set_elem.c
> > > > > index 3794f12594079..4225a96ee5a0a 100644
> > > > > --- a/src/set_elem.c
> > > > > +++ b/src/set_elem.c
> > > > > @@ -96,10 +96,20 @@ void nftnl_set_elem_unset(struct nftnl_set_elem *s, uint16_t attr)
> > > > >  	s->flags &= ~(1 << attr);
> > > > >  }
> > > > >  
> > > > > +static uint32_t nftnl_set_elem_validate[NFTNL_SET_ELEM_MAX + 1] = {
> > > > > +	[NFTNL_SET_ELEM_FLAGS]		= sizeof(uint32_t),
> > > > > +	[NFTNL_SET_ELEM_VERDICT]	= sizeof(int), /* FIXME: data.verdict is int?! */
> > > > 
> > > > This is uint32_t, update this before pushing out this.
> > > 
> > > Oh, sorry. I missed this note to myself.
> > > 
> > > So, should we change union nftnl_data_reg accordingly then?
> > 
> > I'm seeing this is being used from nftables.git as...
> > 
> >         nftnl_set_elem_set_u32(nlse, NFTNL_SET_ELEM_VERDICT, ...
> 
> Well, there's no nftnl_set_elem_set_int() so it naturally uses that. My
> question was whether 'verdict' field in union nftnl_data_reg should be
> changed to uint32_t type as well. Currently it is just int, which
> doesn't make a difference unless one tries to run nftables on a 16bit
> machine. :)

ok

Re: [libnftnl PATCH 5/6] obj/ct_timeout: Avoid array overrun in timeout_parse_attr_data()

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 06:35:29PM +0200, Phil Sutter wrote:
> On Tue, Oct 15, 2019 at 06:33:46PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Oct 15, 2019 at 06:21:30PM +0200, Phil Sutter wrote:
> > > Hi,
> > > 
> > > On Tue, Oct 15, 2019 at 05:57:16PM +0200, Pablo Neira Ayuso wrote:
> > > > On Tue, Oct 15, 2019 at 04:16:57PM +0200, Phil Sutter wrote:
> > > > > Array 'tb' has only 'attr_max' elements, the loop overstepped its
> > > > > boundary by one. Copy array_size() macro from include/utils.h in
> > > > > nftables.git to make sure code does the right thing.
> > > > > 
> > > > > Fixes: 0adceeab1597a ("src: add ct timeout support")
> > > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > > ---
> > > > >  include/utils.h      | 8 ++++++++
> > > > >  src/obj/ct_timeout.c | 2 +-
> > > > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/include/utils.h b/include/utils.h
> > > > > index 3cc659652fe2e..91fbebb1956fd 100644
> > > > > --- a/include/utils.h
> > > > > +++ b/include/utils.h
> > > > > @@ -58,6 +58,14 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
> > > > >  		ret = remain;				\
> > > > >  	remain -= ret;					\
> > > > >  
> > > > > +
> > > > > +#define BUILD_BUG_ON_ZERO(e)	(sizeof(char[1 - 2 * !!(e)]) - 1)
> > > > > +
> > > > > +#define __must_be_array(a) \
> > > > > +	BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
> > > > > +
> > > > > +#define array_size(arr)		(sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> > > > > +
> > > > >  const char *nftnl_family2str(uint32_t family);
> > > > >  int nftnl_str2family(const char *family);
> > > > >  
> > > > > diff --git a/src/obj/ct_timeout.c b/src/obj/ct_timeout.c
> > > > > index a439432deee18..a09e25ae5d44f 100644
> > > > > --- a/src/obj/ct_timeout.c
> > > > > +++ b/src/obj/ct_timeout.c
> > > > > @@ -134,7 +134,7 @@ timeout_parse_attr_data(struct nftnl_obj *e,
> > > > >  	if (mnl_attr_parse_nested(nest, parse_timeout_attr_policy_cb, &cnt) < 0)
> > > > >  		return -1;
> > > > >  
> > > > > -	for (i = 1; i <= attr_max; i++) {
> > > > > +	for (i = 1; i < array_size(tb); i++) {
> > > > 
> > > > Are you sure this is correct?
> > > > 
> > > > array use NFTNL_CTTIMEOUT_* while tb uses netlink NFTA_* attributes.
> > > 
> > > The old code can't be correct. Basically it was:
> > > 
> > > | struct nlattr *tb[attr_max];
> > > [...]
> > > | for (i = 1; i <= attr_max; i++) {
> > > |   if (tb[i]) {
> > > [...]
> > > 
> > > So in the last round, it accesses 'tb[attr_max]' which is out of bounds.
> > 
> > I see, thanks for explaining.
> > 
> > > Regarding the question of whether the array is big enough at all, I had
> > > a look at values in 'timeout_protocol' array struct field values
> > > 'attr_max': either NFTNL_CTTIMEOUT_TCP_MAX or NFTNL_CTTIMEOUT_UDP_MAX.
> > > Both are last items in unions so serve only for defining array sizes.
> > > Without checking differences between NFTNL_CTTIMEOUT_* and respective
> > > NFTA_* symbols, I'd bet the array is large enough! :)
> > 
> > OK!
> 
> Is that a synonym for ACK? ;)

I think ct_timeout needs a review on all this small things, I don't
have time to look in detail right now. The fact that NFTNL_ and NFTA_
are being mixed might be problems, even if the array is large enough
right now.

Re: [libnftnl PATCH 4/6] set: Don't bypass checks in nftnl_set_set_u{32,64}()

[Phil Sutter]

On Tue, Oct 15, 2019 at 06:32:39PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 15, 2019 at 06:11:34PM +0200, Phil Sutter wrote:
> > Hi,
> > 
> > On Tue, Oct 15, 2019 at 05:53:46PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Oct 15, 2019 at 04:16:56PM +0200, Phil Sutter wrote:
> > > > By calling nftnl_set_set(), any data size checks are effectively
> > > > bypassed. Better call nftnl_set_set_data() directly, passing the real
> > > > size for validation.
> > > > 
> > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > 
> > > Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > 
> > > Probably attribute((deprecated)) is better so we don't forget. Anyway,
> > > we can probably nuke this function in the next release.
> > 
> > But if we drop it, we break ABI, no? Sadly, nftables use(d) the symbol,
> > so we would break older nftables versions with the new libnftnl release.
> >
> > Should I send a v2 setting attribute((deprecated))? I think it's worth
> > doing it.
> 
> OK.

Well, given that there are more cases like this (e.g. nftnl_obj_set()),
I'll just drop the comment from existing patch and follow-up with a
separate one deprecating all unqualified setter symbols at once.

Cheers, Phil

Re: [libnftnl PATCH 5/6] obj/ct_timeout: Avoid array overrun in timeout_parse_attr_data()

[Phil Sutter]

On Tue, Oct 15, 2019 at 06:37:21PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 15, 2019 at 06:35:29PM +0200, Phil Sutter wrote:
> > On Tue, Oct 15, 2019 at 06:33:46PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Oct 15, 2019 at 06:21:30PM +0200, Phil Sutter wrote:
> > > > Hi,
> > > > 
> > > > On Tue, Oct 15, 2019 at 05:57:16PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Tue, Oct 15, 2019 at 04:16:57PM +0200, Phil Sutter wrote:
> > > > > > Array 'tb' has only 'attr_max' elements, the loop overstepped its
> > > > > > boundary by one. Copy array_size() macro from include/utils.h in
> > > > > > nftables.git to make sure code does the right thing.
> > > > > > 
> > > > > > Fixes: 0adceeab1597a ("src: add ct timeout support")
> > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > > > ---
> > > > > >  include/utils.h      | 8 ++++++++
> > > > > >  src/obj/ct_timeout.c | 2 +-
> > > > > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/include/utils.h b/include/utils.h
> > > > > > index 3cc659652fe2e..91fbebb1956fd 100644
> > > > > > --- a/include/utils.h
> > > > > > +++ b/include/utils.h
> > > > > > @@ -58,6 +58,14 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
> > > > > >  		ret = remain;				\
> > > > > >  	remain -= ret;					\
> > > > > >  
> > > > > > +
> > > > > > +#define BUILD_BUG_ON_ZERO(e)	(sizeof(char[1 - 2 * !!(e)]) - 1)
> > > > > > +
> > > > > > +#define __must_be_array(a) \
> > > > > > +	BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
> > > > > > +
> > > > > > +#define array_size(arr)		(sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> > > > > > +
> > > > > >  const char *nftnl_family2str(uint32_t family);
> > > > > >  int nftnl_str2family(const char *family);
> > > > > >  
> > > > > > diff --git a/src/obj/ct_timeout.c b/src/obj/ct_timeout.c
> > > > > > index a439432deee18..a09e25ae5d44f 100644
> > > > > > --- a/src/obj/ct_timeout.c
> > > > > > +++ b/src/obj/ct_timeout.c
> > > > > > @@ -134,7 +134,7 @@ timeout_parse_attr_data(struct nftnl_obj *e,
> > > > > >  	if (mnl_attr_parse_nested(nest, parse_timeout_attr_policy_cb, &cnt) < 0)
> > > > > >  		return -1;
> > > > > >  
> > > > > > -	for (i = 1; i <= attr_max; i++) {
> > > > > > +	for (i = 1; i < array_size(tb); i++) {
> > > > > 
> > > > > Are you sure this is correct?
> > > > > 
> > > > > array use NFTNL_CTTIMEOUT_* while tb uses netlink NFTA_* attributes.
> > > > 
> > > > The old code can't be correct. Basically it was:
> > > > 
> > > > | struct nlattr *tb[attr_max];
> > > > [...]
> > > > | for (i = 1; i <= attr_max; i++) {
> > > > |   if (tb[i]) {
> > > > [...]
> > > > 
> > > > So in the last round, it accesses 'tb[attr_max]' which is out of bounds.
> > > 
> > > I see, thanks for explaining.
> > > 
> > > > Regarding the question of whether the array is big enough at all, I had
> > > > a look at values in 'timeout_protocol' array struct field values
> > > > 'attr_max': either NFTNL_CTTIMEOUT_TCP_MAX or NFTNL_CTTIMEOUT_UDP_MAX.
> > > > Both are last items in unions so serve only for defining array sizes.
> > > > Without checking differences between NFTNL_CTTIMEOUT_* and respective
> > > > NFTA_* symbols, I'd bet the array is large enough! :)
> > > 
> > > OK!
> > 
> > Is that a synonym for ACK? ;)
> 
> I think ct_timeout needs a review on all this small things, I don't
> have time to look in detail right now. The fact that NFTNL_ and NFTA_
> are being mixed might be problems, even if the array is large enough
> right now.

Where do you see that mixed use? I just searched through the file for
'NFTA' and compared it to src/obj/ct_expect.c but didn't spot any extra
use.

Anyway, are you fine with the array_size() change by itself?

Thanks, Phil

Re: [libnftnl PATCH 5/6] obj/ct_timeout: Avoid array overrun in timeout_parse_attr_data()

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 07:27:27PM +0200, Phil Sutter wrote:
> On Tue, Oct 15, 2019 at 06:37:21PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Oct 15, 2019 at 06:35:29PM +0200, Phil Sutter wrote:
> > > On Tue, Oct 15, 2019 at 06:33:46PM +0200, Pablo Neira Ayuso wrote:
> > > > On Tue, Oct 15, 2019 at 06:21:30PM +0200, Phil Sutter wrote:
> > > > > Hi,
> > > > > 
> > > > > On Tue, Oct 15, 2019 at 05:57:16PM +0200, Pablo Neira Ayuso wrote:
> > > > > > On Tue, Oct 15, 2019 at 04:16:57PM +0200, Phil Sutter wrote:
> > > > > > > Array 'tb' has only 'attr_max' elements, the loop overstepped its
> > > > > > > boundary by one. Copy array_size() macro from include/utils.h in
> > > > > > > nftables.git to make sure code does the right thing.
> > > > > > > 
> > > > > > > Fixes: 0adceeab1597a ("src: add ct timeout support")
> > > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > > > > ---
> > > > > > >  include/utils.h      | 8 ++++++++
> > > > > > >  src/obj/ct_timeout.c | 2 +-
> > > > > > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/include/utils.h b/include/utils.h
> > > > > > > index 3cc659652fe2e..91fbebb1956fd 100644
> > > > > > > --- a/include/utils.h
> > > > > > > +++ b/include/utils.h
> > > > > > > @@ -58,6 +58,14 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
> > > > > > >  		ret = remain;				\
> > > > > > >  	remain -= ret;					\
> > > > > > >  
> > > > > > > +
> > > > > > > +#define BUILD_BUG_ON_ZERO(e)	(sizeof(char[1 - 2 * !!(e)]) - 1)
> > > > > > > +
> > > > > > > +#define __must_be_array(a) \
> > > > > > > +	BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0])))
> > > > > > > +
> > > > > > > +#define array_size(arr)		(sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> > > > > > > +
> > > > > > >  const char *nftnl_family2str(uint32_t family);
> > > > > > >  int nftnl_str2family(const char *family);
> > > > > > >  
> > > > > > > diff --git a/src/obj/ct_timeout.c b/src/obj/ct_timeout.c
> > > > > > > index a439432deee18..a09e25ae5d44f 100644
> > > > > > > --- a/src/obj/ct_timeout.c
> > > > > > > +++ b/src/obj/ct_timeout.c
> > > > > > > @@ -134,7 +134,7 @@ timeout_parse_attr_data(struct nftnl_obj *e,
> > > > > > >  	if (mnl_attr_parse_nested(nest, parse_timeout_attr_policy_cb, &cnt) < 0)
> > > > > > >  		return -1;
> > > > > > >  
> > > > > > > -	for (i = 1; i <= attr_max; i++) {
> > > > > > > +	for (i = 1; i < array_size(tb); i++) {
> > > > > > 
> > > > > > Are you sure this is correct?
> > > > > > 
> > > > > > array use NFTNL_CTTIMEOUT_* while tb uses netlink NFTA_* attributes.
> > > > > 
> > > > > The old code can't be correct. Basically it was:
> > > > > 
> > > > > | struct nlattr *tb[attr_max];
> > > > > [...]
> > > > > | for (i = 1; i <= attr_max; i++) {
> > > > > |   if (tb[i]) {
> > > > > [...]
> > > > > 
> > > > > So in the last round, it accesses 'tb[attr_max]' which is out of bounds.
> > > > 
> > > > I see, thanks for explaining.
> > > > 
> > > > > Regarding the question of whether the array is big enough at all, I had
> > > > > a look at values in 'timeout_protocol' array struct field values
> > > > > 'attr_max': either NFTNL_CTTIMEOUT_TCP_MAX or NFTNL_CTTIMEOUT_UDP_MAX.
> > > > > Both are last items in unions so serve only for defining array sizes.
> > > > > Without checking differences between NFTNL_CTTIMEOUT_* and respective
> > > > > NFTA_* symbols, I'd bet the array is large enough! :)
> > > > 
> > > > OK!
> > > 
> > > Is that a synonym for ACK? ;)
> > 
> > I think ct_timeout needs a review on all this small things, I don't
> > have time to look in detail right now. The fact that NFTNL_ and NFTA_
> > are being mixed might be problems, even if the array is large enough
> > right now.
> 
> Where do you see that mixed use? I just searched through the file for
> 'NFTA' and compared it to src/obj/ct_expect.c but didn't spot any extra
> use.
> 
> Anyway, are you fine with the array_size() change by itself?

Yes, thanks.

Re: [libnftnl PATCH 4/6] set: Don't bypass checks in nftnl_set_set_u{32,64}()

[Pablo Neira Ayuso]

On Tue, Oct 15, 2019 at 07:09:33PM +0200, Phil Sutter wrote:
> On Tue, Oct 15, 2019 at 06:32:39PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Oct 15, 2019 at 06:11:34PM +0200, Phil Sutter wrote:
> > > Hi,
> > > 
> > > On Tue, Oct 15, 2019 at 05:53:46PM +0200, Pablo Neira Ayuso wrote:
> > > > On Tue, Oct 15, 2019 at 04:16:56PM +0200, Phil Sutter wrote:
> > > > > By calling nftnl_set_set(), any data size checks are effectively
> > > > > bypassed. Better call nftnl_set_set_data() directly, passing the real
> > > > > size for validation.
> > > > > 
> > > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > 
> > > > Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > 
> > > > Probably attribute((deprecated)) is better so we don't forget. Anyway,
> > > > we can probably nuke this function in the next release.
> > > 
> > > But if we drop it, we break ABI, no? Sadly, nftables use(d) the symbol,
> > > so we would break older nftables versions with the new libnftnl release.
> > >
> > > Should I send a v2 setting attribute((deprecated))? I think it's worth
> > > doing it.
> > 
> > OK.
> 
> Well, given that there are more cases like this (e.g. nftnl_obj_set()),
> I'll just drop the comment from existing patch and follow-up with a
> separate one deprecating all unqualified setter symbols at once.

That's fine with me.