Skip to content

explicitly check for assumptions in api/dns.rs#6311

Merged
anonrig merged 1 commit intomainfrom
yagiz/improve-node-dns
Mar 13, 2026
Merged

explicitly check for assumptions in api/dns.rs#6311
anonrig merged 1 commit intomainfrom
yagiz/improve-node-dns

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Mar 12, 2026

No description provided.

@anonrig anonrig requested review from a team as code owners March 12, 2026 18:45
Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm Bonk, and I've done a quick review of your PR.

This PR adds bounds checks to parse_caa_record and parse_naptr_record to convert panics from out-of-bounds slice indexing into proper Err returns. Since panics across CXX abort the process, this is a good hardening change.

Issues (by severity):

  1. Medium: parse_replacement has the same class of bug. The function at line 95 does input[offset_index..length + offset_index] where length is parsed from untrusted hex input. A malicious or malformed length value will cause a panic. Since parse_replacement is called from parse_naptr_record (line 264), this is reachable from JS via the same path. The same hardening applied in this PR should be extended there:

    if length + offset_index > input.len() {
        return Err(DnsParserError::InvalidDnsResponse(
            "replacement data too short for declared frame length".to_owned(),
        ));
    }
    let subset = input[offset_index..length + offset_index].to_vec();
  2. Low: Redundant check in parse_caa_record. The data.len() < 2 check on line 154 is technically always true when parts.len() == 3 (since the first guard already ensured parts.len() >= 3, and data = parts[2..]). This is defensible as defense-in-depth, so just flagging for awareness — no action needed.

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 12, 2026

Review posted successfully on PR #6311. Here's a summary of what I found:

1 actionable issue:

  • Medium severity: parse_replacement (line 95) has the exact same unchecked slice indexing vulnerability that this PR fixes in the other two functions. A malformed length value parsed from untrusted hex input can cause a panic, which aborts across CXX. Since parse_replacement is called from parse_naptr_record, this is reachable from JS. I suggested the same bounds-check pattern be applied there.

1 informational note:

  • The data.len() < 2 check in parse_caa_record is logically redundant given the earlier parts.len() < 3 guard, but it's fine as defense-in-depth.

github run

@anonrig anonrig force-pushed the yagiz/improve-node-dns branch from 9d844d1 to 44dbfd1 Compare March 12, 2026 18:52
@anonrig anonrig requested a review from jasnell March 12, 2026 18:53
@anonrig anonrig force-pushed the yagiz/improve-node-dns branch from 44dbfd1 to 9f698c9 Compare March 12, 2026 20:28
@anonrig anonrig force-pushed the yagiz/improve-node-dns branch from 9f698c9 to b2791c6 Compare March 12, 2026 22:58
Copy link
Contributor

@tewaro tewaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@anonrig anonrig merged commit 04c6588 into main Mar 13, 2026
24 of 25 checks passed
@anonrig anonrig deleted the yagiz/improve-node-dns branch March 13, 2026 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants