[iproute PATCH v4 0/6] Covscan: Fixes for string termination

[iproute PATCH v4 0/6] Covscan: Fixes for string termination

[Phil Sutter]

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.

Changes since v3:
- Dropped patch 2 since upstream discussion in v3 is not conclusive yet.

Phil Sutter (6):
  ipntable: Avoid memory allocation for filter.name
  lib/fs: Fix format string in find_fs_mount()
  lib/inet_proto: Review inet_proto_{a2n,n2a}()
  lnstat_util: Simplify alloc_and_open() a bit
  tc/m_xt: Fix for potential string buffer overflows
  lib/ll_map: Choose size of new cache items at run-time

 ip/ipntable.c      |  6 +++---
 lib/fs.c           |  2 +-
 lib/inet_proto.c   | 24 +++++++++++++-----------
 lib/ll_map.c       |  4 ++--
 misc/lnstat_util.c |  7 ++-----
 tc/m_xt.c          |  7 ++++---
 6 files changed, 25 insertions(+), 25 deletions(-)

-- 
2.13.1

[iproute PATCH v4 1/6] ipntable: Avoid memory allocation for filter.name

[Phil Sutter]

The original issue was that filter.name might end up unterminated if
user provided string was too long. But in fact it is not necessary to
copy the commandline parameter at all: just make filter.name point to it
instead.

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

diff --git a/ip/ipntable.c b/ip/ipntable.c
index 1837909fa42e7..88236ce0ec1e1 100644
--- a/ip/ipntable.c
+++ b/ip/ipntable.c
@@ -37,7 +37,7 @@ static struct
 	int family;
 	int index;
 #define NONE_DEV	(-1)
-	char name[1024];
+	const char *name;
 } filter;
 
 static void usage(void) __attribute__((noreturn));
@@ -367,7 +367,7 @@ static int print_ntable(const struct sockaddr_nl *who, struct nlmsghdr *n, void
 	if (tb[NDTA_NAME]) {
 		const char *name = rta_getattr_str(tb[NDTA_NAME]);
 
-		if (strlen(filter.name) > 0 && strcmp(filter.name, name))
+		if (filter.name && strcmp(filter.name, name))
 			return 0;
 	}
 	if (tb[NDTA_PARMS]) {
@@ -631,7 +631,7 @@ static int ipntable_show(int argc, char **argv)
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();
 
-			strncpy(filter.name, *argv, sizeof(filter.name));
+			filter.name = *argv;
 		} else
 			invarg("unknown", *argv);
 
-- 
2.13.1

[iproute PATCH v4 2/6] lib/fs: Fix format string in find_fs_mount()

[Phil Sutter]

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

[iproute PATCH v4 3/6] lib/inet_proto: Review inet_proto_{a2n,n2a}()

[Phil Sutter]

The original intent was to make sure strings written by those functions
are NUL-terminated at all times, though it was suggested to get rid of
the 15 char protocol name limit as well which this patch accomplishes.

In addition to that, simplify inet_proto_a2n() a bit: Use the error
checking in get_u8() to find out whether passed 'buf' contains a valid
decimal number instead of checking the first character's value manually.

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

diff --git a/lib/inet_proto.c b/lib/inet_proto.c
index ceda082b12a2e..53c029039b6d5 100644
--- a/lib/inet_proto.c
+++ b/lib/inet_proto.c
@@ -25,7 +25,7 @@
 
 const char *inet_proto_n2a(int proto, char *buf, int len)
 {
-	static char ncache[16];
+	static char *ncache;
 	static int icache = -1;
 	struct protoent *pe;
 
@@ -34,9 +34,12 @@ const char *inet_proto_n2a(int proto, char *buf, int len)
 
 	pe = getprotobynumber(proto);
 	if (pe) {
+		if (icache != -1)
+			free(ncache);
 		icache = proto;
-		strncpy(ncache, pe->p_name, 16);
-		strncpy(buf, pe->p_name, len);
+		ncache = strdup(pe->p_name);
+		strncpy(buf, pe->p_name, len - 1);
+		buf[len - 1] = '\0';
 		return buf;
 	}
 	snprintf(buf, len, "ipproto-%d", proto);
@@ -45,24 +48,23 @@ const char *inet_proto_n2a(int proto, char *buf, int len)
 
 int inet_proto_a2n(const char *buf)
 {
-	static char ncache[16];
+	static char *ncache;
 	static int icache = -1;
 	struct protoent *pe;
+	__u8 ret;
 
-	if (icache>=0 && strcmp(ncache, buf) == 0)
+	if (icache != -1 && strcmp(ncache, buf) == 0)
 		return icache;
 
-	if (buf[0] >= '0' && buf[0] <= '9') {
-		__u8 ret;
-		if (get_u8(&ret, buf, 10))
-			return -1;
+	if (!get_u8(&ret, buf, 10))
 		return ret;
-	}
 
 	pe = getprotobyname(buf);
 	if (pe) {
+		if (icache != -1)
+			free(ncache);
 		icache = pe->p_proto;
-		strncpy(ncache, pe->p_name, 16);
+		ncache = strdup(pe->p_name);
 		return pe->p_proto;
 	}
 	return -1;
-- 
2.13.1

[iproute PATCH v4 4/6] lnstat_util: Simplify alloc_and_open() a bit

[Phil Sutter]

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

[iproute PATCH v4 5/6] tc/m_xt: Fix for potential string buffer overflows

[Phil Sutter]

- 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

[iproute PATCH v4 6/6] lib/ll_map: Choose size of new cache items at run-time

[Phil Sutter]

Instead of having a fixed buffer of 16 bytes for the interface name,
tailor size of new ll_cache entry using the interface name's actual
length. This also makes sure the following call to strcpy() is safe.

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..70684b02042b6 100644
--- 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;
-- 
2.13.1

Re: [iproute PATCH v4 0/6] Covscan: Fixes for string termination

[Stephen Hemminger]

On Thu, 24 Aug 2017 11:51:44 +0200
Phil Sutter <phil@nwl.cc> wrote:

> 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.
> 
> Changes since v3:
> - Dropped patch 2 since upstream discussion in v3 is not conclusive yet.
> 
> Phil Sutter (6):
>   ipntable: Avoid memory allocation for filter.name
>   lib/fs: Fix format string in find_fs_mount()
>   lib/inet_proto: Review inet_proto_{a2n,n2a}()
>   lnstat_util: Simplify alloc_and_open() a bit
>   tc/m_xt: Fix for potential string buffer overflows
>   lib/ll_map: Choose size of new cache items at run-time
> 
>  ip/ipntable.c      |  6 +++---
>  lib/fs.c           |  2 +-
>  lib/inet_proto.c   | 24 +++++++++++++-----------
>  lib/ll_map.c       |  4 ++--
>  misc/lnstat_util.c |  7 ++-----
>  tc/m_xt.c          |  7 ++++---
>  6 files changed, 25 insertions(+), 25 deletions(-)
> 

Applied.