<div dir="ltr"><div dir="ltr">Hi Simon, hi list,<br><br>While reviewing the option parser with a sanitizer-instrumented build, I<br>found that --add-subnet and --auth-zone accept prefix lengths far outside<br>their documented ranges (the man page allows 0-32 for IPv4 and 0-128 for<br>IPv6, and 0 is explicitly valid) and silently store them. The values are<br>later used as memcpy()/memcmp() lengths on fixed-size stack objects, so a<br>single DNS query from any client crashes the daemon. Both issues reproduce<br>on the v2.93 release and on current master (a9880c59).<br><br>Since these values cannot occur in a sane configuration, this is an<br>input-validation/hardening issue rather than a classic remote hole - but<br>dnsmasq configs are often generated by management tools, so refusing the<br>values at start-up would be the right fix. Details and suggested fixes<br>below; happy to test any proposed patch.<br><br>Affected versions: --add-subnet since 2.67 (when it was added);<br>--auth-zone since 2.68 (the same release entry reads "Remove restriction<br>on prefix-length in --auth-zone", so prefixes have been unbounded since<br>then). Verified on tag v2.93 (3ff66da) and master<br>a9880c595f052d63859d6ed8aa86a3a6b007bf20; all line numbers below refer to<br>master.<br><br><br>1. --add-subnet: stack-buffer-overflow (WRITE), CWE-787<br>-------------------------------------------------------<br><br>Parse side (option.c, LOPT_ADD_SBNET, lines 2248-2293; the atoi_check()<br>calls at 2262/2266/2280 only reject non-digit strings, no range check):<br><br> if ((end = split_chr(arg, '/')))<br> { parse_mysockaddr(arg, &new->addr);<br> if (!atoi_check(end, &new->mask)) ... } /* 2262: addr/mask form */<br> else if (!atoi_check(arg, &new->mask)) ... /* 2266: bare number form */<br> daemon->add_subnet4 = new; /* first field = v4 entry */<br> ... atoi_check(end, &new->mask) ... /* 2280: v6 entry */<br><br>Consumer (edns0.c:339-343 and 395-396):<br><br> struct subnet_opt { /* 20 bytes total */<br> u16 family; u8 source_netmask, scope_netmask;<br> u8 addr[IN6ADDRSZ]; /* last member, 16 bytes */<br> };<br> len = ((opt->source_netmask - 1) >> 3) + 1; /* 395: mask=192 -> len=24 */<br> memcpy(opt->addr, addrp, len); /* 396: OOB write, up to 16 bytes */<br><br>`opt` is a stack local of add_source_addr() (edns0.c:415-446).<br><br>Reproduction (any query works and no upstream is needed:<br>add_edns0_config() runs unconditionally in receive_query() before the<br>answer/forward decision, so even locally answered queries hit it):<br><br> $ cat p1.conf<br> port=5354<br> add-subnet=192<br><br> $ dnsmasq -d -C p1.conf &<br> $ dig @<a href="http://127.0.0.1">127.0.0.1</a> -p 5354 <a href="http://anything.example.com">anything.example.com</a> A<br><br> ==1687==ERROR: AddressSanitizer: stack-buffer-overflow ... WRITE of size 24<br> #1 calc_subnet_opt src/edns0.c:396:7<br> #2 add_source_addr src/edns0.c:427:13<br> #3 add_edns0_config src/edns0.c:575:10<br> #4 receive_query src/forward.c:1966:11<br><br>Negative control: identical config without the add-subnet line -> process<br>survives, no ASAN report. Without ASAN the 8-16 byte write corrupts<br>adjacent stack frames; the daemon dies or is left corrupted either way.<br><br><br>2. --auth-zone: out-of-bounds read in is_same_net6(), CWE-125<br>--------------------------------------------------------------<br><br>Parse side (option.c:2564/2576/2582):<br><br> if (prefix && !atoi_check(prefix, &prefixlen)) /* 2564: no bounds check */<br> ...<br> subnet->prefixlen = (prefixlen == 0) ? 24 : prefixlen; /* 2576: v4 */<br> subnet->prefixlen = (prefixlen == 0) ? 64 : prefixlen; /* 2582: v6 */<br><br>Authoritative PTR queries reach find_addrlist() -> is_same_net6()<br>(auth.c:36 -> util.c:518-534):<br><br> int pfbytes = prefixlen >> 3; /* 526 */<br> if (memcmp(&a->s6_addr, &b->s6_addr, pfbytes) != 0) /* 529 */<br> ...<br> a->s6_addr[pfbytes] ... /* ~533 */<br><br>`a` is a 24-byte union all_addr local of answer_auth(); `b` is the heap<br>struct addrlist. A stored prefixlen of 193-255 makes memcmp() read 25-31<br>bytes from the 24-byte stack object - a deterministic ASAN report, while<br>non-ASAN builds stay inside mapped memory and only corrupt the<br>subnet-match result (wrong authoritative answers). A *negative* prefixlen<br>(decimal overflow, e.g. "/2147483649") makes the memcmp length huge and<br>crashes production builds immediately (SIGSEGV, no sanitizer involved).<br><br>Reproduction:<br><br> $ cat p2.conf<br> port=5355<br> auth-server=8.b.d.0.1.0.0.2.ip6.arpa,lo<br> auth-zone=8.b.d.0.1.0.0.2.ip6.arpa,2001:db8::/240<br><br> $ dnsmasq -d -C p2.conf &<br> $ dig @<a href="http://127.0.0.1">127.0.0.1</a> -p 5355 -x ::1 # any IPv6 reverse query works; the<br> # address need not be inside the subnet<br><br> ==1696==ERROR: AddressSanitizer: stack-buffer-overflow ... READ of size 30<br> #2 is_same_net6 src/util.c:529:7<br> #3 find_addrlist src/auth.c:36:14<br> #4 find_subnet src/auth.c:49:10<br> #5 answer_auth src/auth.c:160:20<br> #6 receive_query src/forward.c:1953:11<br><br>Two configuration details matter here (both verified empirically):<br><br> * the interface qualifier on auth-server (",lo"): auth_dns must come<br> from the interface dns_auth flag; the pure domain-match path sets<br> local_auth=1, and answer_auth() skips the find_subnet() branch when<br> local_query is true;<br> * the zone domain should be the reverse zone of the subnet, otherwise<br> the PTR query is forwarded instead of answered authoritatively.<br><br>Negative control: prefix /64 (in range) -> process survives and answers<br>authoritative NXDOMAIN; no ASAN report.<br><br><br>Suggested fix<br>-------------<br><br>Parse-side, consistent with existing checks elsewhere (synth-domain<br>already rejects "msize > 128"):<br><br> /* LOPT_ADD_SBNET: first field feeds add_subnet4 (IPv4) */<br> if (new->mask < 0 || new->mask > 32) ret_err(...);<br> /* field after comma feeds add_subnet6 (IPv6) */<br> if (new->mask < 0 || new->mask > 128) ret_err(...);<br> /* addr/mask form: validate per the parsed address family */<br> /* auth-zone subnets: same 32/128 bounds per family, reject negatives */<br><br>Consumer-side hardening (defense in depth):<br><br> /* calc_subnet_opt() */<br> if (opt->source_netmask > 8 * sizeof(opt->addr))<br> opt->source_netmask = 8 * sizeof(opt->addr);<br> /* is_same_net6() */<br> if (prefixlen < 0 || prefixlen > 128) return 0;<br><br>Related spots of the same family (same root cause; happy to test a fix<br>for these too):<br><br> * dhcp-host=.../<negative>: atoi_check(pref, &new_addr->prefixlen) only<br> rejects >128 (option.c:4127-4129); negative values (atoi overflow)<br> later reach is_same_net6() / 1 << (128 - prefixlen) (rfc3315.c:1795,<br> util.c:529) -> huge memcmp / 2^63-iteration allocation loop;<br> * bogus-nxdomain=X/0: prefix 0 is not rejected (option.c:2999) and<br> is_same_net_prefix() (util.c:514-521) computes 1 << (32 - prefix),<br> a shift-width UB.<br><br><br>Environment<br>-----------<br><br>Both issues were verified against a fresh clone of<br><a href="https://thekelleys.org.uk/git/dnsmasq.git">https://thekelleys.org.uk/git/dnsmasq.git</a> - tag v2.93 (3ff66da) and<br>master a9880c595f052d63859d6ed8aa86a3a6b007bf20 - built with clang 10<br>(-fsanitize=address). Clients were standard dig queries over UDP; no<br>upstream servers were configured (not needed for either issue).<br><br>Happy to test a proposed patch on master and v2.93; the configs above<br>reproduce each issue with a single dig command.<br><br>Thanks,<br>Chen</div>
</div>