fix: harden the tls/x509 parser against malformed peer input - #445
Merged
Conversation
the der walker sliced body[pos .. pos + hdr.length] from attacker- controlled length fields without checking the bound first in ~15 spots. in releasesafe an out-of-bounds slice panics — so a malformed peer cert could crash the mtls handshake thread (remote dos). add a sliceChecked(buf, start, len) helper that validates the bound (overflow-safe subtraction form) and returns error.InvalidCert, and route every length-driven slice in parseDer / parseTbsFields / extractCnFromName / extractEcPointFromSpki / extractSanUrisFromExtensions / fillSanUris / extractFirstOid through it. also fix SAN handling to fail closed: a cert with more SAN URIs than the cap previously dropped the rest silently, so a required identity URI past the cap would never match. raise the cap (8 -> 32) and reject on overflow instead of truncating. new tests: parseDer rejects every truncation prefix and single-byte header corruptions without panicking; fillSanUris rejects an over-cap SAN list. existing positive round-trips unchanged.
both the client (doHandshakeInner) and server (acceptClientAuthAndFinished) reassemble the peer's handshake flight into a growing buffer with no cap and no iteration limit — a malicious peer could stream handshake records forever, exhausting memory. cap each side's flight at 64 KB (real cert chains are a few KB) and fail the handshake past it. also widen the san_buf call sites to x509_verify.max_san_uris now that the cap moved to 32.
the tls stack was the only attack-surface parser without a fuzz target (http/manifest/dns/cluster-msg/gossip-msg/wireguard already have one). fuzz-tls drives x509_verify.parseDer and the handshake message parsers on arbitrary bytes, asserting every returned slice stays within the input and nothing panics.
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
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.
summary
first of a four-PR hardening pass (targeted, high-confidence). this one
covers the freshest, most exposed surface: the hand-written X.509/DER
parser and TLS handshake parsers that consume bytes from a remote peer
during the mTLS handshake. all changes are reject-the-malformed /
bound-the-unbounded — no behavior change for well-formed inputs.
security-critical crypto path → not marked auto-merge, matching the
Phase 8 posture.
what's in here
1. bounds-check every length-driven slice in the DER parser
the walker sliced
body[pos .. pos + hdr.length]from attacker-controlledlength fields without checking the bound first in ~15 places. in
ReleaseSafe an out-of-bounds slice panics — so a malformed peer cert
could crash the handshake thread (remote DoS; not memory corruption —
Zig is bounds-checked). added a
sliceChecked(buf, start, len)helper(overflow-safe subtraction form) returning
error.InvalidCert, androuted every length-driven slice through it.
2. SAN handling fails closed
a cert with more SAN URIs than the cap silently dropped the rest — a
required identity URI past the cap would never match. raised the cap
8 → 32 and now reject on overflow instead of truncating.
3. bound the handshake read loops
both client (
doHandshakeInner) and server (acceptClientAuthAndFinished)reassembled the peer's handshake flight into a growing buffer with no cap
and no iteration limit — a malicious peer could stream records forever.
capped each side at 64 KB (real cert chains are a few KB).
4. fuzz-tls target
the TLS stack was the only attack-surface parser without a fuzz target.
zig build fuzz-tlsdrivesx509_verify.parseDer+ the handshakemessage parsers on arbitrary bytes, asserting every returned slice stays
within the input and nothing panics.
tests
parseDer rejects truncations at every prefix length without panicking— slices the cert at every length and asserts an error, never a crash
parseDer rejects a cert whose inner length fields point past the buffer— perturbs each header/length byte; asserts no panic
fillSanUris fails closed when SAN count exceeds the captests stay green
verification
YOQ_SKIP_SLOW_TESTS=1 zig build test— 2238 passed, 0 failedzig build fuzz-tls— corpus pass green (43 cases incl. inline tests)tools/panic_audit.shclean — the bounds helper returns errors, neverpanics, so no new panic-shaped constructs
zig fmt --checkclean