[rust] use saturating arithmetic in DNS record length checks#6748
Open
winstoncrooker wants to merge 1 commit into
Open
[rust] use saturating arithmetic in DNS record length checks#6748winstoncrooker wants to merge 1 commit into
winstoncrooker wants to merge 1 commit into
Conversation
The bounds checks added in cloudflare#6311 use unchecked addition between an attacker-supplied length (parsed via `usize::from_str_radix` from a hex token) and a small offset. With overflow-checks disabled in release builds, a length value of `usize::MAX` makes `length + offset` wrap to a small integer. The bounds check then passes and the subsequent `input[start..end]` slice (or `data[i]` index) panics — exactly the process-abort behavior cloudflare#6311 was meant to prevent (per the test commentary at the top of the malformed-input block in dns.rs). Switch each affected check to `saturating_add` so an overflowing length saturates at `usize::MAX`, which is always greater than `data.len()`, firing the existing `Err` branch as intended. Affected sites: `parse_replacement` (~line 98), `parse_caa_record` (~line 170), and the flag / service / regexp length checks in `parse_naptr_record` (~lines 247, 256, 265). Five new tests in `dns.rs::tests` mirror the existing `test_parse_*_too_short_*` cases but use `FFFFFFFFFFFFFFFF` (or its decimal form for CAA) as the length token. They panic on `main` and pass with this patch.
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
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.
While reading through
src/rust/api/dns.rsI noticed the bounds checks added in #6311 use unchecked addition between an attacker-supplied length (parsed viausize::from_str_radixfrom a hex token in the DNS record) and a small offset. With-Cdebug-assertions=nin release builds (per.bazelrc), a length value ofusize::MAXmakeslength + offsetwrap to a small integer, the> input.len()check passes, and the subsequent slice/index panics — the exact behavior the test commentary above the malformed-input block says we want to avoid ("these previously caused panics (index out of bounds) which would abort the process via CXX. They must return Err, not panic.").The fix is one token per check site:
length + offset→length.saturating_add(offset). Saturated overflow atusize::MAXis always greater thandata.len(), so the existingErrbranch fires.Sites changed
parse_replacementparse_caa_recordprefix_lengthparse_naptr_recordflag_lengthparse_naptr_recordservice_lengthparse_naptr_recordregexp_lengthTests
Five new tests in
dns.rs::tests, mirroring the existingtest_parse_*_too_short_*/_truncated_at_*cases but withFFFFFFFFFFFFFFFF(or its decimal form for CAA) as the length token:test_parse_replacement_length_overflowtest_parse_caa_record_prefix_length_overflowtest_parse_naptr_record_flag_length_overflowtest_parse_naptr_record_service_length_overflowtest_parse_naptr_record_regexp_length_overflowEach one panics on
mainand passes with this patch. I ran them in a standalone Cargo crate that mirrors the function bodies (the workerd build is bazel-only, but the math is identical in isolation); CI will run the in-tree versions viabazel test //src/rust/....Reachability note
The whole-process impact is mitigated by
jsg::catch_panicwrapping#[jsg_method]calls — a slice panic inparse_replacementis caught and surfaces asthrow_internal_error+terminate_execution()on the calling isolate, not a workerd-wide abort. So in practice this is a per-request failure on Workers that calldns.resolveNaptr/dns.resolveCaawith attacker-influenced hostnames, not a tenant-isolation bug. Still seems worth fixing both because the test commentary explicitly forbids panicking here and because future callers outside the jsg catch boundary would lose that protection.