Constant-time secret comparison + stdlib-reuse audit (Tier 1+2)#28
Merged
Conversation
RFC 9218 §4.2 default is non-incremental (sequential). Flip the SendStream default and have WebTransport bidi/uni opens explicitly set incremental=true, since WT streams have no defined inter-stream ordering and round-robin interleaving matches the quicperf path. Browser demo: allow empty cert hash to rely on CA trust, and use datagrams.createWritable() fallback for the Safari 26.4 API change. Assisted-by: Claude Opus 4.7
Six authentication secrets were compared with std.mem.eql, which short-circuits on the first differing byte — a timing side-channel. Zig's stdlib already provides the correct primitive (std.crypto.timing_safe.eql); use it directly. Every QUIC/TLS reference stack (BoringSSL, rustls, quic-go, quiche) compares these in constant time; this closes a conformance + security gap. Pre-existing, unrelated to the std.crypto.tls dedup branch. Sites (length is public/spec-fixed, only the value is secret, so a public length check precedes the constant-time array compare): - quic/stateless_reset.zig stateless reset token (RFC 9000 §10.3) - quic/packet.zig Retry integrity tag (RFC 9001 §5.8) - quic/tls13.zig ×2 Finished verify_data (RFC 8446 §4.4.4) - quic/tls13.zig PSK binder HMAC (RFC 8446 §4.2.11.2) - http1/tls.zig Finished verify_data (RFC 8446 §4.4.4) Wrong-length rejection semantics preserved. Tests 522/522, client/server build clean, and a real-Chrome WebTransport full TLS 1.3 handshake (exercising server-side Finished verification) + bidi/datagram echo verified via Puppeteer. Assisted-by: Claude Opus 4.7
Tier 2 of the stdlib-reuse audit (clear, low-risk): - stream.zig: two hand-rolled descending insertion sorts for send_order scheduling replaced with std.sort.insertion (stable; equal send_order keeps arrival order, matching prior semantics). - transport_params.zig: preferred_address port decode used std.mem.bigToNative(u16, @bitcast([2]u8)) -- spell it as the authoritative std.mem.readInt(u16, &bytes, .big). (Skipped as borderline: the symmetric write-side toBytes(nativeToBig(...)) -- already pure-stdlib, and switching hinges on the writer type exposing writeInt; not worth the churn.) Tests 522/522, client/server build clean. Assisted-by: Claude Opus 4.7
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
1 participant
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
Whole-codebase audit for functionality that duplicates / misuses Zig's standard library. The headline is security, not tidiness.
Tier 1 — Security (commit
de72983)Six authentication secrets were compared with
std.mem.eql, which short-circuits on the first differing byte — a timing side-channel. Zig's stdlib already provides the correct primitive; this switches them tostd.crypto.timing_safe.eql. Pre-existing; unrelated to PR #27. Every QUIC/TLS reference stack (BoringSSL, rustls, quic-go, quiche) compares these in constant time — this closes a conformance + security gap.quic/stateless_reset.zigquic/packet.zigquic/tls13.zig×2quic/tls13.zighttp1/tls.zigLength is public (fixed by spec/cipher-suite), so a public length check precedes the constant-time array compare; wrong-length rejection semantics preserved. Two of the six (
http1/tls.zig,packet.zig) were missed by a file-scoped pass and only caught by the whole-codebase sweep.Tier 2 — stdlib reuse (commit
12917aa)stream.zig: two hand-rolled descending insertion sorts →std.sort.insertion(stable; same equal-key semantics).transport_params.zig:bigToNative(u16, @bitCast([2]u8))→std.mem.readInt(u16, &bytes, .big).Deliberately not changed (evaluated)
qlog.zighand-rolled JSON (justified: streaming JSON-SEQ, no allocation —std.jsonis DOM/heavier),ack_handler.zigthinArrayListwrappers (named-type readability; removal ripples for marginal gain),sys.zig/io_compat.zig(deliberate Zig 0.16 shims),sockaddr.zig(POSIX syscall adapters), MoQ/QPACK varints + Huffman (RFC-specified, no stdlib equivalent). The codebase is otherwise disciplined.Verification
zig build test --summary all: 522/522 at every commitzig build: client/server clean🤖 Generated with Claude Code