Skip to content

Commit 625df64

Browse files
Phil Suttershemminger
Phil Sutter
authored and
shemminger
committed
Check user supplied interface name lengths
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, overlong or otherwise invalid 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>
1 parent ee47484 commit 625df64

File tree

11 files changed

+72
-28
lines changed

11 files changed

+72
-28
lines changed

‎include/utils.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,8 @@ void missarg(const char *) __attribute__((noreturn));
133133
void invarg(const char *, const char *) __attribute__((noreturn));
134134
void duparg(const char *, const char *) __attribute__((noreturn));
135135
void duparg2(const char *, const char *) __attribute__((noreturn));
136+
int check_ifname(const char *);
137+
int get_ifname(char *, const char *);
136138
int matches(const char *arg, const char *pattern);
137139
int inet_addr_match(const inet_prefix *a, const inet_prefix *b, int bits);
138140

‎ip/ip6tunnel.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
273273
usage();
274274
if (p->name[0])
275275
duparg2("name", *argv);
276-
strncpy(p->name, *argv, IFNAMSIZ - 1);
276+
if (get_ifname(p->name, *argv))
277+
invarg("\"name\" not a valid ifname", *argv);
277278
if (cmd == SIOCCHGTUNNEL && count == 0) {
278279
struct ip6_tnl_parm2 old_p = {};
279280

‎ip/ipl2tp.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ static int create_session(struct l2tp_parm *p)
182182
if (p->peer_cookie_len)
183183
addattr_l(&req.n, 1024, L2TP_ATTR_PEER_COOKIE,
184184
p->peer_cookie, p->peer_cookie_len);
185-
if (p->ifname && p->ifname[0])
185+
if (p->ifname)
186186
addattrstrz(&req.n, 1024, L2TP_ATTR_IFNAME, p->ifname);
187187

188188
if (rtnl_talk(&genl_rth, &req.n, NULL, 0) < 0)
@@ -545,6 +545,8 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p)
545545
}
546546
} else if (strcmp(*argv, "name") == 0) {
547547
NEXT_ARG();
548+
if (check_ifname(*argv))
549+
invarg("\"name\" not a valid ifname", *argv);
548550
p->ifname = *argv;
549551
} else if (strcmp(*argv, "remote") == 0) {
550552
NEXT_ARG();

‎ip/iplink.c

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -573,6 +573,8 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
573573
req->i.ifi_flags &= ~IFF_UP;
574574
} else if (strcmp(*argv, "name") == 0) {
575575
NEXT_ARG();
576+
if (check_ifname(*argv))
577+
invarg("\"name\" not a valid ifname", *argv);
576578
*name = *argv;
577579
} else if (strcmp(*argv, "index") == 0) {
578580
NEXT_ARG();
@@ -848,6 +850,8 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
848850
NEXT_ARG();
849851
if (*dev)
850852
duparg2("dev", *argv);
853+
if (check_ifname(*argv))
854+
invarg("\"dev\" not a valid ifname", *argv);
851855
*dev = *argv;
852856
dev_index = ll_name_to_index(*dev);
853857
}
@@ -870,7 +874,6 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
870874

871875
static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
872876
{
873-
int len;
874877
char *dev = NULL;
875878
char *name = NULL;
876879
char *link = NULL;
@@ -960,13 +963,8 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
960963
}
961964

962965
if (name) {
963-
len = strlen(name) + 1;
964-
if (len == 1)
965-
invarg("\"\" is not a valid device identifier\n",
966-
"name");
967-
if (len > IFNAMSIZ)
968-
invarg("\"name\" too long\n", name);
969-
addattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);
966+
addattr_l(&req.n, sizeof(req),
967+
IFLA_IFNAME, name, strlen(name) + 1);
970968
}
971969

972970
if (type) {
@@ -1016,7 +1014,6 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
10161014

10171015
int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
10181016
{
1019-
int len;
10201017
struct iplink_req req = {
10211018
.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
10221019
.n.nlmsg_flags = NLM_F_REQUEST | flags,
@@ -1029,13 +1026,8 @@ int iplink_get(unsigned int flags, char *name, __u32 filt_mask)
10291026
} answer;
10301027

10311028
if (name) {
1032-
len = strlen(name) + 1;
1033-
if (len == 1)
1034-
invarg("\"\" is not a valid device identifier\n",
1035-
"name");
1036-
if (len > IFNAMSIZ)
1037-
invarg("\"name\" too long\n", name);
1038-
addattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, len);
1029+
addattr_l(&req.n, sizeof(req),
1030+
IFLA_IFNAME, name, strlen(name) + 1);
10391031
}
10401032
addattr32(&req.n, sizeof(req), IFLA_EXT_MASK, filt_mask);
10411033

@@ -1265,6 +1257,8 @@ static int do_set(int argc, char **argv)
12651257
flags &= ~IFF_UP;
12661258
} else if (strcmp(*argv, "name") == 0) {
12671259
NEXT_ARG();
1260+
if (check_ifname(*argv))
1261+
invarg("\"name\" not a valid ifname", *argv);
12681262
newname = *argv;
12691263
} else if (matches(*argv, "address") == 0) {
12701264
NEXT_ARG();
@@ -1355,6 +1349,8 @@ static int do_set(int argc, char **argv)
13551349

13561350
if (dev)
13571351
duparg2("dev", *argv);
1352+
if (check_ifname(*argv))
1353+
invarg("\"dev\" not a valid ifname", *argv);
13581354
dev = *argv;
13591355
}
13601356
argc--; argv++;
@@ -1383,9 +1379,6 @@ static int do_set(int argc, char **argv)
13831379
}
13841380

13851381
if (newname && strcmp(dev, newname)) {
1386-
if (strlen(newname) == 0)
1387-
invarg("\"\" is not a valid device identifier\n",
1388-
"name");
13891382
if (do_changename(dev, newname) < 0)
13901383
return -1;
13911384
dev = newname;

‎ip/ipmaddr.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,8 @@ static int multiaddr_modify(int cmd, int argc, char **argv)
284284
NEXT_ARG();
285285
if (ifr.ifr_name[0])
286286
duparg("dev", *argv);
287-
strncpy(ifr.ifr_name, *argv, IFNAMSIZ);
287+
if (get_ifname(ifr.ifr_name, *argv))
288+
invarg("\"dev\" not a valid ifname", *argv);
288289
} else {
289290
if (matches(*argv, "address") == 0) {
290291
NEXT_ARG();

‎ip/iprule.c

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -472,11 +472,13 @@ static int iprule_list_flush_or_save(int argc, char **argv, int action)
472472
} else if (strcmp(*argv, "dev") == 0 ||
473473
strcmp(*argv, "iif") == 0) {
474474
NEXT_ARG();
475-
strncpy(filter.iif, *argv, IFNAMSIZ);
475+
if (get_ifname(filter.iif, *argv))
476+
invarg("\"iif\"/\"dev\" not a valid ifname", *argv);
476477
filter.iifmask = 1;
477478
} else if (strcmp(*argv, "oif") == 0) {
478479
NEXT_ARG();
479-
strncpy(filter.oif, *argv, IFNAMSIZ);
480+
if (get_ifname(filter.oif, *argv))
481+
invarg("\"oif\" not a valid ifname", *argv);
480482
filter.oifmask = 1;
481483
} else if (strcmp(*argv, "l3mdev") == 0) {
482484
filter.l3mdev = 1;
@@ -695,10 +697,14 @@ static int iprule_modify(int cmd, int argc, char **argv)
695697
} else if (strcmp(*argv, "dev") == 0 ||
696698
strcmp(*argv, "iif") == 0) {
697699
NEXT_ARG();
700+
if (check_ifname(*argv))
701+
invarg("\"iif\"/\"dev\" not a valid ifname", *argv);
698702
addattr_l(&req.n, sizeof(req), FRA_IFNAME,
699703
*argv, strlen(*argv)+1);
700704
} else if (strcmp(*argv, "oif") == 0) {
701705
NEXT_ARG();
706+
if (check_ifname(*argv))
707+
invarg("\"oif\" not a valid ifname", *argv);
702708
addattr_l(&req.n, sizeof(req), FRA_OIFNAME,
703709
*argv, strlen(*argv)+1);
704710
} else if (strcmp(*argv, "l3mdev") == 0) {

‎ip/iptunnel.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,8 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
178178

179179
if (p->name[0])
180180
duparg2("name", *argv);
181-
strncpy(p->name, *argv, IFNAMSIZ - 1);
181+
if (get_ifname(p->name, *argv))
182+
invarg("\"name\" not a valid ifname", *argv);
182183
if (cmd == SIOCCHGTUNNEL && count == 0) {
183184
struct ip_tunnel_parm old_p = {};
184185

@@ -487,6 +488,8 @@ static int do_prl(int argc, char **argv)
487488
count++;
488489
} else if (strcmp(*argv, "dev") == 0) {
489490
NEXT_ARG();
491+
if (check_ifname(*argv))
492+
invarg("\"dev\" not a valid ifname", *argv);
490493
medium = *argv;
491494
} else {
492495
fprintf(stderr,
@@ -534,6 +537,8 @@ static int do_6rd(int argc, char **argv)
534537
cmd = SIOCDEL6RD;
535538
} else if (strcmp(*argv, "dev") == 0) {
536539
NEXT_ARG();
540+
if (check_ifname(*argv))
541+
invarg("\"dev\" not a valid ifname", *argv);
537542
medium = *argv;
538543
} else {
539544
fprintf(stderr,

‎ip/iptuntap.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,15 +176,17 @@ static int parse_args(int argc, char **argv,
176176
ifr->ifr_flags |= IFF_MULTI_QUEUE;
177177
} else if (matches(*argv, "dev") == 0) {
178178
NEXT_ARG();
179-
strncpy(ifr->ifr_name, *argv, IFNAMSIZ-1);
179+
if (get_ifname(ifr->ifr_name, *argv))
180+
invarg("\"dev\" not a valid ifname", *argv);
180181
} else {
181182
if (matches(*argv, "name") == 0) {
182183
NEXT_ARG();
183184
} else if (matches(*argv, "help") == 0)
184185
usage();
185186
if (ifr->ifr_name[0])
186187
duparg2("name", *argv);
187-
strncpy(ifr->ifr_name, *argv, IFNAMSIZ);
188+
if (get_ifname(ifr->ifr_name, *argv))
189+
invarg("\"name\" not a valid ifname", *argv);
188190
}
189191
count++;
190192
argc--; argv++;

‎lib/utils.c

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include <sys/socket.h>
2121
#include <netinet/in.h>
2222
#include <string.h>
23+
#include <ctype.h>
2324
#include <netdb.h>
2425
#include <arpa/inet.h>
2526
#include <asm/types.h>
@@ -699,6 +700,34 @@ void duparg2(const char *key, const char *arg)
699700
exit(-1);
700701
}
701702

703+
int check_ifname(const char *name)
704+
{
705+
/* These checks mimic kernel checks in dev_valid_name */
706+
if (*name == '\0')
707+
return -1;
708+
if (strlen(name) >= IFNAMSIZ)
709+
return -1;
710+
711+
while (*name) {
712+
if (*name == '/' || isspace(*name))
713+
return -1;
714+
++name;
715+
}
716+
return 0;
717+
}
718+
719+
/* buf is assumed to be IFNAMSIZ */
720+
int get_ifname(char *buf, const char *name)
721+
{
722+
int ret;
723+
724+
ret = check_ifname(name);
725+
if (ret == 0)
726+
strncpy(buf, name, IFNAMSIZ);
727+
728+
return ret;
729+
}
730+
702731
int matches(const char *cmd, const char *pattern)
703732
{
704733
int len = strlen(cmd);

‎misc/arpd.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -664,7 +664,8 @@ int main(int argc, char **argv)
664664
struct ifreq ifr = {};
665665

666666
for (i = 0; i < ifnum; i++) {
667-
strncpy(ifr.ifr_name, ifnames[i], IFNAMSIZ);
667+
if (get_ifname(ifr.ifr_name, ifnames[i]))
668+
invarg("not a valid ifname", ifnames[i]);
668669
if (ioctl(udp_sock, SIOCGIFINDEX, &ifr)) {
669670
perror("ioctl(SIOCGIFINDEX)");
670671
exit(-1);

‎tc/f_flower.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -630,6 +630,8 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
630630
flags |= TCA_CLS_FLAGS_SKIP_SW;
631631
} else if (matches(*argv, "indev") == 0) {
632632
NEXT_ARG();
633+
if (check_ifname(*argv))
634+
invarg("\"indev\" not a valid ifname", *argv);
633635
addattrstrz(n, MAX_MSG, TCA_FLOWER_INDEV, *argv);
634636
} else if (matches(*argv, "vlan_id") == 0) {
635637
__u16 vid;

0 commit comments

Comments
 (0)