Various memory-safety related fixes

Various memory-safety related fixes

[Daniel Gröber]

Hi,

here's a slew of fixes for issues discovered while working on adding a
parser for pretty printed conntrack entries in snprintf_default.c
format.

Most of these were found while debugging my libtheft property tests
with ASan.

--Daniel

PS: Resending because of problems with unencoded 8bit garbage in mail
header.

[libnf_ct resend PATCH 1/8] Handle negative snprintf return values properly

[Daniel Gröber]

Currently the BUFFER_SIZE macro doesn't take negative 'ret' values into
account. A negative return should just be passed through to the caller,
snprintf will already have set 'errno' properly.

Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
---
 include/internal/internal.h | 2 ++
 src/conntrack/api.c         | 6 +++---
 src/conntrack/snprintf.c    | 3 +++
 src/expect/snprintf.c       | 3 +++
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/internal/internal.h b/include/internal/internal.h
index bb44e12..859724b 100644
--- a/include/internal/internal.h
+++ b/include/internal/internal.h
@@ -41,6 +41,8 @@
 #endif
 
 #define BUFFER_SIZE(ret, size, len, offset)		\
+	if(ret < 0)					\
+		return -1;				\
 	size += ret;					\
 	if (ret > len)					\
 		ret = len;				\
diff --git a/src/conntrack/api.c b/src/conntrack/api.c
index ffa5216..78d7d61 100644
--- a/src/conntrack/api.c
+++ b/src/conntrack/api.c
@@ -1099,9 +1099,9 @@ int nfct_catch(struct nfct_handle *h)
  * print the message just after you receive the destroy event. If you want
  * more accurate timestamping, use NFCT_OF_TIMESTAMP.
  *
- * This function returns the size of the information that _would_ have been 
- * written to the buffer, even if there was no room for it. Thus, the
- * behaviour is similar to snprintf.
+ * On error, -1 is returned and errno is set appropiately. Otherwise the
+ * size of what _would_ be written is returned, even if the size of the
+ * buffer is insufficient. This behaviour is similar to snprintf.
  */
 int nfct_snprintf(char *buf,
 		  unsigned int size,
diff --git a/src/conntrack/snprintf.c b/src/conntrack/snprintf.c
index 17ad885..a87c0c9 100644
--- a/src/conntrack/snprintf.c
+++ b/src/conntrack/snprintf.c
@@ -85,6 +85,9 @@ int __snprintf_conntrack(char *buf,
 		return -1;
 	}
 
+        if(size < 0)
+                return size;
+
 	/* NULL terminated string */
 	buf[size+1 > len ? len-1 : size] = '\0';
 
diff --git a/src/expect/snprintf.c b/src/expect/snprintf.c
index 3a104b5..b28ac62 100644
--- a/src/expect/snprintf.c
+++ b/src/expect/snprintf.c
@@ -30,6 +30,9 @@ int __snprintf_expect(char *buf,
 		return -1;
 	}
 
+	if(size < 0)
+		return size;
+
 	/* NULL terminated string */
 	buf[size+1 > len ? len-1 : size] = '\0';
 
-- 
2.20.1

[libnf_ct resend PATCH 2/8] Fix nfexp_snprintf return value docs

[Daniel Gröber]

The docs currently say "[...] Otherwise, 0 is returned."  which is just
completely wrong. Just like nfct_snprintf the expected buffer size is
returned.

Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
---
 src/expect/api.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/expect/api.c b/src/expect/api.c
index 33099d8..39cd092 100644
--- a/src/expect/api.c
+++ b/src/expect/api.c
@@ -795,8 +795,9 @@ int nfexp_catch(struct nfct_handle *h)
  * 	- NFEXP_O_LAYER: include layer 3 information in the output, this is
  * 			*only* required by NFEXP_O_DEFAULT.
  * 
- * On error, -1 is returned and errno is set appropiately. Otherwise,
- * 0 is returned.
+ * On error, -1 is returned and errno is set appropiately. Otherwise the
+ * size of what _would_ be written is returned, even if the size of the
+ * buffer is insufficient. This behaviour is similar to snprintf.
  */
 int nfexp_snprintf(char *buf,
 		  unsigned int size,
-- 
2.20.1

[libnf_ct resend PATCH 3/8] Replace strncpy with snprintf to improve null byte handling

[Daniel Gröber]

We currently use strncpy in a bunch of places which has this weird quirk
where it doesn't write a terminating null byte if the input string is >=
the max length. To mitigate this we write a null byte to the last character
manually.

While this works it is easy to forget. Instead we should just be using
snprintf which has more sensible behaviour as it always writes a null byte
even when truncating the string.

Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
---
 src/conntrack/copy.c      |  4 ++--
 src/conntrack/parse_mnl.c |  5 ++---
 src/conntrack/setter.c    |  3 +--
 src/expect/parse_mnl.c    | 16 ++++++++--------
 src/expect/setter.c       |  6 ++----
 5 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/src/conntrack/copy.c b/src/conntrack/copy.c
index eca202e..5a53df5 100644
--- a/src/conntrack/copy.c
+++ b/src/conntrack/copy.c
@@ -427,8 +427,8 @@ static void copy_attr_repl_off_aft(struct nf_conntrack *dest,
 static void copy_attr_helper_name(struct nf_conntrack *dest,
 				  const struct nf_conntrack *orig)
 {
-	strncpy(dest->helper_name, orig->helper_name, NFCT_HELPER_NAME_MAX);
-	dest->helper_name[NFCT_HELPER_NAME_MAX-1] = '\0';
+	snprintf(dest->helper_name, NFCT_HELPER_NAME_MAX, "%s",
+                 orig->helper_name);
 }
 
 static void copy_attr_zone(struct nf_conntrack *dest,
diff --git a/src/conntrack/parse_mnl.c b/src/conntrack/parse_mnl.c
index 515deff..3cbfc6a 100644
--- a/src/conntrack/parse_mnl.c
+++ b/src/conntrack/parse_mnl.c
@@ -690,9 +690,8 @@ nfct_parse_helper(const struct nlattr *attr, struct nf_conntrack *ct)
 	if (!tb[CTA_HELP_NAME])
 		return 0;
 
-	strncpy(ct->helper_name, mnl_attr_get_str(tb[CTA_HELP_NAME]),
-		NFCT_HELPER_NAME_MAX);
-	ct->helper_name[NFCT_HELPER_NAME_MAX-1] = '\0';
+	snprintf(ct->helper_name, NFCT_HELPER_NAME_MAX, "%s",
+		 mnl_attr_get_str(tb[CTA_HELP_NAME]));
 	set_bit(ATTR_HELPER_NAME, ct->head.set);
 
 	if (!tb[CTA_HELP_INFO])
diff --git a/src/conntrack/setter.c b/src/conntrack/setter.c
index 7b96936..0157de4 100644
--- a/src/conntrack/setter.c
+++ b/src/conntrack/setter.c
@@ -389,8 +389,7 @@ set_attr_repl_off_aft(struct nf_conntrack *ct, const void *value, size_t len)
 static void
 set_attr_helper_name(struct nf_conntrack *ct, const void *value, size_t len)
 {
-	strncpy(ct->helper_name, value, NFCT_HELPER_NAME_MAX);
-	ct->helper_name[NFCT_HELPER_NAME_MAX-1] = '\0';
+	snprintf(ct->helper_name, NFCT_HELPER_NAME_MAX, "%s", (char*)value);
 }
 
 static void
diff --git a/src/expect/parse_mnl.c b/src/expect/parse_mnl.c
index 091a8ae..1c94cad 100644
--- a/src/expect/parse_mnl.c
+++ b/src/expect/parse_mnl.c
@@ -10,6 +10,7 @@
  */
 
 #include "internal/internal.h"
+#include <assert.h>
 #include <libmnl/libmnl.h>
 
 static int nlmsg_parse_expection_attr_cb(const struct nlattr *attr, void *data)
@@ -139,11 +140,9 @@ int nfexp_nlmsg_parse(const struct nlmsghdr *nlh, struct nf_expect *exp)
 		set_bit(ATTR_EXP_FLAGS, exp->set);
 	}
 	if (tb[CTA_EXPECT_HELP_NAME]) {
-		strncpy(exp->helper_name,
-			mnl_attr_get_str(tb[CTA_EXPECT_HELP_NAME]),
-			NFCT_HELPER_NAME_MAX);
-		exp->helper_name[NFCT_HELPER_NAME_MAX - 1] = '\0';
-		set_bit(ATTR_EXP_HELPER_NAME, exp->set);
+		snprintf(exp->helper_name, NFCT_HELPER_NAME_MAX, "%s",
+			 mnl_attr_get_str(tb[CTA_EXPECT_HELP_NAME]));
+ 		set_bit(ATTR_EXP_HELPER_NAME, exp->set);
 	}
 	if (tb[CTA_EXPECT_CLASS]) {
 		exp->class = ntohl(mnl_attr_get_u32(tb[CTA_EXPECT_CLASS]));
@@ -153,9 +152,10 @@ int nfexp_nlmsg_parse(const struct nlmsghdr *nlh, struct nf_expect *exp)
 		nfexp_nlmsg_parse_nat(nfg, tb[CTA_EXPECT_NAT], exp);
 
 	if (tb[CTA_EXPECT_FN]) {
-		strncpy(exp->expectfn, mnl_attr_get_payload(tb[CTA_EXPECT_FN]),
-			__NFCT_EXPECTFN_MAX);
-		exp->expectfn[__NFCT_EXPECTFN_MAX - 1] = '\0';
+		int len = mnl_attr_get_payload_len(tb[CTA_EXPECT_FN]);
+		assert(len <= __NFCT_EXPECTFN_MAX); /* the kernel doesn't impose a max lenght on this str */
+		snprintf(exp->expectfn, __NFCT_EXPECTFN_MAX, "%s",
+			 (char*)mnl_attr_get_payload(tb[CTA_EXPECT_FN]));
 		set_bit(ATTR_EXP_FN, exp->set);
 	}
 
diff --git a/src/expect/setter.c b/src/expect/setter.c
index 18c925a..0a950c2 100644
--- a/src/expect/setter.c
+++ b/src/expect/setter.c
@@ -46,8 +46,7 @@ static void set_exp_attr_class(struct nf_expect *exp, const void *value)
 
 static void set_exp_attr_helper_name(struct nf_expect *exp, const void *value)
 {
-	strncpy(exp->helper_name, value, NFCT_HELPER_NAME_MAX);
-	exp->helper_name[NFCT_HELPER_NAME_MAX-1] = '\0';
+	snprintf(exp->helper_name, NFCT_HELPER_NAME_MAX, "%s", (char*)value);
 }
 
 static void set_exp_attr_nat_dir(struct nf_expect *exp, const void *value)
@@ -62,8 +61,7 @@ static void set_exp_attr_nat_tuple(struct nf_expect *exp, const void *value)
 
 static void set_exp_attr_expectfn(struct nf_expect *exp, const void *value)
 {
-	strncpy(exp->expectfn, value, __NFCT_EXPECTFN_MAX);
-	exp->expectfn[__NFCT_EXPECTFN_MAX-1] = '\0';
+	snprintf(exp->expectfn, __NFCT_EXPECTFN_MAX, "%s", (char*)value);
 }
 
 const set_exp_attr set_exp_attr_array[ATTR_EXP_MAX] = {
-- 
2.20.1

[libnf_ct resend PATCH 4/8] Fix incorrect snprintf size calculation

[Daniel Gröber]

The previous BUFFER_SIZE() call already updated the remaining 'len'. So
there is no need to subtract 'size' again. While this just makes the buffer
appear smaller than it is, which is mostly harmless, the subtraction might
underflow as 'size > len' is not checked like BUFFER_SIZE() does.

Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
---
 src/conntrack/snprintf_default.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conntrack/snprintf_default.c b/src/conntrack/snprintf_default.c
index 2f2f918..d00c5cb 100644
--- a/src/conntrack/snprintf_default.c
+++ b/src/conntrack/snprintf_default.c
@@ -108,7 +108,7 @@ static int __snprintf_address_ipv6(char *buf,
 	if (!inet_ntop(AF_INET6, &dst, tmp, sizeof(tmp)))
 		return -1;
 
-	ret = snprintf(buf+offset, len-size, "%s=%s ", dst_tag, tmp);
+	ret = snprintf(buf+offset, len, "%s=%s ", dst_tag, tmp);
 	BUFFER_SIZE(ret, size, len, offset);
 
 	return size;
-- 
2.20.1

[libnf_ct resend PATCH 5/8] Add asizeof() macro

[Daniel Gröber]

Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
---
 include/internal/internal.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/internal/internal.h b/include/internal/internal.h
index 859724b..68c5ef0 100644
--- a/include/internal/internal.h
+++ b/include/internal/internal.h
@@ -40,6 +40,8 @@
 #define IPPROTO_DCCP 33
 #endif
 
+#define asizeof(x) (sizeof(x) / sizeof(*x))
+
 #define BUFFER_SIZE(ret, size, len, offset)		\
 	if(ret < 0)					\
 		return -1;				\
-- 
2.20.1

[libnf_ct resend PATCH 6/8] Fix buffer overflow on invalid icmp type in setters

[Daniel Gröber]

When type is out of range for the invmap_icmp{,v6} array we leave rtype at
zero which will map to type=255 just like other error cases in this
function.

Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
---
 src/conntrack/grp_setter.c |  8 +++++---
 src/conntrack/setter.c     | 11 +++++++----
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/src/conntrack/grp_setter.c b/src/conntrack/grp_setter.c
index fccf578..dfbdc91 100644
--- a/src/conntrack/grp_setter.c
+++ b/src/conntrack/grp_setter.c
@@ -85,18 +85,20 @@ static void set_attr_grp_repl_port(struct nf_conntrack *ct, const void *value)
 
 static void set_attr_grp_icmp(struct nf_conntrack *ct, const void *value)
 {
-	uint8_t rtype;
+	uint8_t rtype = 0;
 	const struct nfct_attr_grp_icmp *this = value;
 
 	ct->head.orig.l4dst.icmp.type = this->type;
 
 	switch(ct->head.orig.l3protonum) {
 		case AF_INET:
-			rtype = invmap_icmp[this->type];
+			if(this->type < asizeof(invmap_icmp))
+				rtype = invmap_icmp[this->type];
 			break;
 
 		case AF_INET6:
-			rtype = invmap_icmpv6[this->type - 128];
+			if(this->type - 128 < asizeof(invmap_icmp))
+				rtype = invmap_icmpv6[this->type - 128];
 			break;
 
 		default:
diff --git a/src/conntrack/setter.c b/src/conntrack/setter.c
index 0157de4..ed65ba0 100644
--- a/src/conntrack/setter.c
+++ b/src/conntrack/setter.c
@@ -124,17 +124,20 @@ set_attr_repl_zone(struct nf_conntrack *ct, const void *value, size_t len)
 static void
 set_attr_icmp_type(struct nf_conntrack *ct, const void *value, size_t len)
 {
-	uint8_t rtype;
+        uint8_t type = *((uint8_t *) value);
+	uint8_t rtype = 0;
 
-	ct->head.orig.l4dst.icmp.type = *((uint8_t *) value);
+	ct->head.orig.l4dst.icmp.type = type;
 
 	switch(ct->head.orig.l3protonum) {
 		case AF_INET:
-			rtype = invmap_icmp[*((uint8_t *) value)];
+                        if(type < asizeof(invmap_icmp))
+                                rtype = invmap_icmp[type];
 			break;
 
 		case AF_INET6:
-			rtype = invmap_icmpv6[*((uint8_t *) value) - 128];
+                        if(type - 128 < asizeof(invmap_icmpv6))
+                                rtype = invmap_icmpv6[type - 128];
 			break;
 
 		default:
-- 
2.20.1

[libnf_ct resend PATCH 7/8] Fix buffer overflow in protocol related snprintf functions

[Daniel Gröber]

Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
---
 src/conntrack/snprintf_default.c | 12 ++++--------
 src/conntrack/snprintf_xml.c     | 20 ++++++++++++++++++--
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/src/conntrack/snprintf_default.c b/src/conntrack/snprintf_default.c
index d00c5cb..8e3d41c 100644
--- a/src/conntrack/snprintf_default.c
+++ b/src/conntrack/snprintf_default.c
@@ -13,20 +13,16 @@ static int __snprintf_l3protocol(char *buf,
 				 unsigned int len,
 				 const struct nf_conntrack *ct)
 {
-	return (snprintf(buf, len, "%-8s %u ", 
-		l3proto2str[ct->head.orig.l3protonum] == NULL ?
-		"unknown" : l3proto2str[ct->head.orig.l3protonum], 
-		 ct->head.orig.l3protonum));
+        uint8_t num = ct->head.orig.l3protonum;
+	return snprintf(buf, len, "%-8s %u ", __l3proto2str(num), num);
 }
 
 int __snprintf_protocol(char *buf,
 			unsigned int len,
 			const struct nf_conntrack *ct)
 {
-	return (snprintf(buf, len, "%-8s %u ", 
-		proto2str[ct->head.orig.protonum] == NULL ?
-		"unknown" : proto2str[ct->head.orig.protonum], 
-		 ct->head.orig.protonum));
+        uint8_t num = ct->head.orig.protonum;
+	return snprintf(buf, len, "%-8s %u ", __proto2str(num), num);
 }
 
 static int __snprintf_timeout(char *buf,
diff --git a/src/conntrack/snprintf_xml.c b/src/conntrack/snprintf_xml.c
index c3a836a..6a9dfb4 100644
--- a/src/conntrack/snprintf_xml.c
+++ b/src/conntrack/snprintf_xml.c
@@ -55,12 +55,28 @@
 
 const char *__proto2str(uint8_t protonum)
 {
-	return proto2str[protonum] ? proto2str[protonum] : "unknown";
+        const char *str = NULL;
+
+        if(protonum < asizeof(proto2str))
+                str = proto2str[protonum];
+
+        if(str == NULL)
+                str = "unknown";
+
+	return str;
 }
 
 const char *__l3proto2str(uint8_t protonum)
 {
-	return l3proto2str[protonum] ? l3proto2str[protonum] : "unknown";
+        const char *str = NULL;
+
+        if(protonum < asizeof(l3proto2str))
+                str = l3proto2str[protonum];
+
+        if(str == NULL)
+                str = "unknown";
+
+	return str;
 }
 
 static int __snprintf_ipv4_xml(char *buf,
-- 
2.20.1

[libnf_ct resend PATCH 8/8] Fix buffer overflows in __snprintf_protoinfo* like in *2str fns

[Daniel Gröber]

Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
---
 src/conntrack/snprintf_default.c | 42 +++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/src/conntrack/snprintf_default.c b/src/conntrack/snprintf_default.c
index 8e3d41c..89eee8f 100644
--- a/src/conntrack/snprintf_default.c
+++ b/src/conntrack/snprintf_default.c
@@ -36,30 +36,48 @@ static int __snprintf_protoinfo(char *buf,
 				unsigned int len,
 				const struct nf_conntrack *ct)
 {
-	return snprintf(buf, len, "%s ",
-			ct->protoinfo.tcp.state < TCP_CONNTRACK_MAX ?
-			states[ct->protoinfo.tcp.state] :
-			states[TCP_CONNTRACK_NONE]);
+        const char *str = NULL;
+        uint8_t state = ct->protoinfo.tcp.state;
+
+        if(state < asizeof(states))
+                str = states[state];
+
+        if(str == NULL)
+                str = states[TCP_CONNTRACK_NONE];
+
+	return snprintf(buf, len, "%s ", str);
 }
 
 static int __snprintf_protoinfo_sctp(char *buf,
 				     unsigned int len,
 				     const struct nf_conntrack *ct)
 {
-	return snprintf(buf, len, "%s ",
-			ct->protoinfo.sctp.state < SCTP_CONNTRACK_MAX ?
-			sctp_states[ct->protoinfo.sctp.state] :
-			sctp_states[SCTP_CONNTRACK_NONE]);
+        const char *str = NULL;
+        uint8_t state = ct->protoinfo.sctp.state;
+
+        if(state < asizeof(sctp_states))
+                str = sctp_states[state];
+
+        if(str == NULL)
+                str = sctp_states[SCTP_CONNTRACK_NONE];
+
+	return snprintf(buf, len, "%s ", str);
 }
 
 static int __snprintf_protoinfo_dccp(char *buf,
 				     unsigned int len,
 				     const struct nf_conntrack *ct)
 {
-	return snprintf(buf, len, "%s ",
-			ct->protoinfo.dccp.state < DCCP_CONNTRACK_MAX ?
-			sctp_states[ct->protoinfo.dccp.state] :
-			sctp_states[DCCP_CONNTRACK_NONE]);
+        const char *str = NULL;
+        uint8_t state = ct->protoinfo.dccp.state;
+
+        if(state < asizeof(dccp_states))
+                str = dccp_states[state];
+
+        if(str == NULL)
+                str = dccp_states[SCTP_CONNTRACK_NONE];
+
+	return snprintf(buf, len, "%s ", str);
 }
 
 static int __snprintf_address_ipv4(char *buf,
-- 
2.20.1

Re: [libnf_ct resend PATCH 1/8] Handle negative snprintf return values properly

[Pablo Neira Ayuso]

On Tue, Jun 23, 2020 at 02:33:56PM +0200, Daniel Gröber wrote:
> Currently the BUFFER_SIZE macro doesn't take negative 'ret' values into
> account. A negative return should just be passed through to the caller,
> snprintf will already have set 'errno' properly.
> 
> Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
> ---
>  include/internal/internal.h | 2 ++
>  src/conntrack/api.c         | 6 +++---
>  src/conntrack/snprintf.c    | 3 +++
>  src/expect/snprintf.c       | 3 +++
>  4 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/include/internal/internal.h b/include/internal/internal.h
> index bb44e12..859724b 100644
> --- a/include/internal/internal.h
> +++ b/include/internal/internal.h
> @@ -41,6 +41,8 @@
>  #endif
>  
>  #define BUFFER_SIZE(ret, size, len, offset)		\
> +	if(ret < 0)					\

        if (ret < 0)
          ^
missing space

> +		return -1;				\
>  	size += ret;					\
>  	if (ret > len)					\
>  		ret = len;				\
> diff --git a/src/conntrack/api.c b/src/conntrack/api.c
> index ffa5216..78d7d61 100644
> --- a/src/conntrack/api.c
> +++ b/src/conntrack/api.c
> @@ -1099,9 +1099,9 @@ int nfct_catch(struct nfct_handle *h)
>   * print the message just after you receive the destroy event. If you want
>   * more accurate timestamping, use NFCT_OF_TIMESTAMP.
>   *
> - * This function returns the size of the information that _would_ have been 
> - * written to the buffer, even if there was no room for it. Thus, the
> - * behaviour is similar to snprintf.
> + * On error, -1 is returned and errno is set appropiately. Otherwise the
> + * size of what _would_ be written is returned, even if the size of the
> + * buffer is insufficient. This behaviour is similar to snprintf.
>   */
>  int nfct_snprintf(char *buf,
>  		  unsigned int size,
> diff --git a/src/conntrack/snprintf.c b/src/conntrack/snprintf.c
> index 17ad885..a87c0c9 100644
> --- a/src/conntrack/snprintf.c
> +++ b/src/conntrack/snprintf.c
> @@ -85,6 +85,9 @@ int __snprintf_conntrack(char *buf,
>  		return -1;
>  	}
>  
> +        if(size < 0)
   ^^^^^^^^
I can see spaces here as indentations.

> +                return size;
> +
>  	/* NULL terminated string */
>  	buf[size+1 > len ? len-1 : size] = '\0';
>  
> diff --git a/src/expect/snprintf.c b/src/expect/snprintf.c
> index 3a104b5..b28ac62 100644
> --- a/src/expect/snprintf.c
> +++ b/src/expect/snprintf.c
> @@ -30,6 +30,9 @@ int __snprintf_expect(char *buf,
>  		return -1;
>  	}
>  
> +	if(size < 0)
> +		return size;
> +
>  	/* NULL terminated string */
>  	buf[size+1 > len ? len-1 : size] = '\0';
>  
> -- 
> 2.20.1
> 

Re: [libnf_ct resend PATCH 5/8] Add asizeof() macro

[Pablo Neira Ayuso]

On Tue, Jun 23, 2020 at 02:34:00PM +0200, Daniel Gröber wrote:
> Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
> ---
>  include/internal/internal.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/internal/internal.h b/include/internal/internal.h
> index 859724b..68c5ef0 100644
> --- a/include/internal/internal.h
> +++ b/include/internal/internal.h
> @@ -40,6 +40,8 @@
>  #define IPPROTO_DCCP 33
>  #endif
>  
> +#define asizeof(x) (sizeof(x) / sizeof(*x))

Please call this ARRAY_SIZE.

Thanks.

Re: [libnf_ct resend PATCH 6/8] Fix buffer overflow on invalid icmp type in setters

[Pablo Neira Ayuso]

On Tue, Jun 23, 2020 at 02:34:01PM +0200, Daniel Gröber wrote:
> When type is out of range for the invmap_icmp{,v6} array we leave rtype at
> zero which will map to type=255 just like other error cases in this
> function.
> 
> Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
> ---
>  src/conntrack/grp_setter.c |  8 +++++---
>  src/conntrack/setter.c     | 11 +++++++----
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/src/conntrack/grp_setter.c b/src/conntrack/grp_setter.c
> index fccf578..dfbdc91 100644
> --- a/src/conntrack/grp_setter.c
> +++ b/src/conntrack/grp_setter.c
> @@ -85,18 +85,20 @@ static void set_attr_grp_repl_port(struct nf_conntrack *ct, const void *value)
>  
>  static void set_attr_grp_icmp(struct nf_conntrack *ct, const void *value)
>  {
> -	uint8_t rtype;
> +	uint8_t rtype = 0;
>  	const struct nfct_attr_grp_icmp *this = value;
>  
>  	ct->head.orig.l4dst.icmp.type = this->type;
>  
>  	switch(ct->head.orig.l3protonum) {
>  		case AF_INET:
> -			rtype = invmap_icmp[this->type];
> +			if(this->type < asizeof(invmap_icmp))
> +				rtype = invmap_icmp[this->type];

Probably add a small helper function for this?

static inline uint8_t icmp_invmap(uint8_t type)
{
        assert(this->type < ARRAY_SIZE(invmap_icmp));

        return invmap_icmp[this->type];
}

> diff --git a/src/conntrack/setter.c b/src/conntrack/setter.c
> index 0157de4..ed65ba0 100644
> --- a/src/conntrack/setter.c
> +++ b/src/conntrack/setter.c
> @@ -124,17 +124,20 @@ set_attr_repl_zone(struct nf_conntrack *ct, const void *value, size_t len)
>  static void
>  set_attr_icmp_type(struct nf_conntrack *ct, const void *value, size_t len)
>  {
> -	uint8_t rtype;
> +        uint8_t type = *((uint8_t *) value);
> +	uint8_t rtype = 0;
>  
> -	ct->head.orig.l4dst.icmp.type = *((uint8_t *) value);
> +	ct->head.orig.l4dst.icmp.type = type;
>  
>  	switch(ct->head.orig.l3protonum) {
>  		case AF_INET:
> -			rtype = invmap_icmp[*((uint8_t *) value)];
> +                        if(type < asizeof(invmap_icmp))
> +                                rtype = invmap_icmp[type];

Make sure indentation tab is 8-char instead of spaces.

Thanks.