[libnf_ct PATCH v2 1/9] Handle negative snprintf return values properly

[libnf_ct PATCH v2 1/9] 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..b1fc670 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..eb26af4 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..8c2f3e4 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 PATCH v2 2/9] 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 PATCH v2 3/9] 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    | 15 ++++++++-------
 src/expect/setter.c       |  6 ++----
 5 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/src/conntrack/copy.c b/src/conntrack/copy.c
index eca202e..402f994 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..3a293b0 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..fb4bdb7 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,10 +140,8 @@ 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';
+		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]) {
@@ -153,9 +152,11 @@ 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]);
+		/* the kernel doesn't impose a max length on this str */
+		assert(len <= __NFCT_EXPECTFN_MAX);
+		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..c2ca412 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 PATCH v2 4/9] 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 PATCH v2 5/9] Add ARRAY_SIZE() 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 b1fc670..0f59f1a 100644
--- a/include/internal/internal.h
+++ b/include/internal/internal.h
@@ -40,6 +40,8 @@
 #define IPPROTO_DCCP 33
 #endif
 
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
+
 #define BUFFER_SIZE(ret, size, len, offset)		\
 	if (ret < 0)					\
 		return -1;				\
-- 
2.20.1

[libnf_ct PATCH v2 6/9] 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..49dc033 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 < ARRAY_SIZE(invmap_icmp))
+				rtype = invmap_icmp[this->type];
 			break;
 
 		case AF_INET6:
-			rtype = invmap_icmpv6[this->type - 128];
+			if (this->type - 128 < ARRAY_SIZE(invmap_icmp))
+				rtype = invmap_icmpv6[this->type - 128];
 			break;
 
 		default:
diff --git a/src/conntrack/setter.c b/src/conntrack/setter.c
index 3a293b0..1d3b971 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 < ARRAY_SIZE(invmap_icmp))
+				rtype = invmap_icmp[type];
 			break;
 
 		case AF_INET6:
-			rtype = invmap_icmpv6[*((uint8_t *) value) - 128];
+			if (type - 128 < ARRAY_SIZE(invmap_icmpv6))
+				rtype = invmap_icmpv6[type - 128];
 			break;
 
 		default:
-- 
2.20.1

[libnf_ct PATCH v2 7/9] Move icmp request>reply type mapping to common file

[Daniel Gröber]

Currently the invmap_icmp* arrays are duplicated in setter.c and
grp_setter.c. This moves them to a new module 'proto'.

Instead of having the code access the arrays directly we provide new
wrapper functions __icmp{,v6}_reply_type.

Signed-off-by: Daniel Gröber <dxld@darkboxed.org>
---
 include/internal/internal.h |  1 +
 include/internal/proto.h    | 19 +++++++++++++++++++
 src/conntrack/Makefile.am   |  3 ++-
 src/conntrack/grp_setter.c  | 34 ++--------------------------------
 src/conntrack/proto.c       | 36 ++++++++++++++++++++++++++++++++++++
 src/conntrack/setter.c      | 34 ++--------------------------------
 6 files changed, 62 insertions(+), 65 deletions(-)
 create mode 100644 include/internal/proto.h
 create mode 100644 src/conntrack/proto.c

diff --git a/include/internal/internal.h b/include/internal/internal.h
index 0f59f1a..2ef8a90 100644
--- a/include/internal/internal.h
+++ b/include/internal/internal.h
@@ -27,6 +27,7 @@
 #include "internal/types.h"
 #include "internal/extern.h"
 #include "internal/bitops.h"
+#include "internal/proto.h"
 
 #ifndef IPPROTO_SCTP
 #define IPPROTO_SCTP 132
diff --git a/include/internal/proto.h b/include/internal/proto.h
new file mode 100644
index 0000000..40e7bfe
--- /dev/null
+++ b/include/internal/proto.h
@@ -0,0 +1,19 @@
+#ifndef _NFCT_PROTO_H_
+#define _NFCT_PROTO_H_
+
+#include <stdint.h>
+#include <linux/icmp.h>
+#include <linux/icmpv6.h>
+
+#ifndef ICMPV6_NI_QUERY
+#define ICMPV6_NI_QUERY 139
+#endif
+
+#ifndef ICMPV6_NI_REPLY
+#define ICMPV6_NI_REPLY 140
+#endif
+
+uint8_t __icmp_reply_type(uint8_t type);
+uint8_t __icmpv6_reply_type(uint8_t type);
+
+#endif
diff --git a/src/conntrack/Makefile.am b/src/conntrack/Makefile.am
index 602ed33..1fbf176 100644
--- a/src/conntrack/Makefile.am
+++ b/src/conntrack/Makefile.am
@@ -14,4 +14,5 @@ libnfconntrack_la_SOURCES = api.c \
 			    copy.c \
 			    filter.c bsf.c filter_dump.c \
 			    grp.c grp_getter.c grp_setter.c \
-			    stack.c
+			    stack.c \
+			    proto.c
diff --git a/src/conntrack/grp_setter.c b/src/conntrack/grp_setter.c
index 49dc033..82a6139 100644
--- a/src/conntrack/grp_setter.c
+++ b/src/conntrack/grp_setter.c
@@ -8,34 +8,6 @@
  */
 
 #include "internal/internal.h"
-#include <linux/icmp.h>
-#include <linux/icmpv6.h>
-
-static const uint8_t invmap_icmp[] = {
-	[ICMP_ECHO]		= ICMP_ECHOREPLY + 1,
-	[ICMP_ECHOREPLY]	= ICMP_ECHO + 1,
-	[ICMP_TIMESTAMP]	= ICMP_TIMESTAMPREPLY + 1,
-	[ICMP_TIMESTAMPREPLY]	= ICMP_TIMESTAMP + 1,
-	[ICMP_INFO_REQUEST]	= ICMP_INFO_REPLY + 1,
-	[ICMP_INFO_REPLY]	= ICMP_INFO_REQUEST + 1,
-	[ICMP_ADDRESS]		= ICMP_ADDRESSREPLY + 1,
-	[ICMP_ADDRESSREPLY]	= ICMP_ADDRESS + 1
-};
-
-#ifndef ICMPV6_NI_QUERY
-#define ICMPV6_NI_QUERY 139
-#endif
-
-#ifndef ICMPV6_NI_REPLY
-#define ICMPV6_NI_REPLY 140
-#endif
-
-static const uint8_t invmap_icmpv6[] = {
-	[ICMPV6_ECHO_REQUEST - 128]	= ICMPV6_ECHO_REPLY + 1,
-	[ICMPV6_ECHO_REPLY - 128]	= ICMPV6_ECHO_REQUEST + 1,
-	[ICMPV6_NI_QUERY - 128]		= ICMPV6_NI_QUERY + 1,
-	[ICMPV6_NI_REPLY - 128]		= ICMPV6_NI_REPLY + 1
-};
 
 static void set_attr_grp_orig_ipv4(struct nf_conntrack *ct, const void *value)
 {
@@ -92,13 +64,11 @@ static void set_attr_grp_icmp(struct nf_conntrack *ct, const void *value)
 
 	switch(ct->head.orig.l3protonum) {
 		case AF_INET:
-			if (this->type < ARRAY_SIZE(invmap_icmp))
-				rtype = invmap_icmp[this->type];
+			rtype = __icmp_reply_type(this->type);
 			break;
 
 		case AF_INET6:
-			if (this->type - 128 < ARRAY_SIZE(invmap_icmp))
-				rtype = invmap_icmpv6[this->type - 128];
+			rtype = __icmpv6_reply_type(this->type);
 			break;
 
 		default:
diff --git a/src/conntrack/proto.c b/src/conntrack/proto.c
new file mode 100644
index 0000000..ba79b9b
--- /dev/null
+++ b/src/conntrack/proto.c
@@ -0,0 +1,36 @@
+#include <internal/proto.h>
+#include <internal/internal.h>
+
+static const uint8_t invmap_icmp[] = {
+	[ICMP_ECHO]		= ICMP_ECHOREPLY + 1,
+	[ICMP_ECHOREPLY]	= ICMP_ECHO + 1,
+	[ICMP_TIMESTAMP]	= ICMP_TIMESTAMPREPLY + 1,
+	[ICMP_TIMESTAMPREPLY]	= ICMP_TIMESTAMP + 1,
+	[ICMP_INFO_REQUEST]	= ICMP_INFO_REPLY + 1,
+	[ICMP_INFO_REPLY]	= ICMP_INFO_REQUEST + 1,
+	[ICMP_ADDRESS]		= ICMP_ADDRESSREPLY + 1,
+	[ICMP_ADDRESSREPLY]	= ICMP_ADDRESS + 1
+};
+
+static const uint8_t invmap_icmpv6[] = {
+	[ICMPV6_ECHO_REQUEST - 128]	= ICMPV6_ECHO_REPLY + 1,
+	[ICMPV6_ECHO_REPLY - 128]	= ICMPV6_ECHO_REQUEST + 1,
+	[ICMPV6_NI_QUERY - 128]		= ICMPV6_NI_QUERY + 1,
+	[ICMPV6_NI_REPLY - 128]		= ICMPV6_NI_REPLY + 1
+};
+
+uint8_t __icmp_reply_type(uint8_t type)
+{
+	if (type < ARRAY_SIZE(invmap_icmp))
+		return invmap_icmp[type];
+
+	return 0;
+}
+
+uint8_t __icmpv6_reply_type(uint8_t type)
+{
+	if (type - 128 < ARRAY_SIZE(invmap_icmpv6))
+		return invmap_icmpv6[type - 128];
+
+	return 0;
+}
diff --git a/src/conntrack/setter.c b/src/conntrack/setter.c
index 1d3b971..cee81f1 100644
--- a/src/conntrack/setter.c
+++ b/src/conntrack/setter.c
@@ -8,34 +8,6 @@
  */
 
 #include "internal/internal.h"
-#include <linux/icmp.h>
-#include <linux/icmpv6.h>
-
-static const uint8_t invmap_icmp[] = {
-	[ICMP_ECHO]		= ICMP_ECHOREPLY + 1,
-	[ICMP_ECHOREPLY]	= ICMP_ECHO + 1,
-	[ICMP_TIMESTAMP]	= ICMP_TIMESTAMPREPLY + 1,
-	[ICMP_TIMESTAMPREPLY]	= ICMP_TIMESTAMP + 1,
-	[ICMP_INFO_REQUEST]	= ICMP_INFO_REPLY + 1,
-	[ICMP_INFO_REPLY]	= ICMP_INFO_REQUEST + 1,
-	[ICMP_ADDRESS]		= ICMP_ADDRESSREPLY + 1,
-	[ICMP_ADDRESSREPLY]	= ICMP_ADDRESS + 1
-};
-
-#ifndef ICMPV6_NI_QUERY
-#define ICMPV6_NI_QUERY 139
-#endif
-
-#ifndef ICMPV6_NI_REPLY
-#define ICMPV6_NI_REPLY 140
-#endif
-
-static const uint8_t invmap_icmpv6[] = {
-	[ICMPV6_ECHO_REQUEST - 128]	= ICMPV6_ECHO_REPLY + 1,
-	[ICMPV6_ECHO_REPLY - 128]	= ICMPV6_ECHO_REQUEST + 1,
-	[ICMPV6_NI_QUERY - 128]		= ICMPV6_NI_QUERY + 1,
-	[ICMPV6_NI_REPLY - 128]		= ICMPV6_NI_REPLY + 1
-};
 
 static void
 set_attr_orig_ipv4_src(struct nf_conntrack *ct, const void *value, size_t len)
@@ -131,13 +103,11 @@ set_attr_icmp_type(struct nf_conntrack *ct, const void *value, size_t len)
 
 	switch(ct->head.orig.l3protonum) {
 		case AF_INET:
-			if (type < ARRAY_SIZE(invmap_icmp))
-				rtype = invmap_icmp[type];
+			rtype = __icmp_reply_type(type);
 			break;
 
 		case AF_INET6:
-			if (type - 128 < ARRAY_SIZE(invmap_icmpv6))
-				rtype = invmap_icmpv6[type - 128];
+			rtype = __icmpv6_reply_type(type);
 			break;
 
 		default:
-- 
2.20.1

[libnf_ct PATCH v2 8/9] 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 | 14 ++++++--------
 src/conntrack/snprintf_xml.c     | 20 ++++++++++++++++++--
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/src/conntrack/snprintf_default.c b/src/conntrack/snprintf_default.c
index d00c5cb..d18d2f2 100644
--- a/src/conntrack/snprintf_default.c
+++ b/src/conntrack/snprintf_default.c
@@ -13,20 +13,18 @@ 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..e557df2 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 < ARRAY_SIZE(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 < ARRAY_SIZE(l3proto2str))
+		str = l3proto2str[protonum];
+
+	if (str == NULL)
+		str = "unknown";
+
+	return str;
 }
 
 static int __snprintf_ipv4_xml(char *buf,
-- 
2.20.1

[libnf_ct PATCH v2 9/9] 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 | 54 +++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/src/conntrack/snprintf_default.c b/src/conntrack/snprintf_default.c
index d18d2f2..467f4a8 100644
--- a/src/conntrack/snprintf_default.c
+++ b/src/conntrack/snprintf_default.c
@@ -15,6 +15,9 @@ static int __snprintf_l3protocol(char *buf,
 {
 	uint8_t num = ct->head.orig.l3protonum;
 
+	if (!test_bit(ATTR_ORIG_L3PROTO, ct->head.set))
+		return -1;
+
 	return snprintf(buf, len, "%-8s %u ", __l3proto2str(num), num);
 }
 
@@ -24,6 +27,9 @@ int __snprintf_protocol(char *buf,
 {
 	uint8_t num = ct->head.orig.protonum;
 
+	if (!test_bit(ATTR_ORIG_L4PROTO, ct->head.set))
+		return -1;
+
 	return snprintf(buf, len, "%-8s %u ", __proto2str(num), num);
 }
 
@@ -38,30 +44,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 < ARRAY_SIZE(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 < ARRAY_SIZE(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 < ARRAY_SIZE(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,
@@ -134,7 +158,7 @@ int __snprintf_address(char *buf,
 	return size;
 }
 
-int __snprintf_proto(char *buf, 
+int __snprintf_proto(char *buf,
 		     unsigned int len,
 		     const struct __nfct_tuple *tuple)
 {
@@ -197,7 +221,7 @@ static int __snprintf_status_not_seen_reply(char *buf,
 					    const struct nf_conntrack *ct)
 {
 	int size = 0;
-	
+
         if (!(ct->status & IPS_SEEN_REPLY))
                 size = snprintf(buf, len, "[UNREPLIED] ");
 
@@ -345,7 +369,7 @@ __snprintf_clabels(char *buf, unsigned int len,
 	return size;
 }
 
-int __snprintf_conntrack_default(char *buf, 
+int __snprintf_conntrack_default(char *buf,
 				 unsigned int len,
 				 const struct nf_conntrack *ct,
 				 unsigned int msg_type,
-- 
2.20.1

Re: [libnf_ct PATCH v2 1/9] Handle negative snprintf return values properly

[Pablo Neira Ayuso]

On Wed, Jun 24, 2020 at 03:29:57PM +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.

Series applied, thanks.

> diff --git a/include/internal/internal.h b/include/internal/internal.h
> index bb44e12..b1fc670 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;				\

Side note: I don't like this hidden branch under a macro. But
snprintf() == -1 is unlikely to happen and I don't have a better idea
to deal with this case ATM.

Re: [libnf_ct PATCH v2 1/9] Handle negative snprintf return values properly

[Daniel Gröber]

On Wed, Jul 01, 2020 at 01:09:51PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Jun 24, 2020 at 03:29:57PM +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.
> 
> Series applied, thanks.

Great, thanks!

> > diff --git a/include/internal/internal.h b/include/internal/internal.h
> > index bb44e12..b1fc670 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;				\
> 
> Side note: I don't like this hidden branch under a macro. But
> snprintf() == -1 is unlikely to happen and I don't have a better idea
> to deal with this case ATM.

Since there is already another branch in this macro I take it the issue is
the part where we return now, right?

I can see how this is kind of nasty being completely implicit and all, how
about using a label and passing the name to the macro? Something like:

    ret = snprintf(...)
    BUFFER_SIZE_ERR(ret, size, len, offset, err);
    [...]
    err:
        return -1;

--Daniel