From 73451259daaa84185bd151461252590ba67cdee0 Mon Sep 17 00:00:00 2001 From: Yulia Kartseva Date: Sat, 30 Sep 2017 20:18:40 -0700 Subject: [PATCH 1/5] tc: fix ipv6 filter selector attribute for some prefix lengths Wrong TCA_U32_SEL attribute packing if prefixLen AND 0x1f equals 0x1f. These are /31, /63, /95 and /127 prefix lengths. Example: ip6 dst face:b00f::/31 filter parent b: protocol ipv6 pref 2307 u32 filter parent b: protocol ipv6 pref 2307 u32 fh 800: ht divisor 1 filter parent b: protocol ipv6 pref 2307 u32 fh 800::800 order 2048 key ht 800 bkt 0 match faceb00f/ffffffff at 24 v2: previous patch was made with a wrong repo Signed-off-by: Yulia Kartseva --- tc/f_u32.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tc/f_u32.c b/tc/f_u32.c index 5815be9c..14b95889 100644 --- a/tc/f_u32.c +++ b/tc/f_u32.c @@ -385,8 +385,7 @@ static int parse_ip6_addr(int *argc_p, char ***argv_p, plen = addr.bitlen; for (i = 0; i < plen; i += 32) { - /* if (((i + 31) & ~0x1F) <= plen) { */ - if (i + 31 <= plen) { + if (i + 31 < plen) { res = pack_key(sel, addr.data[i / 32], 0xFFFFFFFF, off + 4 * (i / 32), offmask); if (res < 0) From 4c0939a29e2c1739f0141c87ecd7940825734a22 Mon Sep 17 00:00:00 2001 From: Michal Kubecek Date: Fri, 29 Sep 2017 13:41:05 +0200 Subject: [PATCH 2/5] ip xfrm: use correct key length for netlink message When SA is added manually using "ip xfrm state add", xfrm_state_modify() uses alg_key_len field of struct xfrm_algo for the length of key passed to kernel in the netlink message. However alg_key_len is bit length of the key while we need byte length here. This is usually harmless as kernel ignores the excess data but when the bit length of the key exceeds 512 (XFRM_ALGO_KEY_BUF_SIZE), it can result in buffer overflow. We can simply divide by 8 here as the only place setting alg_key_len is in xfrm_algo_parse() where it is always set to a multiple of 8 (and there are already multiple places using "algo->alg_key_len / 8"). Signed-off-by: Michal Kubecek --- ip/xfrm_state.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ip/xfrm_state.c b/ip/xfrm_state.c index 4483fb8f..99fdec23 100644 --- a/ip/xfrm_state.c +++ b/ip/xfrm_state.c @@ -539,7 +539,7 @@ static int xfrm_state_modify(int cmd, unsigned int flags, int argc, char **argv) xfrm_algo_parse((void *)&alg, type, name, key, buf, sizeof(alg.buf)); - len += alg.u.alg.alg_key_len; + len += alg.u.alg.alg_key_len / 8; addattr_l(&req.n, sizeof(req.buf), type, (void *)&alg, len); From 26111ab1dba820421ccaf283ac097a79b95023a2 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 2 Oct 2017 13:46:35 +0200 Subject: [PATCH 3/5] ip{6, }tunnel: Avoid copying user-supplied interface name around 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 --- 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 b4a7def1..c12d700e 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 105d0f55..0acfd079 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); } From ee474849c85116ec36e387882447f737ac3fdefb Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 2 Oct 2017 13:46:36 +0200 Subject: [PATCH 4/5] tc: flower: No need to cache indev arg Since addattrstrz() will copy the provided string into the attribute payload, there is no need to cache the data. Signed-off-by: Phil Sutter --- 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 934832e2..99e62a38 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; From 625df645b703dc858d54784c35beff64464afae2 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 2 Oct 2017 13:46:37 +0200 Subject: [PATCH 5/5] 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 --- include/utils.h | 2 ++ ip/ip6tunnel.c | 3 ++- ip/ipl2tp.c | 4 +++- ip/iplink.c | 31 ++++++++++++------------------- ip/ipmaddr.c | 3 ++- ip/iprule.c | 10 ++++++++-- ip/iptunnel.c | 7 ++++++- ip/iptuntap.c | 6 ++++-- lib/utils.c | 29 +++++++++++++++++++++++++++++ misc/arpd.c | 3 ++- tc/f_flower.c | 2 ++ 11 files changed, 72 insertions(+), 28 deletions(-) diff --git a/include/utils.h b/include/utils.h index c9ed230b..76addb32 100644 --- a/include/utils.h +++ b/include/utils.h @@ -133,6 +133,8 @@ 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)); +int check_ifname(const char *); +int get_ifname(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 c12d700e..bc44bef7 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); + if (get_ifname(p->name, *argv)) + invarg("\"name\" not a valid ifname", *argv); if (cmd == SIOCCHGTUNNEL && count == 0) { struct ip6_tnl_parm2 old_p = {}; diff --git a/ip/ipl2tp.c b/ip/ipl2tp.c index 88664c90..1e37b175 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,8 @@ static int parse_args(int argc, char **argv, int cmd, struct l2tp_parm *p) } } else if (strcmp(*argv, "name") == 0) { NEXT_ARG(); + if (check_ifname(*argv)) + invarg("\"name\" not a valid ifname", *argv); p->ifname = *argv; } else if (strcmp(*argv, "remote") == 0) { NEXT_ARG(); diff --git a/ip/iplink.c b/ip/iplink.c index ff5b56c0..6a96ea9f 100644 --- a/ip/iplink.c +++ b/ip/iplink.c @@ -573,6 +573,8 @@ 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(); + if (check_ifname(*argv)) + invarg("\"name\" not a valid ifname", *argv); *name = *argv; } else if (strcmp(*argv, "index") == 0) { NEXT_ARG(); @@ -848,6 +850,8 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, NEXT_ARG(); if (*dev) duparg2("dev", *argv); + if (check_ifname(*argv)) + invarg("\"dev\" not a valid ifname", *argv); *dev = *argv; dev_index = ll_name_to_index(*dev); } @@ -870,7 +874,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 +963,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 +1014,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 +1026,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 +1257,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("\"name\" not a valid ifname", *argv); newname = *argv; } else if (matches(*argv, "address") == 0) { NEXT_ARG(); @@ -1355,6 +1349,8 @@ static int do_set(int argc, char **argv) if (dev) duparg2("dev", *argv); + if (check_ifname(*argv)) + invarg("\"dev\" not a valid ifname", *argv); dev = *argv; } argc--; argv++; @@ -1383,9 +1379,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 85a69e77..5683f6fa 100644 --- a/ip/ipmaddr.c +++ b/ip/ipmaddr.c @@ -284,7 +284,8 @@ static int multiaddr_modify(int cmd, int argc, char **argv) NEXT_ARG(); if (ifr.ifr_name[0]) duparg("dev", *argv); - strncpy(ifr.ifr_name, *argv, IFNAMSIZ); + if (get_ifname(ifr.ifr_name, *argv)) + invarg("\"dev\" not a valid ifname", *argv); } else { if (matches(*argv, "address") == 0) { NEXT_ARG(); diff --git a/ip/iprule.c b/ip/iprule.c index 8313138d..36c57fa7 100644 --- a/ip/iprule.c +++ b/ip/iprule.c @@ -472,11 +472,13 @@ 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(); - strncpy(filter.iif, *argv, IFNAMSIZ); + if (get_ifname(filter.iif, *argv)) + invarg("\"iif\"/\"dev\" not a valid ifname", *argv); filter.iifmask = 1; } else if (strcmp(*argv, "oif") == 0) { NEXT_ARG(); - strncpy(filter.oif, *argv, IFNAMSIZ); + if (get_ifname(filter.oif, *argv)) + invarg("\"oif\" not a valid ifname", *argv); filter.oifmask = 1; } else if (strcmp(*argv, "l3mdev") == 0) { filter.l3mdev = 1; @@ -695,10 +697,14 @@ static int iprule_modify(int cmd, int argc, char **argv) } else if (strcmp(*argv, "dev") == 0 || strcmp(*argv, "iif") == 0) { NEXT_ARG(); + if (check_ifname(*argv)) + invarg("\"iif\"/\"dev\" not a valid ifname", *argv); addattr_l(&req.n, sizeof(req), FRA_IFNAME, *argv, strlen(*argv)+1); } else if (strcmp(*argv, "oif") == 0) { NEXT_ARG(); + if (check_ifname(*argv)) + invarg("\"oif\" not a valid ifname", *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 0acfd079..208a1f06 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); + if (get_ifname(p->name, *argv)) + invarg("\"name\" not a valid ifname", *argv); if (cmd == SIOCCHGTUNNEL && count == 0) { struct ip_tunnel_parm old_p = {}; @@ -487,6 +488,8 @@ static int do_prl(int argc, char **argv) count++; } else if (strcmp(*argv, "dev") == 0) { NEXT_ARG(); + if (check_ifname(*argv)) + invarg("\"dev\" not a valid ifname", *argv); medium = *argv; } else { fprintf(stderr, @@ -534,6 +537,8 @@ static int do_6rd(int argc, char **argv) cmd = SIOCDEL6RD; } else if (strcmp(*argv, "dev") == 0) { NEXT_ARG(); + if (check_ifname(*argv)) + invarg("\"dev\" not a valid ifname", *argv); medium = *argv; } else { fprintf(stderr, diff --git a/ip/iptuntap.c b/ip/iptuntap.c index 451f7f0e..b46e452f 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); + if (get_ifname(ifr->ifr_name, *argv)) + invarg("\"dev\" not a valid ifname", *argv); } else { if (matches(*argv, "name") == 0) { NEXT_ARG(); @@ -184,7 +185,8 @@ static int parse_args(int argc, char **argv, usage(); if (ifr->ifr_name[0]) duparg2("name", *argv); - strncpy(ifr->ifr_name, *argv, IFNAMSIZ); + if (get_ifname(ifr->ifr_name, *argv)) + invarg("\"name\" not a valid ifname", *argv); } count++; argc--; argv++; diff --git a/lib/utils.c b/lib/utils.c index bbd3cbc4..0cf99619 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -699,6 +700,34 @@ void duparg2(const char *key, const char *arg) exit(-1); } +int check_ifname(const char *name) +{ + /* These checks 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); + + return ret; +} + int matches(const char *cmd, const char *pattern) { int len = strlen(cmd); diff --git a/misc/arpd.c b/misc/arpd.c index bfab4454..c2666f76 100644 --- a/misc/arpd.c +++ b/misc/arpd.c @@ -664,7 +664,8 @@ int main(int argc, char **argv) struct ifreq ifr = {}; for (i = 0; i < ifnum; i++) { - strncpy(ifr.ifr_name, ifnames[i], IFNAMSIZ); + if (get_ifname(ifr.ifr_name, ifnames[i])) + invarg("not a valid ifname", ifnames[i]); if (ioctl(udp_sock, SIOCGIFINDEX, &ifr)) { perror("ioctl(SIOCGIFINDEX)"); exit(-1); diff --git a/tc/f_flower.c b/tc/f_flower.c index 99e62a38..b1802107 100644 --- a/tc/f_flower.c +++ b/tc/f_flower.c @@ -630,6 +630,8 @@ 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(); + if (check_ifname(*argv)) + invarg("\"indev\" not a valid ifname", *argv); addattrstrz(n, MAX_MSG, TCA_FLOWER_INDEV, *argv); } else if (matches(*argv, "vlan_id") == 0) { __u16 vid;