common: bound untrusted inputs and address parsing (v26.06.7 security, 4/7) - #9510
Merged
nGoline merged 8 commits intoSep 15, 2026
Merged
Conversation
daywalker90
requested changes
Sep 15, 2026
nGoline
force-pushed
the
port-26.06.7-input-bounds
branch
from
September 15, 2026 12:52
a240a36 to
9b5a39d
Compare
connectd assembles the SOCKS5 CONNECT request in a fixed buffer, but never checks that the hostname fits it. Add a test that a maximum-length proxied hostname produces exactly the request it should, with connectd still alive afterwards. Changelog-None (cherry picked from commit 6ecead4)
The SOCKS5 CONNECT request is built into a fixed 255-byte buffer, but a domain-name request needs 7 + strlen(host) bytes and the host length was never checked. A host longer than 248 bytes overran the buffer while the request was assembled, corrupting the adjacent length field, which was then used as the io_write() length and produced a large out-of-bounds read: the proxy socket received connectd's heap instead of the request, and connectd died. The host reaches this both from a proxied connect and from a gossiped DNS address. Size the buffer to hold the largest legal request (header plus a maximum-length domain name), and refuse a hostname which still does not fit rather than building a request we cannot represent. Truncating the host is not an option: that is a connection to the wrong destination. Changelog-Fixed: connectd: a proxied connection to a very long hostname (including a gossiped DNS address) could crash the node. (cherry picked from commit 04ca809)
fromwire_wireaddr() accepted any bytes at all as an ADDR_TYPE_DNS address, so a node_announcement could put arbitrary content into log messages, listnodes output and outgoing connect attempts. is_dnsaddr() already encodes the rules; apply it, and ignore the descriptor when it fails, the same way we already ignore a descriptor with a zero port. Changelog-Changed: gossipd: DNS addresses in a node_announcement which are not valid hostnames are now ignored. (cherry picked from commit 9574eaa)
fromwire_wireaddr() now ignores a DNS descriptor which isn't a hostname, but
parse_wireaddr() still accepts one: it only checks the length. So an
announce-addr which fails is_dnsaddr() is accepted at startup, gossiped, and
then dropped by every peer which receives it, with nothing in our own logs to
say the address is useless.
Apply the same rule where the operator can see the answer. A name which
passes is_dnsaddr() is unaffected; a name which fails it was already being
ignored by everyone.
This also restores the towire/fromwire round trip which tests/fuzz/fuzz-wireaddr
asserts, and which the fromwire-side check alone broke:
Assertion failed: (fromwire_wireaddr((const u8 **)&output_buffer, &len,
&decoded_wa) == FROMWIREADDR_OK),
function run, file fuzz-wireaddr.c, line 37.
Changelog-Changed: config: `dns:` addresses which are not valid hostnames are now rejected at startup instead of being silently ignored by peers.
(cherry picked from commit 0613903)
json_next() and the JSMN result validator each recurse once per level of nesting, so a sufficiently deeply nested (but otherwise valid) JSON document could exhaust the C stack. Bound the nesting depth in json_parse_input(), before either recursive walk runs, and reject anything deeper. The depth check is itself iterative, so it cannot overflow. Changelog-Fixed: JSON-RPC: reject excessively-nested JSON rather than risk a stack overflow. (cherry picked from commit b940aa3)
Changelog-Fixed: clnrest: an unauthenticated user could crash the node with a large request body; bodies are now capped at 2MiB. (cherry picked from commit d5c2f6d)
Changelog-None (cherry picked from commit 23e98db)
tlv_span tracked its range with two pointers and returned end - start. end was assigned only after the type > maxfield test, so a stream whose first in-range field also exceeds maxfield left end NULL while start was set, and the guard that copies start over only fired when start was NULL. The returned end - start was then a pointer difference against NULL, close to the full width of size_t, which callers hand to sha256_update as a length: an out-of-bounds read walking mapped memory until it faults. Track the span as offsets rather than pointers, and return an empty span (at the range's position) whenever no field falls inside the range or the end precedes the start, which is the correct measure for such a stream. Changelog-Fixed: A crafted BOLT12 message could cause an out-of-bounds read while computing its identifier. Co-authored-by: Claude <noreply@anthropic.com> (cherry picked from commit a0b4517)
nGoline
force-pushed
the
port-26.06.7-input-bounds
branch
from
September 15, 2026 21:26
9b5a39d to
4e3dce8
Compare
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
2 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.
Fourth of seven PRs forward-porting the v26.06.7 security point release onto master.
Bounds on untrusted input: SOCKS5 and DNS hostnames, JSON nesting depth, BOLT12 TLV spans, and clnrest request bodies.
8 commits:
pytest: check the SOCKS5 request for a maximum-length hostnameconnectd: bound the SOCKS5 request hostname to the request buffercommon: ignore DNS address descriptors which are not hostnamescommon: reject dns: addresses which are not hostnamescommon: bound JSON nesting depth when parsingclnrest: restrict maximum size of request bodies to 2MiBtests: add clnrest test for large request bodiescommon: fix tlv_span returning huge length for out-of-range streamSix
Changelog-entries, including twoChangedthat affect configuration:dns:addresses which are not valid hostnames are now rejected at startup rather than silently ignored by peers, and DNS addresses in anode_announcementwhich are not valid hostnames are ignored.For reviewers, please read before looking at CI
tests/test_clnrest.py::test_large_request_bodymight fail here, and it is shipped that way deliberately.The test POSTs a 32 MiB body against the new 2 MiB cap and asserts a
413. It passes on OpenSSL 3.6.3 and fails on every OpenSSL 3.0.x environment tested so far: Ubuntu 22.04 (3.0.2, Python 3.10.12), Ubuntu 24.04 in Docker (3.0.13, Python 3.12.3), a native Linux x86-64 box, and an arm64 Docker run. Both architectures, with identicalrequests2.33.1 andurllib32.6.3 on the passing and failing sides, so the client library versions are not the variable and CPython does not setOP_IGNORE_UNEXPECTED_EOFby default. CI runsubuntu-24.04, so it might have the failing configuration.The mechanism, as far as it is understood: the handler stops reading at 2 MiB and writes the
413while the peer is still sending, so the connection is closed with unread data still queued. That produces an RST rather than a FIN, and an RST discards whatever is already sitting in the client's receive buffer, including the response. Whether the client sees a413or a transport error is then a race, and OpenSSL 3.0.x surfaces the abrupt close as an error where newer versions are more forgiving.This PR ports what the release shipped, unmodified, rather than weakening the test to accept a connection error: doing that would stop it asserting the
413contract and would bake in the behaviour. If CI confirms the failure, the fix is server-side and small, keep reading and discarding until the body is consumed before responding, which keeps memory bounded and makes the response delivery deterministic. Happy to add that here or as a follow-up, and it is owed to the release line too.