From 2b623e991115358a57275af8a53feb5ae707b3ae Mon Sep 17 00:00:00 2001 From: Vitezslav Samel Date: Thu, 21 Nov 2024 08:43:57 +0100 Subject: [PATCH] interface names: limit length to IFNAMSIZ This fixes CVE-2024-52949 (stack based buffer overflow) when copying user supplied interface name without any check. Problem was reported by Massimiliano Ferraresi and Massimiliano Brolli from TIM Red team (https://www.gruppotim.it/it/footer/red-team.html) Reported-by: Massimiliano Ferraresi, Massimiliano Brolli Signed-off-by: Vitezslav Samel --- src/ifaces.c | 16 ++++++++-------- src/ifstats.c | 6 +++--- src/iptraf-ng-compat.h | 1 + src/iptraf.c | 9 +++++++++ src/othptab.c | 2 +- src/promisc.c | 2 +- src/tcptable.c | 4 ++-- src/wrapper.c | 8 ++++++++ 8 files changed, 33 insertions(+), 15 deletions(-) diff --git a/src/ifaces.c b/src/ifaces.c index aeb1614..4c5a545 100644 --- a/src/ifaces.c +++ b/src/ifaces.c @@ -67,7 +67,7 @@ int dev_up(char *iface) fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP); - strcpy(ifr.ifr_name, iface); + ifname_copy(ifr.ifr_name, iface); ir = ioctl(fd, SIOCGIFFLAGS, &ifr); close(fd); @@ -90,7 +90,7 @@ int dev_get_ifindex(const char *iface) return fd; struct ifreq ifr; - strcpy(ifr.ifr_name, iface); + ifname_copy(ifr.ifr_name, iface); int ir = ioctl(fd, SIOCGIFINDEX, &ifr); /* need to preserve errno across call to close() */ @@ -114,7 +114,7 @@ int dev_get_mtu(const char *iface) return fd; struct ifreq ifr; - strcpy(ifr.ifr_name, iface); + ifname_copy(ifr.ifr_name, iface); int ir = ioctl(fd, SIOCGIFMTU, &ifr); /* need to preserve errno across call to close() */ @@ -138,7 +138,7 @@ int dev_get_flags(const char *iface) return fd; struct ifreq ifr; - strcpy(ifr.ifr_name, iface); + ifname_copy(ifr.ifr_name, iface); int ir = ioctl(fd, SIOCGIFFLAGS, &ifr); /* need to preserve errno across call to close() */ @@ -162,7 +162,7 @@ int dev_set_flags(const char *iface, int flags) return fd; struct ifreq ifr; - strcpy(ifr.ifr_name, iface); + ifname_copy(ifr.ifr_name, iface); int ir = ioctl(fd, SIOCGIFFLAGS, &ifr); if (ir == -1) goto err; @@ -190,7 +190,7 @@ int dev_clear_flags(const char *iface, int flags) return fd; struct ifreq ifr; - strcpy(ifr.ifr_name, iface); + ifname_copy(ifr.ifr_name, iface); int ir = ioctl(fd, SIOCGIFFLAGS, &ifr); if (ir == -1) goto err; @@ -233,7 +233,7 @@ int dev_get_ifname(int ifindex, char *ifname) return ir; } - strncpy(ifname, ifr.ifr_name, IFNAMSIZ); + ifname_copy(ifname, ifr.ifr_name); return ir; } @@ -256,7 +256,7 @@ int dev_bind_ifname(int fd, const char * const ifname) int ir; struct ifreq ifr; - strcpy(ifr.ifr_name, ifname); + ifname_copy(ifr.ifr_name, ifname); ir = ioctl(fd, SIOCGIFINDEX, &ifr); if (ir) return ir; diff --git a/src/ifstats.c b/src/ifstats.c index 00a2a3f..1b687b6 100644 --- a/src/ifstats.c +++ b/src/ifstats.c @@ -194,7 +194,7 @@ static void initiflist(struct iflist **list) struct iflist *itmp = alloc_iflist_entry(); itmp->ifindex = ifindex; - strcpy(itmp->ifname, ifname); + ifname_copy(itmp->ifname, ifname); /* make the linked list sorted by ifindex */ struct iflist *cur = *list, *last = NULL; @@ -714,9 +714,9 @@ void selectiface(char *ifname, int withall, int *aborted) if (!(*aborted) && (list != NULL)) { ptmp = (struct iflist *) scrolllist.textptr->nodeptr; if ((withall) && (ptmp->prev_entry == NULL)) /* All Interfaces */ - strcpy(ifname, ""); + ifname_copy(ifname, ""); else - strcpy(ifname, ptmp->ifname); + ifname_copy(ifname, ptmp->ifname); } tx_destroy_list(&scrolllist); diff --git a/src/iptraf-ng-compat.h b/src/iptraf-ng-compat.h index 5aec185..845f18b 100644 --- a/src/iptraf-ng-compat.h +++ b/src/iptraf-ng-compat.h @@ -112,6 +112,7 @@ extern void *xmallocz(size_t size); extern char *xstrdup(const char *s); extern int strtoul_ui(char const *s, int base, unsigned int *result); extern int strtol_i(char const *s, int base, int *result); +extern void ifname_copy(char *dst, const char *src); extern void die(const char *err, ...) __noreturn __printf(1,2); extern void die_errno(const char *fmt, ...) __noreturn __printf(1,2); diff --git a/src/iptraf.c b/src/iptraf.c index 95f8e53..e5dcb64 100644 --- a/src/iptraf.c +++ b/src/iptraf.c @@ -388,6 +388,15 @@ int main(int argc, char **argv) if (__builtin_popcount(command) > 1) die("only one of -i|-d|-s|-z|-l|-g options must be used"); + /* sanity check of passed arguments */ + if ((i_opt && strlen(i_opt) >= IFNAMSIZ) || + (d_opt && strlen(d_opt) >= IFNAMSIZ) || + (s_opt && strlen(s_opt) >= IFNAMSIZ) || + (z_opt && strlen(z_opt) >= IFNAMSIZ) || + (l_opt && strlen(l_opt) >= IFNAMSIZ)) { + die("interface name is too long"); + } + strcpy(current_logfile, ""); if (f_opt) { diff --git a/src/othptab.c b/src/othptab.c index d1d9658..80f3dc8 100644 --- a/src/othptab.c +++ b/src/othptab.c @@ -271,7 +271,7 @@ struct othptabent *add_othp_entry(struct othptable *table, struct pkt_hdr *pkt, } new_entry->protocol = protocol; - strcpy(new_entry->iface, ifname); + ifname_copy(new_entry->iface, ifname); new_entry->pkt_length = pkt->pkt_len; diff --git a/src/promisc.c b/src/promisc.c index d94e8bb..4737962 100644 --- a/src/promisc.c +++ b/src/promisc.c @@ -70,7 +70,7 @@ static void promisc_enable_dev(struct list_head *promisc, int sock, const char * struct promisc_list *new = xmallocz(sizeof(*new)); new->ifindex = ifindex; - strcpy(new->ifname, dev); + ifname_copy(new->ifname, dev); list_add_tail(&new->list, promisc); } diff --git a/src/tcptable.c b/src/tcptable.c index 159d628..2c4efc1 100644 --- a/src/tcptable.c +++ b/src/tcptable.c @@ -365,8 +365,8 @@ struct tcptableent *addentry(struct tcptable *table, * Store interface name */ - strcpy(new_entry->ifname, ifname); - strcpy(new_entry->oth_connection->ifname, ifname); + ifname_copy(new_entry->ifname, ifname); + ifname_copy(new_entry->oth_connection->ifname, ifname); /* * Zero out MAC address fields diff --git a/src/wrapper.c b/src/wrapper.c index 2eb3b59..1d2dc6f 100644 --- a/src/wrapper.c +++ b/src/wrapper.c @@ -78,3 +78,11 @@ int strtol_i(char const *s, int base, int *result) *result = ul; return 0; } + +/* it's up to the caller to ensure there is room for */ +/* at least IFNAMSIZ bytes in dst */ +void ifname_copy(char *dst, const char *src) +{ + strncpy(dst, src, IFNAMSIZ - 1); + dst[IFNAMSIZ - 1] = '\0'; +} -- 2.33.0