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

[libnf_ct 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 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

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.