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

[iproute PATCH v3 0/7] 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 v2:
- Rebased onto current upstream master branch.
- Replaced patches 1, 4 and 7 by more appropriate ones given feedback
  from v2 review.

Phil Sutter (7):
  ipntable: Avoid memory allocation for filter.name
  xfrm_state: Make sure alg_name is NULL-terminated
  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 +++---
 ip/xfrm_state.c    |  3 ++-
 lib/fs.c           |  2 +-
 lib/inet_proto.c   | 24 +++++++++++++-----------
 lib/ll_map.c       |  4 ++--
 misc/lnstat_util.c |  7 ++-----
 tc/m_xt.c          |  7 ++++---
 7 files changed, 27 insertions(+), 26 deletions(-)

-- 
2.13.1

[iproute PATCH v3 1/7] 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 879626ee4f491..e0bd9b6ebd155 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));
@@ -369,7 +369,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]) {
@@ -633,7 +633,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 v3 2/7] xfrm_state: Make sure alg_name is NULL-terminated

[Phil Sutter]

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

Re: [iproute PATCH v3 2/7] xfrm_state: Make sure alg_name is NULL-terminated

[Stephen Hemminger]

On Mon, 21 Aug 2017 15:23:36 +0200
Phil Sutter <phil@nwl.cc> wrote:

> 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 */

You are fixing enough of these null terminated string issues, that maybe
introducing strlcpy() would make sense. Either in utils (or -lbsd).

Re: [iproute PATCH v3 2/7] xfrm_state: Make sure alg_name is NULL-terminated

[Phil Sutter]

On Mon, Aug 21, 2017 at 05:28:20PM -0700, Stephen Hemminger wrote:
> On Mon, 21 Aug 2017 15:23:36 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > 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 */
> 
> You are fixing enough of these null terminated string issues, that maybe
> introducing strlcpy() would make sense. Either in utils (or -lbsd).

I thought about that already, but decided against it since we don't want
to truncate user chosen interface names so instead implemented
assert_valid_dev_name() and went on with manually sanitizing the other
cases.

Is adding libbsd as additional dependency acceptable? If not, I could
provide a simple strlcpy() implementation in utils.

Are you fine with a follow-up patch addressing this?

Thanks, Phil

[iproute PATCH v3 3/7] 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 v3 4/7] 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 v3 5/7] 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 v3 6/7] 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 v3 7/7] 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