[iproute PATCH v2 0/3] Check user supplied interface name lengths

[iproute PATCH v2 0/3] Check user supplied interface name lengths

[Phil Sutter]

This series adds explicit checks for user-supplied interface names to
make sure their length fits Linux's requirements.

The first two patches simplify interface name parsing in some places -
these are side-effects of working on the actual implementation provided
in patch three.

Changes since v1:
- Patches 1 and 2 introduced.
- Changes to patch 3 are listed in there.

Phil Sutter (3):
  ip{6,}tunnel: Avoid copying user-supplied interface name around
  tc: flower: No need to cache indev arg
  Check user supplied interface name lengths

 include/utils.h |  1 +
 ip/ip6tunnel.c  |  9 +++++----
 ip/ipl2tp.c     |  3 ++-
 ip/iplink.c     | 27 ++++++++-------------------
 ip/ipmaddr.c    |  1 +
 ip/iprule.c     |  4 ++++
 ip/iptunnel.c   | 27 +++++++++++++--------------
 ip/iptuntap.c   |  4 +++-
 lib/utils.c     | 10 ++++++++++
 misc/arpd.c     |  1 +
 tc/f_flower.c   |  6 ++----
 11 files changed, 50 insertions(+), 43 deletions(-)

-- 
2.13.1

[iproute PATCH v2 1/3] ip{6,}tunnel: Avoid copying user-supplied interface name around

[Phil Sutter]

In both files' parse_args() functions as well as in iptunnel's do_prl()
and do_6rd() functions, a user-supplied 'dev' parameter is uselessly
copied into a temporary buffer before passing it to ll_name_to_index()
or copying into a struct ifreq.  Avoid this by just caching the argv
pointer value until the later lookup/strcpy.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ip6tunnel.c |  6 +++---
 ip/iptunnel.c  | 22 +++++++++-------------
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index b4a7def144226..c12d700e74189 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -136,7 +136,7 @@ static void print_tunnel(struct ip6_tnl_parm2 *p)
 static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
 {
 	int count = 0;
-	char medium[IFNAMSIZ] = {};
+	const char *medium = NULL;
 
 	while (argc > 0) {
 		if (strcmp(*argv, "mode") == 0) {
@@ -180,7 +180,7 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
 			memcpy(&p->laddr, &laddr.data, sizeof(p->laddr));
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(medium, *argv, IFNAMSIZ - 1);
+			medium = *argv;
 		} else if (strcmp(*argv, "encaplimit") == 0) {
 			NEXT_ARG();
 			if (strcmp(*argv, "none") == 0) {
@@ -285,7 +285,7 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
 		count++;
 		argc--; argv++;
 	}
-	if (medium[0]) {
+	if (medium) {
 		p->link = ll_name_to_index(medium);
 		if (p->link == 0) {
 			fprintf(stderr, "Cannot find device \"%s\"\n", medium);
diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 105d0f5576f1a..0acfd0793d3cd 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -60,7 +60,7 @@ static void set_tunnel_proto(struct ip_tunnel_parm *p, int proto)
 static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 {
 	int count = 0;
-	char medium[IFNAMSIZ] = {};
+	const char *medium = NULL;
 	int isatap = 0;
 
 	memset(p, 0, sizeof(*p));
@@ -139,7 +139,7 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 				p->iph.saddr = htonl(INADDR_ANY);
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(medium, *argv, IFNAMSIZ - 1);
+			medium = *argv;
 		} else if (strcmp(*argv, "ttl") == 0 ||
 			   strcmp(*argv, "hoplimit") == 0 ||
 			   strcmp(*argv, "hlim") == 0) {
@@ -216,7 +216,7 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 		}
 	}
 
-	if (medium[0]) {
+	if (medium) {
 		p->link = ll_name_to_index(medium);
 		if (p->link == 0) {
 			fprintf(stderr, "Cannot find device \"%s\"\n", medium);
@@ -465,9 +465,8 @@ static int do_prl(int argc, char **argv)
 {
 	struct ip_tunnel_prl p = {};
 	int count = 0;
-	int devname = 0;
 	int cmd = 0;
-	char medium[IFNAMSIZ] = {};
+	const char *medium = NULL;
 
 	while (argc > 0) {
 		if (strcmp(*argv, "prl-default") == 0) {
@@ -488,8 +487,7 @@ static int do_prl(int argc, char **argv)
 			count++;
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(medium, *argv, IFNAMSIZ-1);
-			devname++;
+			medium = *argv;
 		} else {
 			fprintf(stderr,
 				"Invalid PRL parameter \"%s\"\n", *argv);
@@ -502,7 +500,7 @@ static int do_prl(int argc, char **argv)
 		}
 		argc--; argv++;
 	}
-	if (devname == 0) {
+	if (!medium) {
 		fprintf(stderr, "Must specify device\n");
 		exit(-1);
 	}
@@ -513,9 +511,8 @@ static int do_prl(int argc, char **argv)
 static int do_6rd(int argc, char **argv)
 {
 	struct ip_tunnel_6rd ip6rd = {};
-	int devname = 0;
 	int cmd = 0;
-	char medium[IFNAMSIZ] = {};
+	const char *medium = NULL;
 	inet_prefix prefix;
 
 	while (argc > 0) {
@@ -537,8 +534,7 @@ static int do_6rd(int argc, char **argv)
 			cmd = SIOCDEL6RD;
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(medium, *argv, IFNAMSIZ-1);
-			devname++;
+			medium = *argv;
 		} else {
 			fprintf(stderr,
 				"Invalid 6RD parameter \"%s\"\n", *argv);
@@ -546,7 +542,7 @@ static int do_6rd(int argc, char **argv)
 		}
 		argc--; argv++;
 	}
-	if (devname == 0) {
+	if (!medium) {
 		fprintf(stderr, "Must specify device\n");
 		exit(-1);
 	}
-- 
2.13.1

[iproute PATCH v2 2/3] tc: flower: No need to cache indev arg

[Phil Sutter]

Since addattrstrz() will copy the provided string into the attribute
payload, there is no need to cache the data.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tc/f_flower.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tc/f_flower.c b/tc/f_flower.c
index 934832e2bbe90..99e62a382dec6 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -629,11 +629,8 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 		} else if (matches(*argv, "skip_sw") == 0) {
 			flags |= TCA_CLS_FLAGS_SKIP_SW;
 		} else if (matches(*argv, "indev") == 0) {
-			char ifname[IFNAMSIZ] = {};
-
 			NEXT_ARG();
-			strncpy(ifname, *argv, sizeof(ifname) - 1);
-			addattrstrz(n, MAX_MSG, TCA_FLOWER_INDEV, ifname);
+			addattrstrz(n, MAX_MSG, TCA_FLOWER_INDEV, *argv);
 		} else if (matches(*argv, "vlan_id") == 0) {
 			__u16 vid;
 
-- 
2.13.1

[iproute PATCH v2 3/3] Check user supplied interface name lengths

[Phil Sutter]

The original problem was that something like:

| strncpy(ifr.ifr_name, *argv, IFNAMSIZ);

might leave ifr.ifr_name unterminated if length of *argv exceeds
IFNAMSIZ. In order to fix this, I thought about replacing all those
cases with (equivalent) calls to snprintf() or even introducing
strlcpy(). But as Ulrich Drepper correctly pointed out when rejecting
the latter from being added to glibc, truncating a string without
notifying the user is not to be considered good practice. So let's
excercise what he suggested and reject empty or overlong interface names
right from the start - this way calls to strncpy() like shown above
become safe and the user has a chance to reconsider what he was trying
to do.

Note that this doesn't add calls to check_ifname() to all places where
user supplied interface name is parsed. In many cases, the interface
must exist already and is therefore looked up using ll_name_to_index(),
so if_nametoindex() will perform the necessary checks already.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- added missing check to tc/f_flower.c
- Drop some useless checks from ip/ip{6,}tunnel.c (ll_name_to_index()
  will detect illegal interface names for us).
- Renamed assert_valid_dev_name() to the shorter check_ifname().
- iplink: Check 'name' and 'dev' parameters right where they are parsed.
- ipl2tp: Drop needless check for p->ifname[0].
---
 include/utils.h |  1 +
 ip/ip6tunnel.c  |  3 ++-
 ip/ipl2tp.c     |  3 ++-
 ip/iplink.c     | 27 ++++++++-------------------
 ip/ipmaddr.c    |  1 +
 ip/iprule.c     |  4 ++++
 ip/iptunnel.c   |  5 ++++-
 ip/iptuntap.c   |  4 +++-
 lib/utils.c     | 10 ++++++++++
 misc/arpd.c     |  1 +
 tc/f_flower.c   |  1 +
 11 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index c9ed230b96044..12f0735e8aa0d 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -133,6 +133,7 @@ void missarg(const char *) __attribute__((noreturn));
 void invarg(const char *, const char *) __attribute__((noreturn));
 void duparg(const char *, const char *) __attribute__((noreturn));
 void duparg2(const char *, const char *) __attribute__((noreturn));
+void check_ifname(const char *, const char *);
 int matches(const char *arg, const char *pattern);
 int inet_addr_match(const inet_prefix *a, const inet_prefix *b, int bits);
 
diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index c12d700e74189..2c10b9c3fa7b5 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -273,7 +273,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
 				usage();
 			if (p->name[0])
 				duparg2("name", *argv);
-			strncpy(p->name, *argv, IFNAMSIZ - 1);
+			check_ifname("name", *argv);
+			strncpy(p->name, *argv, IFNAMSIZ);
 			if (cmd == SIOCCHGTUNNEL && count == 0) {
 				struct ip6_tnl_parm2 old_p = {};
 
diff --git a/ip/ipl2tp.c b/ip/ipl2tp.c
index 88664c909e11f..06f1bd064c914 100644
--- a/ip/ipl2tp.c
+++ b/ip/ipl2tp.c
@@ -182,7 +182,7 @@ static int create_session(struct l2tp_parm *p)
 	if (p->peer_cookie_len)
 		addattr_l(&req.n, 1024, L2TP_ATTR_PEER_COOKIE,
 			  p->peer_cookie,  p->peer_cookie_len);
-	if (p->ifname && p->ifname[0])
+	if (p->ifname)
 		addattrstrz(&req.n, 1024, L2TP_ATTR_IFNAME, p->ifname);
 
 	if (rtnl_talk(&genl_rth, &req.n, NULL, 0) < 0)
@@ -545,6 +545,7 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
 			}
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();
+			check_ifname("name", *argv);
 			p->ifname = *argv;
 		} else if (strcmp(*argv, "remote") == 0) {
 			NEXT_ARG();
diff --git a/ip/iplink.c b/ip/iplink.c
index ff5b56c038d28..f000a7992c92e 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -573,6 +573,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 			req->i.ifi_flags &= ~IFF_UP;
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();
+			check_ifname("name", *argv);
 			*name = *argv;
 		} else if (strcmp(*argv, "index") == 0) {
 			NEXT_ARG();
@@ -848,6 +849,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 				NEXT_ARG();
 			if (*dev)
 				duparg2("dev", *argv);
+			check_ifname("dev", *argv);
 			*dev = *argv;
 			dev_index = ll_name_to_index(*dev);
 		}
@@ -870,7 +872,6 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 
 static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 {
-	int len;
 	char *dev = NULL;
 	char *name = NULL;
 	char *link = NULL;
@@ -960,13 +961,8 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 	}
 
 	if (name) {
-		len = strlen(name) + 1;
-		if (len == 1)
-			invarg("\"\" is not a valid device identifier\n",
-			       "name");
-		if (len > IFNAMSIZ)
-			invarg("\"name\" too long\n", name);
-		addattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);
+		addattr_l(&req.n, sizeof(req),
+			  IFLA_IFNAME, name, strlen(name) + 1);
 	}
 
 	if (type) {
@@ -1016,7 +1012,6 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 
 int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
 {
-	int len;
 	struct iplink_req req = {
 		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
 		.n.nlmsg_flags = NLM_F_REQUEST | flags,
@@ -1029,13 +1024,8 @@ int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
 	} answer;
 
 	if (name) {
-		len = strlen(name) + 1;
-		if (len == 1)
-			invarg("\"\" is not a valid device identifier\n",
-				   "name");
-		if (len > IFNAMSIZ)
-			invarg("\"name\" too long\n", name);
-		addattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);
+		addattr_l(&req.n, sizeof(req),
+			  IFLA_IFNAME, name, strlen(name) + 1);
 	}
 	addattr32(&req.n, sizeof(req), IFLA_EXT_MASK, filt_mask);
 
@@ -1265,6 +1255,7 @@ static int do_set(int argc, char **argv)
 			flags &= ~IFF_UP;
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();
+			check_ifname("name", newname);
 			newname = *argv;
 		} else if (matches(*argv, "address") == 0) {
 			NEXT_ARG();
@@ -1355,6 +1346,7 @@ static int do_set(int argc, char **argv)
 
 			if (dev)
 				duparg2("dev", *argv);
+			check_ifname("dev", dev);
 			dev = *argv;
 		}
 		argc--; argv++;
@@ -1383,9 +1375,6 @@ static int do_set(int argc, char **argv)
 	}
 
 	if (newname && strcmp(dev, newname)) {
-		if (strlen(newname) == 0)
-			invarg("\"\" is not a valid device identifier\n",
-			       "name");
 		if (do_changename(dev, newname) < 0)
 			return -1;
 		dev = newname;
diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c
index 85a69e779563d..282a06153c79a 100644
--- a/ip/ipmaddr.c
+++ b/ip/ipmaddr.c
@@ -284,6 +284,7 @@ static int multiaddr_modify(int cmd, int argc, char **argv)
 			NEXT_ARG();
 			if (ifr.ifr_name[0])
 				duparg("dev", *argv);
+			check_ifname("dev", *argv);
 			strncpy(ifr.ifr_name, *argv, IFNAMSIZ);
 		} else {
 			if (matches(*argv, "address") == 0) {
diff --git a/ip/iprule.c b/ip/iprule.c
index 8313138db815f..33cfd87195212 100644
--- a/ip/iprule.c
+++ b/ip/iprule.c
@@ -472,10 +472,12 @@ static int iprule_list_flush_or_save(int argc, char **argv, int action)
 		} else if (strcmp(*argv, "dev") == 0 ||
 			   strcmp(*argv, "iif") == 0) {
 			NEXT_ARG();
+			check_ifname("iif", *argv);
 			strncpy(filter.iif, *argv, IFNAMSIZ);
 			filter.iifmask = 1;
 		} else if (strcmp(*argv, "oif") == 0) {
 			NEXT_ARG();
+			check_ifname("oif", *argv);
 			strncpy(filter.oif, *argv, IFNAMSIZ);
 			filter.oifmask = 1;
 		} else if (strcmp(*argv, "l3mdev") == 0) {
@@ -695,10 +697,12 @@ static int iprule_modify(int cmd, int argc, char **argv)
 		} else if (strcmp(*argv, "dev") == 0 ||
 			   strcmp(*argv, "iif") == 0) {
 			NEXT_ARG();
+			check_ifname("dev/iif", *argv);
 			addattr_l(&req.n, sizeof(req), FRA_IFNAME,
 				  *argv, strlen(*argv)+1);
 		} else if (strcmp(*argv, "oif") == 0) {
 			NEXT_ARG();
+			check_ifname("oif", *argv);
 			addattr_l(&req.n, sizeof(req), FRA_OIFNAME,
 				  *argv, strlen(*argv)+1);
 		} else if (strcmp(*argv, "l3mdev") == 0) {
diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 0acfd0793d3cd..851a80aad73a9 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -178,7 +178,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 
 			if (p->name[0])
 				duparg2("name", *argv);
-			strncpy(p->name, *argv, IFNAMSIZ - 1);
+			check_ifname("name", *argv);
+			strncpy(p->name, *argv, IFNAMSIZ);
 			if (cmd == SIOCCHGTUNNEL && count == 0) {
 				struct ip_tunnel_parm old_p = {};
 
@@ -487,6 +488,7 @@ static int do_prl(int argc, char **argv)
 			count++;
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
+			check_ifname("dev", *argv);
 			medium = *argv;
 		} else {
 			fprintf(stderr,
@@ -534,6 +536,7 @@ static int do_6rd(int argc, char **argv)
 			cmd = SIOCDEL6RD;
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
+			check_ifname("dev", *argv);
 			medium = *argv;
 		} else {
 			fprintf(stderr,
diff --git a/ip/iptuntap.c b/ip/iptuntap.c
index 451f7f0eac6bb..4400dc2fa2a88 100644
--- a/ip/iptuntap.c
+++ b/ip/iptuntap.c
@@ -176,7 +176,8 @@ static int parse_args(int argc, char **argv,
 			ifr->ifr_flags |= IFF_MULTI_QUEUE;
 		} else if (matches(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(ifr->ifr_name, *argv, IFNAMSIZ-1);
+			check_ifname("dev", *argv);
+			strncpy(ifr->ifr_name, *argv, IFNAMSIZ);
 		} else {
 			if (matches(*argv, "name") == 0) {
 				NEXT_ARG();
@@ -184,6 +185,7 @@ static int parse_args(int argc, char **argv,
 				usage();
 			if (ifr->ifr_name[0])
 				duparg2("name", *argv);
+			check_ifname("name", *argv);
 			strncpy(ifr->ifr_name, *argv, IFNAMSIZ);
 		}
 		count++;
diff --git a/lib/utils.c b/lib/utils.c
index bbd3cbc46a0e5..c4a02b8f9f52a 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -699,6 +699,16 @@ void duparg2(const char *key, const char *arg)
 	exit(-1);
 }
 
+void check_ifname(const char *arg, const char *name)
+{
+	size_t len = strlen(name);
+
+	if (!len)
+		invarg("Empty interface name not allowed.", arg);
+	if (len >= IFNAMSIZ)
+		invarg("Interface name is too long.", name);
+}
+
 int matches(const char *cmd, const char *pattern)
 {
 	int len = strlen(cmd);
diff --git a/misc/arpd.c b/misc/arpd.c
index bfab44544ee1d..d42df9e58a9f1 100644
--- a/misc/arpd.c
+++ b/misc/arpd.c
@@ -664,6 +664,7 @@ int main(int argc, char **argv)
 		struct ifreq ifr = {};
 
 		for (i = 0; i < ifnum; i++) {
+			check_ifname(ifnames[i], ifnames[i]);
 			strncpy(ifr.ifr_name, ifnames[i], IFNAMSIZ);
 			if (ioctl(udp_sock, SIOCGIFINDEX, &ifr)) {
 				perror("ioctl(SIOCGIFINDEX)");
diff --git a/tc/f_flower.c b/tc/f_flower.c
index 99e62a382dec6..ff45ea7af086e 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -630,6 +630,7 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 			flags |= TCA_CLS_FLAGS_SKIP_SW;
 		} else if (matches(*argv, "indev") == 0) {
 			NEXT_ARG();
+			check_ifname("indev", *argv);
 			addattrstrz(n, MAX_MSG, TCA_FLOWER_INDEV, *argv);
 		} else if (matches(*argv, "vlan_id") == 0) {
 			__u16 vid;
-- 
2.13.1

Re: [iproute PATCH v2 0/3] Check user supplied interface name lengths

[Stephen Hemminger]

On Tue, 26 Sep 2017 18:35:45 +0200
Phil Sutter <phil@nwl.cc> wrote:

> This series adds explicit checks for user-supplied interface names to
> make sure their length fits Linux's requirements.
> 
> The first two patches simplify interface name parsing in some places -
> these are side-effects of working on the actual implementation provided
> in patch three.
> 
> Changes since v1:
> - Patches 1 and 2 introduced.
> - Changes to patch 3 are listed in there.
> 
> Phil Sutter (3):
>   ip{6,}tunnel: Avoid copying user-supplied interface name around
>   tc: flower: No need to cache indev arg
>   Check user supplied interface name lengths
> 
>  include/utils.h |  1 +
>  ip/ip6tunnel.c  |  9 +++++----
>  ip/ipl2tp.c     |  3 ++-
>  ip/iplink.c     | 27 ++++++++-------------------
>  ip/ipmaddr.c    |  1 +
>  ip/iprule.c     |  4 ++++
>  ip/iptunnel.c   | 27 +++++++++++++--------------
>  ip/iptuntap.c   |  4 +++-
>  lib/utils.c     | 10 ++++++++++
>  misc/arpd.c     |  1 +
>  tc/f_flower.c   |  6 ++----
>  11 files changed, 50 insertions(+), 43 deletions(-)
> 

I like the idea, and checking arguments is good.
Why not merge the check and copy and put in lib/utils.c

int get_ifname(char *name, const char *arg)
{
...

Re: [iproute PATCH v2 0/3] Check user supplied interface name lengths

[Phil Sutter]

On Wed, Sep 27, 2017 at 08:42:49AM +0100, Stephen Hemminger wrote:
> On Tue, 26 Sep 2017 18:35:45 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > This series adds explicit checks for user-supplied interface names to
> > make sure their length fits Linux's requirements.
> > 
> > The first two patches simplify interface name parsing in some places -
> > these are side-effects of working on the actual implementation provided
> > in patch three.
> > 
> > Changes since v1:
> > - Patches 1 and 2 introduced.
> > - Changes to patch 3 are listed in there.
> > 
> > Phil Sutter (3):
> >   ip{6,}tunnel: Avoid copying user-supplied interface name around
> >   tc: flower: No need to cache indev arg
> >   Check user supplied interface name lengths
> > 
> >  include/utils.h |  1 +
> >  ip/ip6tunnel.c  |  9 +++++----
> >  ip/ipl2tp.c     |  3 ++-
> >  ip/iplink.c     | 27 ++++++++-------------------
> >  ip/ipmaddr.c    |  1 +
> >  ip/iprule.c     |  4 ++++
> >  ip/iptunnel.c   | 27 +++++++++++++--------------
> >  ip/iptuntap.c   |  4 +++-
> >  lib/utils.c     | 10 ++++++++++
> >  misc/arpd.c     |  1 +
> >  tc/f_flower.c   |  6 ++----
> >  11 files changed, 50 insertions(+), 43 deletions(-)
> > 
> 
> I like the idea, and checking arguments is good.

Cool!

> Why not merge the check and copy and put in lib/utils.c
> 
> int get_ifname(char *name, const char *arg)
> {
> ...

What do you have in mind exactly? There are basically three situations
to which check_ifname() is added:

1) Simple pointer caching:

   | check_ifname("name", *argv);
   | name = *argv;

2) Value caching:

   | check_ifname("name", *argv);
   | strncpy(name, *argv, IFNAMSIZ);

3) Direct netlink attribute creation:

   | check_ifname("name", *argv);
   | addattr_l(&req.n, sizeof(req), IFNAME, *argv, strlen(*argv) + 1);

To cover them all, I could introduce the following:

| char *check_ifname(const char *name, const char *argv)
| {
| 	/* check *arg, call invarg() if invalid */
| 	return *argv;
| }
| 
| void copy_ifname(char *dst, const char *name, const char *argv)
| {
| 	strncpy(dst, check_ifname(name, argv), IFNAMSIZ);
| }
| 
| void addattr_ifname(struct nlmsghdr *n, int maxlen, int type,
| 		    const char *name, const char *argv)
| {
| 	addattr_l(n, maxlen, type, check_ifname(name, argv),
| 		  strlen(*argv) + 1);
| }

What do you think?

Cheers, Phil

Re: [iproute PATCH v2 0/3] Check user supplied interface name lengths

[Stephen Hemminger]

On Wed, 27 Sep 2017 18:05:28 +0200
Phil Sutter <phil@nwl.cc> wrote:

> On Wed, Sep 27, 2017 at 08:42:49AM +0100, Stephen Hemminger wrote:
> > On Tue, 26 Sep 2017 18:35:45 +0200
> > Phil Sutter <phil@nwl.cc> wrote:
> >   
> > > This series adds explicit checks for user-supplied interface names to
> > > make sure their length fits Linux's requirements.
> > > 
> > > The first two patches simplify interface name parsing in some places -
> > > these are side-effects of working on the actual implementation provided
> > > in patch three.
> > > 
> > > Changes since v1:
> > > - Patches 1 and 2 introduced.
> > > - Changes to patch 3 are listed in there.
> > > 
> > > Phil Sutter (3):
> > >   ip{6,}tunnel: Avoid copying user-supplied interface name around
> > >   tc: flower: No need to cache indev arg
> > >   Check user supplied interface name lengths
> > > 
> > >  include/utils.h |  1 +
> > >  ip/ip6tunnel.c  |  9 +++++----
> > >  ip/ipl2tp.c     |  3 ++-
> > >  ip/iplink.c     | 27 ++++++++-------------------
> > >  ip/ipmaddr.c    |  1 +
> > >  ip/iprule.c     |  4 ++++
> > >  ip/iptunnel.c   | 27 +++++++++++++--------------
> > >  ip/iptuntap.c   |  4 +++-
> > >  lib/utils.c     | 10 ++++++++++
> > >  misc/arpd.c     |  1 +
> > >  tc/f_flower.c   |  6 ++----
> > >  11 files changed, 50 insertions(+), 43 deletions(-)
> > >   
> > 
> > I like the idea, and checking arguments is good.  
> 
> Cool!

I was thinking something like:



diff --git a/include/utils.h b/include/utils.h
index c9ed230b9604..e2702b56f2e0 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -105,6 +105,8 @@ int get_be64(__be64 *val, const char *arg, int base);
 int get_be32(__be32 *val, const char *arg, int base);
 int get_be16(__be16 *val, const char *arg, int base);
 int get_addr64(__u64 *ap, const char *cp);
+int check_ifname(const char *arg);
+int get_ifname(char *buf, const char *arg);
 
 int hex2mem(const char *buf, uint8_t *mem, int count);
 char *hexstring_n2a(const __u8 *str, int len, char *buf, int blen);
diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index b4a7def14422..a6f0e99bdc21 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -180,7 +180,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
 			memcpy(&p->laddr, &laddr.data, sizeof(p->laddr));
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(medium, *argv, IFNAMSIZ - 1);
+			if (get_ifname(medium, *argv))
+				invarg("\"medium\" not a valid ifname", *argv);
 		} else if (strcmp(*argv, "encaplimit") == 0) {
 			NEXT_ARG();
 			if (strcmp(*argv, "none") == 0) {
diff --git a/ip/iplink.c b/ip/iplink.c
index ff5b56c038d2..89aa51ed3b40 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -1265,6 +1265,8 @@ static int do_set(int argc, char **argv)
 			flags &= ~IFF_UP;
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();
+			if (check_ifname(*argv))
+				invarg("Invalid \"name\"\n", *argv);
 			newname = *argv;
 		} else if (matches(*argv, "address") == 0) {
 			NEXT_ARG();
@@ -1383,9 +1385,6 @@ static int do_set(int argc, char **argv)
 	}
 
 	if (newname && strcmp(dev, newname)) {
-		if (strlen(newname) == 0)
-			invarg("\"\" is not a valid device identifier\n",
-			       "name");
 		if (do_changename(dev, newname) < 0)
 			return -1;
 		dev = newname;
diff --git a/lib/utils.c b/lib/utils.c
index bbd3cbc46a0e..a93b45b51a3b 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -20,6 +20,7 @@
 #include <sys/socket.h>
 #include <netinet/in.h>
 #include <string.h>
+#include <ctype.h>
 #include <netdb.h>
 #include <arpa/inet.h>
 #include <asm/types.h>
@@ -465,6 +466,34 @@ int get_addr64(__u64 *ap, const char *cp)
 	return 1;
 }
 
+int check_ifname(const char *name)
+{
+	/* These check mimic kernel checks in dev_valid_name */
+	if (*name == '\0')
+		return -1;
+	if (strlen(name) >= IFNAMSIZ)
+		return -1;
+
+	while (*name) {
+		if (*name == '/' || isspace(*name))
+			return -1;
+		++name;
+	}
+	return 0;
+}
+		
+/* buf is assumed to be IFNAMSIZ */
+int get_ifname(char *buf, const char *name)
+{
+	int ret;
+
+	ret = check_ifname(name);
+	if (ret == 0)
+		strncpy(buf, name, IFNAMSIZ - 1);
+
+	return ret;
+}
+
 int get_addr_1(inet_prefix *addr, const char *name, int family)
 {
 	memset(addr, 0, sizeof(*addr));
-- 
2.11.0

Re: [iproute PATCH v2 0/3] Check user supplied interface name lengths

[Phil Sutter]

On Fri, Sep 29, 2017 at 10:31:07AM -0700, Stephen Hemminger wrote:
[...]
> I was thinking something like:
> 
> 
> 
> diff --git a/include/utils.h b/include/utils.h
> index c9ed230b9604..e2702b56f2e0 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -105,6 +105,8 @@ int get_be64(__be64 *val, const char *arg, int base);
>  int get_be32(__be32 *val, const char *arg, int base);
>  int get_be16(__be16 *val, const char *arg, int base);
>  int get_addr64(__u64 *ap, const char *cp);
> +int check_ifname(const char *arg);
> +int get_ifname(char *buf, const char *arg);

Ah, understood. I'll change my patch accordingly.

Thanks, Phil