Hi Simon and all others, I have tried running dnsmasq under coverity, static analysis tool. It found some warnings. I have fixed some things. Most obvious error was inconsistent handling of buffer length of interface names. Buffer size is IFNAMSIZ long, that is 16 bytes. But if interface should have terminating zero, max. useable length is 15. Sometimes, buffer size is 16+1, sometimes only 16. Sometimes name might be sent to the kernel unterminated. According to [1] it cannot be longer in Linux.
I have created shared simple function that will always terminate string. And few little improvements around. What do you think? It complains a lot about returns from one_opt in option.c. I can make patch that will deallocate memory before returning error. In some cases, i think option parsing does not have to be fatal. Would you accept fixes that will free memory before return? I think in some cases option parsing can be used for files that can be reread multiple times when running. 1. https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/diff/?id=f236da93df5be85409c24f03683e3d8c54fac72b -- Petr Menšík Software Engineer Red Hat, http://www.redhat.com/ email: pemen...@redhat.com PGP: 65C6C973
From a6c3fe2f00468b7e135d5c51df1b8391c2f28733 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= <pemen...@redhat.com> Date: Wed, 15 Aug 2018 18:17:00 +0200 Subject: [PATCH 1/3] Fix lengths of interface names Use helper function similar to copy correctly limited names into buffers. --- src/bpf.c | 2 +- src/dhcp-common.c | 5 ++++- src/dhcp.c | 9 ++++----- src/dnsmasq.h | 1 + src/log.c | 2 +- src/netlink.c | 5 +++++ src/network.c | 12 ++++++------ src/option.c | 9 ++++----- src/rfc2131.c | 10 +++++----- src/tftp.c | 2 +- src/util.c | 13 ++++++++++++- 11 files changed, 44 insertions(+), 26 deletions(-) diff --git a/src/bpf.c b/src/bpf.c index 49a11bf..ff66d6d 100644 --- a/src/bpf.c +++ b/src/bpf.c @@ -169,7 +169,7 @@ int iface_enumerate(int family, void *parm, int (*callback)()) struct in6_ifreq ifr6; memset(&ifr6, 0, sizeof(ifr6)); - strncpy(ifr6.ifr_name, addrs->ifa_name, sizeof(ifr6.ifr_name)); + safe_strncpy(ifr6.ifr_name, addrs->ifa_name, sizeof(ifr6.ifr_name)); ifr6.ifr_addr = *((struct sockaddr_in6 *) addrs->ifa_addr); if (fd != -1 && ioctl(fd, SIOCGIFAFLAG_IN6, &ifr6) != -1) diff --git a/src/dhcp-common.c b/src/dhcp-common.c index 8ff0f0d..78c1d9b 100644 --- a/src/dhcp-common.c +++ b/src/dhcp-common.c @@ -485,8 +485,11 @@ char *whichdevice(void) void bindtodevice(char *device, int fd) { + size_t len = strlen(device)+1; + if (len > IFNAMSIZ) + len = IFNAMSIZ; /* only allowed by root. */ - if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE, device, IFNAMSIZ) == -1 && + if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE, device, len) == -1 && errno != EPERM) die(_("failed to set SO_BINDTODEVICE on DHCP socket: %s"), NULL, EC_BADNET); } diff --git a/src/dhcp.c b/src/dhcp.c index 4d29525..f8d323b 100644 --- a/src/dhcp.c +++ b/src/dhcp.c @@ -232,7 +232,7 @@ void dhcp_packet(time_t now, int pxe_fd) #ifdef HAVE_LINUX_NETWORK /* ARP fiddling uses original interface even if we pretend to use a different one. */ - strncpy(arp_req.arp_dev, ifr.ifr_name, 16); + safe_strncpy(arp_req.arp_dev, ifr.ifr_name, sizeof(arp_req.arp_dev)); #endif /* If the interface on which the DHCP request was received is an @@ -255,7 +255,7 @@ void dhcp_packet(time_t now, int pxe_fd) } else { - strncpy(ifr.ifr_name, bridge->iface, IF_NAMESIZE); + safe_strncpy(ifr.ifr_name, bridge->iface, sizeof(ifr.ifr_name)); break; } } @@ -279,7 +279,7 @@ void dhcp_packet(time_t now, int pxe_fd) is_relay_reply = 1; iov.iov_len = sz; #ifdef HAVE_LINUX_NETWORK - strncpy(arp_req.arp_dev, ifr.ifr_name, 16); + safe_strncpy(arp_req.arp_dev, ifr.ifr_name, sizeof(arp_req.arp_dev)); #endif } else @@ -988,8 +988,7 @@ char *host_from_dns(struct in_addr addr) if (!legal_hostname(hostname)) return NULL; - strncpy(daemon->dhcp_buff, hostname, 256); - daemon->dhcp_buff[255] = 0; + safe_strncpy(daemon->dhcp_buff, hostname, 256); strip_hostname(daemon->dhcp_buff); return daemon->dhcp_buff; diff --git a/src/dnsmasq.h b/src/dnsmasq.h index 4cf12bf..b260f8a 100644 --- a/src/dnsmasq.h +++ b/src/dnsmasq.h @@ -1237,6 +1237,7 @@ int legal_hostname(char *name); char *canonicalise(char *in, int *nomem); unsigned char *do_rfc1035_name(unsigned char *p, char *sval, char *limit); void *safe_malloc(size_t size); +void safe_strncpy(char *dest, const char *src, size_t size); void safe_pipe(int *fd, int read_noblock); void *whine_malloc(size_t size); int sa_len(union mysockaddr *addr); diff --git a/src/log.c b/src/log.c index dae8a75..d0d4780 100644 --- a/src/log.c +++ b/src/log.c @@ -232,7 +232,7 @@ static void log_write(void) logaddr.sun_len = sizeof(logaddr) - sizeof(logaddr.sun_path) + strlen(_PATH_LOG) + 1; #endif logaddr.sun_family = AF_UNIX; - strncpy(logaddr.sun_path, _PATH_LOG, sizeof(logaddr.sun_path)); + safe_strncpy(logaddr.sun_path, _PATH_LOG, sizeof(logaddr.sun_path)); /* Got connection back? try again. */ if (connect(log_fd, (struct sockaddr *)&logaddr, sizeof(logaddr)) != -1) diff --git a/src/netlink.c b/src/netlink.c index 05153f5..849c2ff 100644 --- a/src/netlink.c +++ b/src/netlink.c @@ -149,10 +149,15 @@ int iface_enumerate(int family, void *parm, int (*callback)()) struct rtgenmsg g; } req; + memset(&req, 0, sizeof(req)); + memset(&addr, 0, sizeof(addr)); + addr.nl_family = AF_NETLINK; +#if 0 addr.nl_pad = 0; addr.nl_groups = 0; addr.nl_pid = 0; /* address to kernel */ +#endif again: if (family == AF_UNSPEC) diff --git a/src/network.c b/src/network.c index 71f5186..df8233f 100644 --- a/src/network.c +++ b/src/network.c @@ -29,7 +29,7 @@ int indextoname(int fd, int index, char *name) if (ioctl(fd, SIOCGIFNAME, &ifr) == -1) return 0; - strncpy(name, ifr.ifr_name, IF_NAMESIZE); + safe_strncpy(name, ifr.ifr_name, IF_NAMESIZE); return 1; } @@ -82,12 +82,12 @@ int indextoname(int fd, int index, char *name) for (i = lifc.lifc_len / sizeof(struct lifreq); i; i--, lifrp++) { struct lifreq lifr; - strncpy(lifr.lifr_name, lifrp->lifr_name, IF_NAMESIZE); + safe_strncpy(lifr.lifr_name, lifrp->lifr_name, IF_NAMESIZE); if (ioctl(fd, SIOCGLIFINDEX, &lifr) < 0) return 0; if (lifr.lifr_index == index) { - strncpy(name, lifr.lifr_name, IF_NAMESIZE); + safe_strncpy(name, lifr.lifr_name, IF_NAMESIZE); return 1; } } @@ -188,7 +188,7 @@ int loopback_exception(int fd, int family, struct all_addr *addr, char *name) struct ifreq ifr; struct irec *iface; - strncpy(ifr.ifr_name, name, IF_NAMESIZE); + safe_strncpy(ifr.ifr_name, name, IF_NAMESIZE); if (ioctl(fd, SIOCGIFFLAGS, &ifr) != -1 && ifr.ifr_flags & IFF_LOOPBACK) { @@ -1293,7 +1293,7 @@ static struct serverfd *allocate_sfd(union mysockaddr *addr, char *intname) return NULL; } - strcpy(sfd->interface, intname); + safe_strncpy(sfd->interface, intname, sizeof(sfd->interface)); sfd->source_addr = *addr; sfd->next = daemon->sfds; sfd->ifindex = ifindex; @@ -1465,7 +1465,7 @@ void add_update_server(int flags, serv->flags |= SERV_HAS_DOMAIN; if (interface) - strcpy(serv->interface, interface); + safe_strncpy(serv->interface, interface, sizeof(serv->interface)); if (addr) serv->addr = *addr; if (source_addr) diff --git a/src/option.c b/src/option.c index f77f5aa..69e7811 100644 --- a/src/option.c +++ b/src/option.c @@ -804,7 +804,7 @@ char *parse_server(char *arg, union mysockaddr *addr, union mysockaddr *source_a if (interface_opt) { #if defined(SO_BINDTODEVICE) - strncpy(interface, interface_opt, IF_NAMESIZE - 1); + safe_strncpy(interface, interface_opt, IF_NAMESIZE); #else return _("interface binding not supported"); #endif @@ -833,7 +833,7 @@ char *parse_server(char *arg, union mysockaddr *addr, union mysockaddr *source_a return _("interface can only be specified once"); source_addr->in.sin_addr.s_addr = INADDR_ANY; - strncpy(interface, source, IF_NAMESIZE - 1); + safe_strncpy(interface, source, IF_NAMESIZE); #else return _("interface binding not supported"); #endif @@ -868,7 +868,7 @@ char *parse_server(char *arg, union mysockaddr *addr, union mysockaddr *source_a return _("interface can only be specified once"); source_addr->in6.sin6_addr = in6addr_any; - strncpy(interface, source, IF_NAMESIZE - 1); + safe_strncpy(interface, source, IF_NAMESIZE); #else return _("interface binding not supported"); #endif @@ -4747,8 +4747,7 @@ void read_opts(int argc, char **argv, char *compile_opts) argbuf_size = strlen(optarg) + 1; argbuf = opt_malloc(argbuf_size); } - strncpy(argbuf, optarg, argbuf_size); - argbuf[argbuf_size-1] = 0; + safe_strncpy(argbuf, optarg, argbuf_size); arg = argbuf; } else diff --git a/src/rfc2131.c b/src/rfc2131.c index f68e702..2f20c32 100644 --- a/src/rfc2131.c +++ b/src/rfc2131.c @@ -918,7 +918,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index, mess->siaddr = a_record_from_hosts(boot->tftp_sname, now); if (boot->file) - strncpy((char *)mess->file, boot->file, sizeof(mess->file)-1); + safe_strncpy((char *)mess->file, boot->file, sizeof(mess->file)); } option_put(mess, end, OPTION_MESSAGE_TYPE, 1, @@ -2329,7 +2329,7 @@ static void do_options(struct dhcp_context *context, in_list(req_options, OPTION_SNAME)) option_put_string(mess, end, OPTION_SNAME, boot->sname, 1); else - strncpy((char *)mess->sname, boot->sname, sizeof(mess->sname)-1); + safe_strncpy((char *)mess->sname, boot->sname, sizeof(mess->sname)); } if (boot->file) @@ -2339,7 +2339,7 @@ static void do_options(struct dhcp_context *context, in_list(req_options, OPTION_FILENAME)) option_put_string(mess, end, OPTION_FILENAME, boot->file, 1); else - strncpy((char *)mess->file, boot->file, sizeof(mess->file)-1); + safe_strncpy((char *)mess->file, boot->file, sizeof(mess->file)); } if (boot->next_server.s_addr) @@ -2356,14 +2356,14 @@ static void do_options(struct dhcp_context *context, if ((!req_options || !in_list(req_options, OPTION_FILENAME)) && (opt = option_find2(OPTION_FILENAME)) && !(opt->flags & DHOPT_FORCE)) { - strncpy((char *)mess->file, (char *)opt->val, sizeof(mess->file)-1); + safe_strncpy((char *)mess->file, (char *)opt->val, sizeof(mess->file)); done_file = 1; } if ((!req_options || !in_list(req_options, OPTION_SNAME)) && (opt = option_find2(OPTION_SNAME)) && !(opt->flags & DHOPT_FORCE)) { - strncpy((char *)mess->sname, (char *)opt->val, sizeof(mess->sname)-1); + safe_strncpy((char *)mess->sname, (char *)opt->val, sizeof(mess->sname)); done_server = 1; } diff --git a/src/tftp.c b/src/tftp.c index bccca69..f2eccbc 100644 --- a/src/tftp.c +++ b/src/tftp.c @@ -234,7 +234,7 @@ void tftp_request(struct listener *listen, time_t now) #endif } - strncpy(ifr.ifr_name, name, IF_NAMESIZE); + safe_strncpy(ifr.ifr_name, name, IF_NAMESIZE); if (ioctl(listen->tftpfd, SIOCGIFMTU, &ifr) != -1) { mtu = ifr.ifr_mtu; diff --git a/src/util.c b/src/util.c index 532bc16..3ed1a63 100644 --- a/src/util.c +++ b/src/util.c @@ -281,7 +281,18 @@ void *safe_malloc(size_t size) die(_("could not get memory"), NULL, EC_NOMEM); return ret; -} +} + +/* Ensure limited size string is always terminated. + * Can be replaced by (void)strlcpy() on some platforms */ +void safe_strncpy(char *dest, const char *src, size_t size) +{ + if (size) + { + dest[size-1] = '\0'; + strncpy(dest, src, size-1); + } +} void safe_pipe(int *fd, int read_noblock) { -- 2.14.4
From 63f169310b973e37f142e0c45c2badf39a90a109 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= <pemen...@redhat.com> Date: Wed, 15 Aug 2018 19:41:07 +0200 Subject: [PATCH 2/3] Mark die function as never returning Improves static analysis output and reduces false positives. --- src/dnsmasq.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/dnsmasq.h b/src/dnsmasq.h index b260f8a..dedc412 100644 --- a/src/dnsmasq.h +++ b/src/dnsmasq.h @@ -42,6 +42,12 @@ # define __EXTENSIONS__ #endif +#if (defined(__GNUC__) && __GNUC__ >= 3) || defined(__clang__) +#define ATTRIBUTE_NORETURN __attribute__ ((noreturn)) +#else +#define ATTRIBUTE_NORETURN +#endif + /* get these before config.h for IPv6 stuff... */ #include <sys/types.h> #include <sys/socket.h> @@ -1266,7 +1272,7 @@ int wildcard_match(const char* wildcard, const char* match); int wildcard_matchn(const char* wildcard, const char* match, int num); /* log.c */ -void die(char *message, char *arg1, int exit_code); +void die(char *message, char *arg1, int exit_code) ATTRIBUTE_NORETURN; int log_start(struct passwd *ent_pw, int errfd); int log_reopen(char *log_file); -- 2.14.4
From 6f647a71b768dd2cfe9ecfb20266ffa8e4f6a1bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= <pemen...@redhat.com> Date: Fri, 17 Aug 2018 10:20:05 +0200 Subject: [PATCH 3/3] Minor improvements in lease-tools Limit max interface name to fit into buffer. Make sure pointer have to be always positive. Close socket after received reply. --- contrib/lease-tools/dhcp_lease_time.c | 2 +- contrib/lease-tools/dhcp_release.c | 3 ++- contrib/lease-tools/dhcp_release6.c | 5 ++++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/contrib/lease-tools/dhcp_lease_time.c b/contrib/lease-tools/dhcp_lease_time.c index f9d7a85..697d627 100644 --- a/contrib/lease-tools/dhcp_lease_time.c +++ b/contrib/lease-tools/dhcp_lease_time.c @@ -83,7 +83,7 @@ static unsigned char *option_find1(unsigned char *p, unsigned char *end, int opt if (p >= end - 2) return NULL; /* malformed packet */ opt_len = option_len(p); - if (p >= end - (2 + opt_len)) + if (end - p >= (2 + opt_len)) return NULL; /* malformed packet */ if (*p == opt && opt_len >= minsize) return p; diff --git a/contrib/lease-tools/dhcp_release.c b/contrib/lease-tools/dhcp_release.c index 201fcd3..59883d4 100644 --- a/contrib/lease-tools/dhcp_release.c +++ b/contrib/lease-tools/dhcp_release.c @@ -270,7 +270,8 @@ int main(int argc, char **argv) /* This voodoo fakes up a packet coming from the correct interface, which really matters for a DHCP server */ - strcpy(ifr.ifr_name, argv[1]); + strncpy(ifr.ifr_name, argv[1], sizeof(ifr.ifr_name)-1); + ifr.ifr_name[sizeof(ifr.ifr_name)-1] = '\0'; if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE, &ifr, sizeof(ifr)) == -1) { perror("cannot setup interface"); diff --git a/contrib/lease-tools/dhcp_release6.c b/contrib/lease-tools/dhcp_release6.c index 7f79fa7..d680222 100644 --- a/contrib/lease-tools/dhcp_release6.c +++ b/contrib/lease-tools/dhcp_release6.c @@ -376,9 +376,12 @@ int send_release_packet(const char* iface, struct dhcp6_packet* packet) sleep(1); continue; } + + close(sock); return result; } - + + close(sock); fprintf(stderr, "Response timed out\n"); return -1; } -- 2.14.4
_______________________________________________ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss