Re: [Dnsmasq-discuss] [PATCH] Various fixes detected by static analysis
On 21/08/18 21:22, Petr Mensik wrote: > 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? Looks good on first look. I'll find time to go through it in more detail and apply it soon. > > 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. The only code where an error is not fatal is for dhcp-option, dhcp-host, server and rev-server which get called when files containing just those options are read dynamically. That means parse_dhcp_opt() and code starting at 3025, 2401 and 2493 I suspect there are therefore memory leaks in those which might be worth fixing. For the vast majority of errors in one_opt, they are fatal, and complicating the code massively to avoid warnings from coverity is probably not a good move. Cheers, Simon. > > 1. > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/diff/?id=f236da93df5be85409c24f03683e3d8c54fac72b > > > > ___ > Dnsmasq-discuss mailing list > Dnsmasq-discuss@lists.thekelleys.org.uk > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss > ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] DNS query random ports
On 10/08/18 13:37, Petr Menšík wrote: > Hello, > > we discovered our dnsmasq were using also privileged source ports when > sending queries. Interesting enough, it has right to do it, because it > has to listen also on privileged port. It never drops such privilege. > > It was fixed in commit [1]. But my question is, why is there even custom > generator or random ports, when OS can do it itself? And usually far > better? So I dug a bit into it and came with patch, that would use > random ports from OS by default. > > When I tested it, I got the same results when skipping bind() call on > random ports at all. Is there some reason, why dnsmasq does not follow > OS policy for source outgoing port and choses its own range by itself? > > 1. > http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=baf553db0cdb50707ddab464fb3eff7786ea576c > The random port code was added to dnsmasq in response to the Kaminsky Birthday attack paper, which was in 2009. At that point, there were still people seriously running routers (and therefore dnsmasq) on Linux 2.0 kernels. As best I remember, I did it the way I did because I couldn't be sure that all the platforms dnsmasq would run on would allocate sufficiently random ports: RFC6056 was still more than a year in the future. I'm sure that code could be simplified now. Simon. ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] [PATCH] dhcpv6: fix unaligned access crash on aarch64
Thanks for chasing that down. Your patch will fix it, but I think it's probably better to solve the problem at source, where we copy addresses out of packets, rather than pass unaligned pointers to struct in6_addr around and patch things up at the other end. That stops this coming and biting us again. That makes a much bigger patch, unfortunately. I just pushed my attempt at doing that. Does it work OK for you? Cheers, Simon. On 21/08/18 14:04, Vladislav Grishenko wrote: > Hi, Simon > > > > Faced with dnsmasq crash on aarch64 (32bit userspace, > arm-buildroot-linux-gnueabi-gcc 5.3.0) with dhcpv6 stateful configured. > > Unaligned packet’s req_addr is used in lease6_allocate() for struct copy > since 2.67test15 (commit 89500e31f199e9ae1eadc86213b911ff44d30d6f). > > As for other places, seems well-aligned stack-local vars are used. > > Please refer proposed patch attached and gdb trace: > > > > Program received signal SIGBUS, Bus error. > > 0x0003f990 in lease6_allocate (addrp=0xa55e6, lease_type=32) at lease.c:822 > > 822 lease->addr6 = *addrp; > > (gdb) bt > > #0 0x0003f990 in lease6_allocate (addrp=0xa55e6, lease_type=32) at > lease.c:822 > > #1 0x00055fac in update_leases (state=0xfffef8ac, context=0xa7ee8, > addr=0xa55e6, lease_time=900, now=14609) at rfc3315.c:1825 > > #2 0x00055ab4 in add_address (state=0xfffef8ac, context=0xa7ee8, > lease_time=900, ia_option=0xa55e2, min_time=0xfffef6d0, addr=0xa55e6, > now=14609) at rfc3315.c:1684 > > #3 0x00053764 in dhcp6_no_relay (state=0xfffef8ac, msg_type=3, > inbuff=0xa55a8, sz=128, is_unicast=0, now=14609) at rfc3315.c:938 > > #4 0x00051744 in dhcp6_maybe_relay (state=0xfffef8ac, inbuff=0xa55a8, > sz=128, client_addr=0xfffef9a0, is_unicast=0, now=14609) at rfc3315.c:172 > > #5 0x000513b8 in dhcp6_reply (context=0xa7ee8, interface=23, > iface_name=0xfffef978, fallback=0xfffef9f8, ll_addr=0xfffefa18, > ula_addr=0xfffefa28, sz=128, client_addr=0xfffef9a0, > > now=14609) at rfc3315.c:103 > > #6 0x0004f4b4 in dhcp6_packet (now=14609) at dhcp6.c:233 > > #7 0x00038c78 in main (argc=3, argv=0xfffefd34) at dnsmasq.c:1099 > > > > Best Regards, Vladislav Grishenko > > > ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
[Dnsmasq-discuss] [PATCH] Various fixes detected by static analysis
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?=
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
[Dnsmasq-discuss] [PATCH] dhcpv6: fix unaligned access crash on aarch64
Hi, Simon Faced with dnsmasq crash on aarch64 (32bit userspace, arm-buildroot-linux-gnueabi-gcc 5.3.0) with dhcpv6 stateful configured. Unaligned packet’s req_addr is used in lease6_allocate() for struct copy since 2.67test15 (commit 89500e31f199e9ae1eadc86213b911ff44d30d6f). As for other places, seems well-aligned stack-local vars are used. Please refer proposed patch attached and gdb trace: Program received signal SIGBUS, Bus error. 0x0003f990 in lease6_allocate (addrp=0xa55e6, lease_type=32) at lease.c:822 822 lease->addr6 = *addrp; (gdb) bt #0 0x0003f990 in lease6_allocate (addrp=0xa55e6, lease_type=32) at lease.c:822 #1 0x00055fac in update_leases (state=0xfffef8ac, context=0xa7ee8, addr=0xa55e6, lease_time=900, now=14609) at rfc3315.c:1825 #2 0x00055ab4 in add_address (state=0xfffef8ac, context=0xa7ee8, lease_time=900, ia_option=0xa55e2, min_time=0xfffef6d0, addr=0xa55e6, now=14609) at rfc3315.c:1684 #3 0x00053764 in dhcp6_no_relay (state=0xfffef8ac, msg_type=3, inbuff=0xa55a8, sz=128, is_unicast=0, now=14609) at rfc3315.c:938 #4 0x00051744 in dhcp6_maybe_relay (state=0xfffef8ac, inbuff=0xa55a8, sz=128, client_addr=0xfffef9a0, is_unicast=0, now=14609) at rfc3315.c:172 #5 0x000513b8 in dhcp6_reply (context=0xa7ee8, interface=23, iface_name=0xfffef978, fallback=0xfffef9f8, ll_addr=0xfffefa18, ula_addr=0xfffefa28, sz=128, client_addr=0xfffef9a0, now=14609) at rfc3315.c:103 #6 0x0004f4b4 in dhcp6_packet (now=14609) at dhcp6.c:233 #7 0x00038c78 in main (argc=3, argv=0xfffefd34) at dnsmasq.c:1099 Best Regards, Vladislav Grishenko 0001-dhcpv6-fix-unaligned-access-crash-on-aarch64.patch Description: Binary data ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
