[iproute PATCH v2 0/7] Covscan: Fixes for string termination

This series collects patches from v1 dealing with code potentially
leaving string buffers unterminated. This does not include situations
where it happens for parsed interface names since an overall solution
was attempted for that and it's state is still unclear due to lack of
feedback from upstream.

No changes to the actual patches, just splitting into smaller series.

Phil Sutter (7):
  ipntable: Make sure filter.name is NULL-terminated
  xfrm_state: Make sure alg_name is NULL-terminated
  lib/fs: Fix format string in find_fs_mount()
  lib/inet_proto: Make sure destination buffers are NULL-terminated
  lnstat_util: Simplify alloc_and_open() a bit
  tc/m_xt: Fix for potential string buffer overflows
  lib/ll_map: Make sure im->name is NULL-terminated

 ip/ipntable.c      | 3 ++-
 ip/xfrm_state.c    | 3 ++-
 lib/fs.c           | 2 +-
 lib/inet_proto.c   | 9 ++++++---
 lib/ll_map.c       | 4 ++--
 misc/lnstat_util.c | 7 ++-----
 tc/m_xt.c          | 7 ++++---
 7 files changed, 19 insertions(+), 16 deletions(-)

-- 
2.13.1

- Use strncpy() when writing to target->t->u.user.name and make sure the
  final byte remains untouched (xtables_calloc() set it to zero).
- 'tname' length sanitization was completely wrong: If it's length
  exceeded the 16 bytes available in 'k', passing a length value of 16
  to strncpy() would overwrite the previously NULL'ed 'k[15]'. Also, the
  sanitization has to happen if 'tname' is exactly 16 bytes long as
  well.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/m_xt.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tc/m_xt.c b/tc/m_xt.c
index ad52d239caf61..9218b14594403 100644
--- a/tc/m_xt.c
+++ b/tc/m_xt.c

@@ -95,7 +95,8 @@ build_st(struct xtables_target *target, struct xt_entry_target *t)
 	if (t == NULL) {
 		target->t = xtables_calloc(1, size);
 		target->t->u.target_size = size;
-		strcpy(target->t->u.user.name, target->name);
+		strncpy(target->t->u.user.name, target->name,
+			sizeof(target->t->u.user.name) - 1);
 		target->t->u.user.revision = target->revision;
 
 		if (target->init != NULL)

@@ -277,8 +278,8 @@ static int parse_ipt(struct action_util *a, int *argc_p,
 	}
 	fprintf(stdout, " index %d\n", index);
 
-	if (strlen(tname) > 16) {
-		size = 16;
+	if (strlen(tname) >= 16) {
+		size = 15;
 		k[15] = 0;
 	} else {
 		size = 1 + strlen(tname);

-- 
2.13.1

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

diff --git a/ip/ipntable.c b/ip/ipntable.c
index 879626ee4f491..7be1f04d33d90 100644
--- a/ip/ipntable.c
+++ b/ip/ipntable.c

@@ -633,7 +633,8 @@ static int ipntable_show(int argc, char **argv)
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();
 
-			strncpy(filter.name, *argv, sizeof(filter.name));
+			strncpy(filter.name, *argv, sizeof(filter.name) - 1);
+			filter.name[sizeof(filter.name) - 1] = '\0';
 		} else
 			invarg("unknown", *argv);
 

-- 
2.13.1

A field width of 4096 allows fscanf() to store that amount of characters
into the given buffer, though that doesn't include the terminating NULL
byte. Decrease the value by one to leave space for it.

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

diff --git a/lib/fs.c b/lib/fs.c
index c59ac564581d0..1ff881ecfcd8c 100644
--- a/lib/fs.c
+++ b/lib/fs.c

@@ -45,7 +45,7 @@ static char *find_fs_mount(const char *fs_to_find)
 		return NULL;
 	}
 
-	while (fscanf(fp, "%*s %4096s %127s %*s %*d %*d\n",
+	while (fscanf(fp, "%*s %4095s %127s %*s %*d %*d\n",
 		      path, fstype) == 2) {
 		if (strcmp(fstype, fs_to_find) == 0) {
 			mnt = strdup(path);

-- 
2.13.1

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

diff --git a/lib/ll_map.c b/lib/ll_map.c
index 4e4556c9ac80b..4d06eb69f138a 100644
--- a/lib/ll_map.c
+++ b/lib/ll_map.c

@@ -120,11 +120,11 @@ int ll_remember_index(const struct sockaddr_nl *who,
 		return 0;
 	}
 
-	im = malloc(sizeof(*im));
+	im = calloc(1, sizeof(*im));
 	if (im == NULL)
 		return 0;
 	im->index = ifi->ifi_index;
-	strcpy(im->name, ifname);
+	strncpy(im->name, ifname, IFNAMSIZ - 1);
 	im->type = ifi->ifi_type;
 	im->flags = ifi->ifi_flags;
 

-- 
2.13.1

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/inet_proto.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/inet_proto.c b/lib/inet_proto.c
index ceda082b12a2e..87ed4769fc3da 100644
--- a/lib/inet_proto.c
+++ b/lib/inet_proto.c

@@ -35,8 +35,10 @@ const char *inet_proto_n2a(int proto, char *buf, int len)
 	pe = getprotobynumber(proto);
 	if (pe) {
 		icache = proto;
-		strncpy(ncache, pe->p_name, 16);
-		strncpy(buf, pe->p_name, len);
+		strncpy(ncache, pe->p_name, 15);
+		ncache[15] = '\0';
+		strncpy(buf, pe->p_name, len - 1);
+		buf[len] = '\0';
 		return buf;
 	}
 	snprintf(buf, len, "ipproto-%d", proto);

@@ -62,7 +64,8 @@ int inet_proto_a2n(const char *buf)
 	pe = getprotobyname(buf);
 	if (pe) {
 		icache = pe->p_proto;
-		strncpy(ncache, pe->p_name, 16);
+		strncpy(ncache, pe->p_name, 15);
+		ncache[15] = '\0';
 		return pe->p_proto;
 	}
 	return -1;

-- 
2.13.1

Relying upon callers and using unsafe strcpy() is probably not the best
idea. Aside from that, using snprintf() allows to format the string for
lf->path in one go.

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

diff --git a/misc/lnstat_util.c b/misc/lnstat_util.c
index cc54598fe1bef..ec19238c24b94 100644
--- a/misc/lnstat_util.c
+++ b/misc/lnstat_util.c

@@ -180,11 +180,8 @@ static struct lnstat_file *alloc_and_open(const char *path, const char *file)
 	}
 
 	/* initialize */
-	/* de->d_name is guaranteed to be <= NAME_MAX */
-	strcpy(lf->basename, file);
-	strcpy(lf->path, path);
-	strcat(lf->path, "/");
-	strcat(lf->path, lf->basename);
+	snprintf(lf->basename, sizeof(lf->basename), "%s", file);
+	snprintf(lf->path, sizeof(lf->path), "%s/%s", path, file);
 
 	/* initialize to default */
 	lf->interval.tv_sec = 1;

-- 
2.13.1

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

diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c
index e11c93bf1c3b5..7c0389038986e 100644
--- a/ip/xfrm_state.c
+++ b/ip/xfrm_state.c

@@ -125,7 +125,8 @@ static int xfrm_algo_parse(struct xfrm_algo *alg, enum xfrm_attr_type_t type,
 	fprintf(stderr, "warning: ALGO-NAME/ALGO-KEYMAT values will be sent to the kernel promiscuously! (verifying them isn't implemented yet)\n");
 #endif
 
-	strncpy(alg->alg_name, name, sizeof(alg->alg_name));
+	strncpy(alg->alg_name, name, sizeof(alg->alg_name) - 1);
+	alg->alg_name[sizeof(alg->alg_name) - 1] = '\0';
 
 	if (slen > 2 && strncmp(key, "0x", 2) == 0) {
 		/* split two chars "0x" from the top */

-- 
2.13.1

From: Phil Sutter

Why not check for overflow instead?
			if (filter.name[sizeof(filter.name) - 1])
				usage("filer name too long");

	David

Hi David,

On Fri, Aug 18, 2017 at 09:19:16AM +0000, David Laight wrote:

From: Phil Sutter

quoted

Sent: 17 August 2017 18:09
To: Stephen Hemminger
Cc: netdev@vger.kernel.org
Subject: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is NULL-terminated

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

diff --git a/ip/ipntable.c b/ip/ipntable.c
index 879626ee4f491..7be1f04d33d90 100644
--- a/ip/ipntable.c
+++ b/ip/ipntable.c

@@ -633,7 +633,8 @@ static int ipntable_show(int argc, char **argv)
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();

-			strncpy(filter.name, *argv, sizeof(filter.name));
+			strncpy(filter.name, *argv, sizeof(filter.name) - 1);
+			filter.name[sizeof(filter.name) - 1] = '\0';

Why not check for overflow instead?
			if (filter.name[sizeof(filter.name) - 1])
				usage("filer name too long");

sizeof(filter.name) is 1024, which is maybe a bit over the top for
something a user would input. So I found a better way avoiding all this
at once: I made filter.name a const char *, then just assigned *argv to
it. This should be safe since rtnl_dump_filter() and therefore
print_ntable() callback is called from inside ipntable_show() so *argv
is not accessed outside of it's scope.

What do you think?

Thanks, Phil

From: Phil Sutter

Sent: 18 August 2017 11:52
On Fri, Aug 18, 2017 at 09:19:16AM +0000, David Laight wrote:

quoted

From: Phil Sutter

quoted

Sent: 17 August 2017 18:09
To: Stephen Hemminger
Cc: netdev@vger.kernel.org
Subject: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is NULL-terminated

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

diff --git a/ip/ipntable.c b/ip/ipntable.c
index 879626ee4f491..7be1f04d33d90 100644
--- a/ip/ipntable.c
+++ b/ip/ipntable.c

@@ -633,7 +633,8 @@ static int ipntable_show(int argc, char **argv)
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();

-			strncpy(filter.name, *argv, sizeof(filter.name));
+			strncpy(filter.name, *argv, sizeof(filter.name) - 1);
+			filter.name[sizeof(filter.name) - 1] = '\0';

Why not check for overflow instead?
			if (filter.name[sizeof(filter.name) - 1])
				usage("filer name too long");

sizeof(filter.name) is 1024, which is maybe a bit over the top for
something a user would input. So I found a better way avoiding all this
at once: I made filter.name a const char *, then just assigned *argv to
it. This should be safe since rtnl_dump_filter() and therefore
print_ntable() callback is called from inside ipntable_show() so *argv
is not accessed outside of it's scope.

What do you think?

There isn't a scope problem, *argv is program data (written to unusable
stack space by the kernel during exec.

If the filter is done in userpace it is ok, but I'd have thought
it would be passed to kernel later on?

	David

On Thu, 17 Aug 2017 19:09:32 +0200
Phil Sutter [off-list ref] wrote:

This is not really necessary. kernel won't return
an ifname with a length >= IFNAMSIZ.

If you wanted to future proof it, why not use variable size allocation

--- a/lib/ll_map.c
+++ b/lib/ll_map.c

@@ -30,7 +30,7 @@ struct ll_cache {
        unsigned        flags;
        unsigned        index;
        unsigned short  type;
-       char            name[IFNAMSIZ];
+       char            name[];
 };
 
 #define IDXMAP_SIZE    1024

@@ -120,7 +120,7 @@ int ll_remember_index(const struct sockaddr_nl *who,
                return 0;
        }
 
-       im = malloc(sizeof(*im));
+       im = malloc(sizeof(*im) + strlen(ifname) + 1);
        if (im == NULL)
                return 0;
        im->index = ifi->ifi_index;

On Thu, 17 Aug 2017 19:09:29 +0200
Phil Sutter [off-list ref] wrote:

Depending on proto name to be 15 characters or less is a silly
choice. Why not use strdup() and do it right?

On Fri, Aug 18, 2017 at 04:32:47PM +0000, David Laight wrote:

From: Phil Sutter

quoted

Sent: 18 August 2017 11:52
On Fri, Aug 18, 2017 at 09:19:16AM +0000, David Laight wrote:

quoted

From: Phil Sutter

quoted

Sent: 17 August 2017 18:09
To: Stephen Hemminger
Cc: netdev@vger.kernel.org
Subject: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is NULL-terminated

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

diff --git a/ip/ipntable.c b/ip/ipntable.c
index 879626ee4f491..7be1f04d33d90 100644
--- a/ip/ipntable.c
+++ b/ip/ipntable.c

@@ -633,7 +633,8 @@ static int ipntable_show(int argc, char **argv)
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();

-			strncpy(filter.name, *argv, sizeof(filter.name));
+			strncpy(filter.name, *argv, sizeof(filter.name) - 1);
+			filter.name[sizeof(filter.name) - 1] = '\0';

Why not check for overflow instead?
			if (filter.name[sizeof(filter.name) - 1])
				usage("filer name too long");

sizeof(filter.name) is 1024, which is maybe a bit over the top for
something a user would input. So I found a better way avoiding all this
at once: I made filter.name a const char *, then just assigned *argv to
it. This should be safe since rtnl_dump_filter() and therefore
print_ntable() callback is called from inside ipntable_show() so *argv
is not accessed outside of it's scope.

What do you think?

There isn't a scope problem, *argv is program data (written to unusable
stack space by the kernel during exec.

Ah, thanks for the info!

If the filter is done in userpace it is ok, but I'd have thought
it would be passed to kernel later on?

No, it just calls sends RTM_GETNEIGHTBL to kernel and throws away
anything that doesn't match the filter. So filtering happens in
user space exclusively.

Thanks, Phil

On Fri, Aug 18, 2017 at 09:37:33AM -0700, Stephen Hemminger wrote:

On Thu, 17 Aug 2017 19:09:29 +0200
Phil Sutter [off-list ref] wrote:

quoted

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/inet_proto.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/inet_proto.c b/lib/inet_proto.c
index ceda082b12a2e..87ed4769fc3da 100644
--- a/lib/inet_proto.c
+++ b/lib/inet_proto.c

@@ -35,8 +35,10 @@ const char *inet_proto_n2a(int proto, char *buf, int len)
 	pe = getprotobynumber(proto);
 	if (pe) {
 		icache = proto;
-		strncpy(ncache, pe->p_name, 16);
-		strncpy(buf, pe->p_name, len);
+		strncpy(ncache, pe->p_name, 15);
+		ncache[15] = '\0';
+		strncpy(buf, pe->p_name, len - 1);
+		buf[len] = '\0';
 		return buf;
 	}
 	snprintf(buf, len, "ipproto-%d", proto);

@@ -62,7 +64,8 @@ int inet_proto_a2n(const char *buf)
 	pe = getprotobyname(buf);
 	if (pe) {
 		icache = pe->p_proto;
-		strncpy(ncache, pe->p_name, 16);
+		strncpy(ncache, pe->p_name, 15);
+		ncache[15] = '\0';
 		return pe->p_proto;
 	}
 	return -1;

Depending on proto name to be 15 characters or less is a silly
choice. Why not use strdup() and do it right?

Yes, I was puzzled by that as well. Luckily the longest proto name in my
/etc/protocols is rsvp-e2e-ignore which is 15 chars long. I'll change
the patch to use dynamic allocation.

Thanks, Phil