{"id":818734,"url":"http://patchwork.ozlabs.org/api/patches/818734/?format=json","web_url":"http://patchwork.ozlabs.org/project/netdev/patch/20170926163548.24347-4-phil@nwl.cc/","project":{"id":7,"url":"http://patchwork.ozlabs.org/api/projects/7/?format=json","name":"Linux network development","link_name":"netdev","list_id":"netdev.vger.kernel.org","list_email":"netdev@vger.kernel.org","web_url":null,"scm_url":null,"webscm_url":null,"list_archive_url":"","list_archive_url_format":"","commit_url_format":""},"msgid":"<20170926163548.24347-4-phil@nwl.cc>","list_archive_url":null,"date":"2017-09-26T16:35:48","name":"[iproute,v2,3/3] Check user supplied interface name lengths","commit_ref":null,"pull_url":null,"state":"changes-requested","archived":true,"hash":"5e1bb66d4435497780935c462d5be86ee41c9c47","submitter":{"id":4285,"url":"http://patchwork.ozlabs.org/api/people/4285/?format=json","name":"Phil Sutter","email":"phil@nwl.cc"},"delegate":{"id":389,"url":"http://patchwork.ozlabs.org/api/users/389/?format=json","username":"shemminger","first_name":"stephen","last_name":"hemminger","email":"shemminger@vyatta.com"},"mbox":"http://patchwork.ozlabs.org/project/netdev/patch/20170926163548.24347-4-phil@nwl.cc/mbox/","series":[{"id":5182,"url":"http://patchwork.ozlabs.org/api/series/5182/?format=json","web_url":"http://patchwork.ozlabs.org/project/netdev/list/?series=5182","date":"2017-09-26T16:35:46","name":"Check user supplied interface name lengths","version":2,"mbox":"http://patchwork.ozlabs.org/series/5182/mbox/"}],"comments":"http://patchwork.ozlabs.org/api/patches/818734/comments/","check":"pending","checks":"http://patchwork.ozlabs.org/api/patches/818734/checks/","tags":{},"related":[],"headers":{"Return-Path":"<netdev-owner@vger.kernel.org>","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3y1mmb4Gzjz9t67\n\tfor <patchwork-incoming@ozlabs.org>;\n\tWed, 27 Sep 2017 02:36:15 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S968965AbdIZQgN (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tTue, 26 Sep 2017 12:36:13 -0400","from orbyte.nwl.cc ([151.80.46.58]:58112 \"EHLO orbyte.nwl.cc\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S967740AbdIZQgL (ORCPT <rfc822;netdev@vger.kernel.org>);\n\tTue, 26 Sep 2017 12:36:11 -0400","from localhost ([::1]:52654 helo=xsao)\n\tby orbyte.nwl.cc with esmtp (Exim 4.89)\n\t(envelope-from <phil@nwl.cc>)\n\tid 1dwsqA-0008HI-BI; Tue, 26 Sep 2017 18:36:10 +0200"],"From":"Phil Sutter <phil@nwl.cc>","To":"Stephen Hemminger <stephen@networkplumber.org>","Cc":"netdev@vger.kernel.org","Subject":"[iproute PATCH v2 3/3] Check user supplied interface name lengths","Date":"Tue, 26 Sep 2017 18:35:48 +0200","Message-Id":"<20170926163548.24347-4-phil@nwl.cc>","X-Mailer":"git-send-email 2.13.1","In-Reply-To":"<20170926163548.24347-1-phil@nwl.cc>","References":"<20170926163548.24347-1-phil@nwl.cc>","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<netdev.vger.kernel.org>","X-Mailing-List":"netdev@vger.kernel.org"},"content":"The original problem was that something like:\n\n| strncpy(ifr.ifr_name, *argv, IFNAMSIZ);\n\nmight leave ifr.ifr_name unterminated if length of *argv exceeds\nIFNAMSIZ. In order to fix this, I thought about replacing all those\ncases with (equivalent) calls to snprintf() or even introducing\nstrlcpy(). But as Ulrich Drepper correctly pointed out when rejecting\nthe latter from being added to glibc, truncating a string without\nnotifying the user is not to be considered good practice. So let's\nexcercise what he suggested and reject empty or overlong interface names\nright from the start - this way calls to strncpy() like shown above\nbecome safe and the user has a chance to reconsider what he was trying\nto do.\n\nNote that this doesn't add calls to check_ifname() to all places where\nuser supplied interface name is parsed. In many cases, the interface\nmust exist already and is therefore looked up using ll_name_to_index(),\nso if_nametoindex() will perform the necessary checks already.\n\nSigned-off-by: Phil Sutter <phil@nwl.cc>\n---\nChanges since v1:\n- added missing check to tc/f_flower.c\n- Drop some useless checks from ip/ip{6,}tunnel.c (ll_name_to_index()\n  will detect illegal interface names for us).\n- Renamed assert_valid_dev_name() to the shorter check_ifname().\n- iplink: Check 'name' and 'dev' parameters right where they are parsed.\n- ipl2tp: Drop needless check for p->ifname[0].\n---\n include/utils.h |  1 +\n ip/ip6tunnel.c  |  3 ++-\n ip/ipl2tp.c     |  3 ++-\n ip/iplink.c     | 27 ++++++++-------------------\n ip/ipmaddr.c    |  1 +\n ip/iprule.c     |  4 ++++\n ip/iptunnel.c   |  5 ++++-\n ip/iptuntap.c   |  4 +++-\n lib/utils.c     | 10 ++++++++++\n misc/arpd.c     |  1 +\n tc/f_flower.c   |  1 +\n 11 files changed, 37 insertions(+), 23 deletions(-)","diff":"diff --git a/include/utils.h b/include/utils.h\nindex c9ed230b96044..12f0735e8aa0d 100644\n--- a/include/utils.h\n+++ b/include/utils.h\n@@ -133,6 +133,7 @@ void missarg(const char *) __attribute__((noreturn));\n void invarg(const char *, const char *) __attribute__((noreturn));\n void duparg(const char *, const char *) __attribute__((noreturn));\n void duparg2(const char *, const char *) __attribute__((noreturn));\n+void check_ifname(const char *, const char *);\n int matches(const char *arg, const char *pattern);\n int inet_addr_match(const inet_prefix *a, const inet_prefix *b, int bits);\n \ndiff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c\nindex c12d700e74189..2c10b9c3fa7b5 100644\n--- a/ip/ip6tunnel.c\n+++ b/ip/ip6tunnel.c\n@@ -273,7 +273,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)\n \t\t\t\tusage();\n \t\t\tif (p->name[0])\n \t\t\t\tduparg2(\"name\", *argv);\n-\t\t\tstrncpy(p->name, *argv, IFNAMSIZ - 1);\n+\t\t\tcheck_ifname(\"name\", *argv);\n+\t\t\tstrncpy(p->name, *argv, IFNAMSIZ);\n \t\t\tif (cmd == SIOCCHGTUNNEL && count == 0) {\n \t\t\t\tstruct ip6_tnl_parm2 old_p = {};\n \ndiff --git a/ip/ipl2tp.c b/ip/ipl2tp.c\nindex 88664c909e11f..06f1bd064c914 100644\n--- a/ip/ipl2tp.c\n+++ b/ip/ipl2tp.c\n@@ -182,7 +182,7 @@ static int create_session(struct l2tp_parm *p)\n \tif (p->peer_cookie_len)\n \t\taddattr_l(&req.n, 1024, L2TP_ATTR_PEER_COOKIE,\n \t\t\t  p->peer_cookie,  p->peer_cookie_len);\n-\tif (p->ifname && p->ifname[0])\n+\tif (p->ifname)\n \t\taddattrstrz(&req.n, 1024, L2TP_ATTR_IFNAME, p->ifname);\n \n \tif (rtnl_talk(&genl_rth, &req.n, NULL, 0) < 0)\n@@ -545,6 +545,7 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)\n \t\t\t}\n \t\t} else if (strcmp(*argv, \"name\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"name\", *argv);\n \t\t\tp->ifname = *argv;\n \t\t} else if (strcmp(*argv, \"remote\") == 0) {\n \t\t\tNEXT_ARG();\ndiff --git a/ip/iplink.c b/ip/iplink.c\nindex ff5b56c038d28..f000a7992c92e 100644\n--- a/ip/iplink.c\n+++ b/ip/iplink.c\n@@ -573,6 +573,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,\n \t\t\treq->i.ifi_flags &= ~IFF_UP;\n \t\t} else if (strcmp(*argv, \"name\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"name\", *argv);\n \t\t\t*name = *argv;\n \t\t} else if (strcmp(*argv, \"index\") == 0) {\n \t\t\tNEXT_ARG();\n@@ -848,6 +849,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,\n \t\t\t\tNEXT_ARG();\n \t\t\tif (*dev)\n \t\t\t\tduparg2(\"dev\", *argv);\n+\t\t\tcheck_ifname(\"dev\", *argv);\n \t\t\t*dev = *argv;\n \t\t\tdev_index = ll_name_to_index(*dev);\n \t\t}\n@@ -870,7 +872,6 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,\n \n static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)\n {\n-\tint len;\n \tchar *dev = NULL;\n \tchar *name = NULL;\n \tchar *link = NULL;\n@@ -960,13 +961,8 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)\n \t}\n \n \tif (name) {\n-\t\tlen = strlen(name) + 1;\n-\t\tif (len == 1)\n-\t\t\tinvarg(\"\\\"\\\" is not a valid device identifier\\n\",\n-\t\t\t       \"name\");\n-\t\tif (len > IFNAMSIZ)\n-\t\t\tinvarg(\"\\\"name\\\" too long\\n\", name);\n-\t\taddattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);\n+\t\taddattr_l(&req.n, sizeof(req),\n+\t\t\t  IFLA_IFNAME, name, strlen(name) + 1);\n \t}\n \n \tif (type) {\n@@ -1016,7 +1012,6 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)\n \n int iplink_get(unsigned int flags, char *name, __u32 filt_mask)\n {\n-\tint len;\n \tstruct iplink_req req = {\n \t\t.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),\n \t\t.n.nlmsg_flags = NLM_F_REQUEST | flags,\n@@ -1029,13 +1024,8 @@ int iplink_get(unsigned int flags, char *name, __u32 filt_mask)\n \t} answer;\n \n \tif (name) {\n-\t\tlen = strlen(name) + 1;\n-\t\tif (len == 1)\n-\t\t\tinvarg(\"\\\"\\\" is not a valid device identifier\\n\",\n-\t\t\t\t   \"name\");\n-\t\tif (len > IFNAMSIZ)\n-\t\t\tinvarg(\"\\\"name\\\" too long\\n\", name);\n-\t\taddattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);\n+\t\taddattr_l(&req.n, sizeof(req),\n+\t\t\t  IFLA_IFNAME, name, strlen(name) + 1);\n \t}\n \taddattr32(&req.n, sizeof(req), IFLA_EXT_MASK, filt_mask);\n \n@@ -1265,6 +1255,7 @@ static int do_set(int argc, char **argv)\n \t\t\tflags &= ~IFF_UP;\n \t\t} else if (strcmp(*argv, \"name\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"name\", newname);\n \t\t\tnewname = *argv;\n \t\t} else if (matches(*argv, \"address\") == 0) {\n \t\t\tNEXT_ARG();\n@@ -1355,6 +1346,7 @@ static int do_set(int argc, char **argv)\n \n \t\t\tif (dev)\n \t\t\t\tduparg2(\"dev\", *argv);\n+\t\t\tcheck_ifname(\"dev\", dev);\n \t\t\tdev = *argv;\n \t\t}\n \t\targc--; argv++;\n@@ -1383,9 +1375,6 @@ static int do_set(int argc, char **argv)\n \t}\n \n \tif (newname && strcmp(dev, newname)) {\n-\t\tif (strlen(newname) == 0)\n-\t\t\tinvarg(\"\\\"\\\" is not a valid device identifier\\n\",\n-\t\t\t       \"name\");\n \t\tif (do_changename(dev, newname) < 0)\n \t\t\treturn -1;\n \t\tdev = newname;\ndiff --git a/ip/ipmaddr.c b/ip/ipmaddr.c\nindex 85a69e779563d..282a06153c79a 100644\n--- a/ip/ipmaddr.c\n+++ b/ip/ipmaddr.c\n@@ -284,6 +284,7 @@ static int multiaddr_modify(int cmd, int argc, char **argv)\n \t\t\tNEXT_ARG();\n \t\t\tif (ifr.ifr_name[0])\n \t\t\t\tduparg(\"dev\", *argv);\n+\t\t\tcheck_ifname(\"dev\", *argv);\n \t\t\tstrncpy(ifr.ifr_name, *argv, IFNAMSIZ);\n \t\t} else {\n \t\t\tif (matches(*argv, \"address\") == 0) {\ndiff --git a/ip/iprule.c b/ip/iprule.c\nindex 8313138db815f..33cfd87195212 100644\n--- a/ip/iprule.c\n+++ b/ip/iprule.c\n@@ -472,10 +472,12 @@ static int iprule_list_flush_or_save(int argc, char **argv, int action)\n \t\t} else if (strcmp(*argv, \"dev\") == 0 ||\n \t\t\t   strcmp(*argv, \"iif\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"iif\", *argv);\n \t\t\tstrncpy(filter.iif, *argv, IFNAMSIZ);\n \t\t\tfilter.iifmask = 1;\n \t\t} else if (strcmp(*argv, \"oif\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"oif\", *argv);\n \t\t\tstrncpy(filter.oif, *argv, IFNAMSIZ);\n \t\t\tfilter.oifmask = 1;\n \t\t} else if (strcmp(*argv, \"l3mdev\") == 0) {\n@@ -695,10 +697,12 @@ static int iprule_modify(int cmd, int argc, char **argv)\n \t\t} else if (strcmp(*argv, \"dev\") == 0 ||\n \t\t\t   strcmp(*argv, \"iif\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"dev/iif\", *argv);\n \t\t\taddattr_l(&req.n, sizeof(req), FRA_IFNAME,\n \t\t\t\t  *argv, strlen(*argv)+1);\n \t\t} else if (strcmp(*argv, \"oif\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"oif\", *argv);\n \t\t\taddattr_l(&req.n, sizeof(req), FRA_OIFNAME,\n \t\t\t\t  *argv, strlen(*argv)+1);\n \t\t} else if (strcmp(*argv, \"l3mdev\") == 0) {\ndiff --git a/ip/iptunnel.c b/ip/iptunnel.c\nindex 0acfd0793d3cd..851a80aad73a9 100644\n--- a/ip/iptunnel.c\n+++ b/ip/iptunnel.c\n@@ -178,7 +178,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)\n \n \t\t\tif (p->name[0])\n \t\t\t\tduparg2(\"name\", *argv);\n-\t\t\tstrncpy(p->name, *argv, IFNAMSIZ - 1);\n+\t\t\tcheck_ifname(\"name\", *argv);\n+\t\t\tstrncpy(p->name, *argv, IFNAMSIZ);\n \t\t\tif (cmd == SIOCCHGTUNNEL && count == 0) {\n \t\t\t\tstruct ip_tunnel_parm old_p = {};\n \n@@ -487,6 +488,7 @@ static int do_prl(int argc, char **argv)\n \t\t\tcount++;\n \t\t} else if (strcmp(*argv, \"dev\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"dev\", *argv);\n \t\t\tmedium = *argv;\n \t\t} else {\n \t\t\tfprintf(stderr,\n@@ -534,6 +536,7 @@ static int do_6rd(int argc, char **argv)\n \t\t\tcmd = SIOCDEL6RD;\n \t\t} else if (strcmp(*argv, \"dev\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"dev\", *argv);\n \t\t\tmedium = *argv;\n \t\t} else {\n \t\t\tfprintf(stderr,\ndiff --git a/ip/iptuntap.c b/ip/iptuntap.c\nindex 451f7f0eac6bb..4400dc2fa2a88 100644\n--- a/ip/iptuntap.c\n+++ b/ip/iptuntap.c\n@@ -176,7 +176,8 @@ static int parse_args(int argc, char **argv,\n \t\t\tifr->ifr_flags |= IFF_MULTI_QUEUE;\n \t\t} else if (matches(*argv, \"dev\") == 0) {\n \t\t\tNEXT_ARG();\n-\t\t\tstrncpy(ifr->ifr_name, *argv, IFNAMSIZ-1);\n+\t\t\tcheck_ifname(\"dev\", *argv);\n+\t\t\tstrncpy(ifr->ifr_name, *argv, IFNAMSIZ);\n \t\t} else {\n \t\t\tif (matches(*argv, \"name\") == 0) {\n \t\t\t\tNEXT_ARG();\n@@ -184,6 +185,7 @@ static int parse_args(int argc, char **argv,\n \t\t\t\tusage();\n \t\t\tif (ifr->ifr_name[0])\n \t\t\t\tduparg2(\"name\", *argv);\n+\t\t\tcheck_ifname(\"name\", *argv);\n \t\t\tstrncpy(ifr->ifr_name, *argv, IFNAMSIZ);\n \t\t}\n \t\tcount++;\ndiff --git a/lib/utils.c b/lib/utils.c\nindex bbd3cbc46a0e5..c4a02b8f9f52a 100644\n--- a/lib/utils.c\n+++ b/lib/utils.c\n@@ -699,6 +699,16 @@ void duparg2(const char *key, const char *arg)\n \texit(-1);\n }\n \n+void check_ifname(const char *arg, const char *name)\n+{\n+\tsize_t len = strlen(name);\n+\n+\tif (!len)\n+\t\tinvarg(\"Empty interface name not allowed.\", arg);\n+\tif (len >= IFNAMSIZ)\n+\t\tinvarg(\"Interface name is too long.\", name);\n+}\n+\n int matches(const char *cmd, const char *pattern)\n {\n \tint len = strlen(cmd);\ndiff --git a/misc/arpd.c b/misc/arpd.c\nindex bfab44544ee1d..d42df9e58a9f1 100644\n--- a/misc/arpd.c\n+++ b/misc/arpd.c\n@@ -664,6 +664,7 @@ int main(int argc, char **argv)\n \t\tstruct ifreq ifr = {};\n \n \t\tfor (i = 0; i < ifnum; i++) {\n+\t\t\tcheck_ifname(ifnames[i], ifnames[i]);\n \t\t\tstrncpy(ifr.ifr_name, ifnames[i], IFNAMSIZ);\n \t\t\tif (ioctl(udp_sock, SIOCGIFINDEX, &ifr)) {\n \t\t\t\tperror(\"ioctl(SIOCGIFINDEX)\");\ndiff --git a/tc/f_flower.c b/tc/f_flower.c\nindex 99e62a382dec6..ff45ea7af086e 100644\n--- a/tc/f_flower.c\n+++ b/tc/f_flower.c\n@@ -630,6 +630,7 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,\n \t\t\tflags |= TCA_CLS_FLAGS_SKIP_SW;\n \t\t} else if (matches(*argv, \"indev\") == 0) {\n \t\t\tNEXT_ARG();\n+\t\t\tcheck_ifname(\"indev\", *argv);\n \t\t\taddattrstrz(n, MAX_MSG, TCA_FLOWER_INDEV, *argv);\n \t\t} else if (matches(*argv, \"vlan_id\") == 0) {\n \t\t\t__u16 vid;\n","prefixes":["iproute","v2","3/3"]}