fix: resolve DNS before SSRF checks to close rebinding bypass (CWE-918) - #342
Open
Matthew-Selvam wants to merge 1 commit into
Open
fix: resolve DNS before SSRF checks to close rebinding bypass (CWE-918)#342Matthew-Selvam wants to merge 1 commit into
Matthew-Selvam wants to merge 1 commit into
Conversation
isInternalNetwork() only checked the literal hostname string, not the resolved IP, so an attacker-controlled domain resolving to a private address bypassed the check entirely. isAllowedUri() is now async and resolves the hostname via dns.lookup(), checking every returned address against the private-range blocklist before allowing a fetch. Fails closed if resolution errors. Also unwraps IPv6-mapped IPv4 addresses (::ffff:127.0.0.1 and the compressed hex form) before the range check, since neither was previously matched by the blocklist regexes. Updates the one caller (fetchAgentCard) and the isAllowedUri call sites in tests to await the now-async function, with DNS mocked for determinism, plus new regression tests for the rebinding bypass, multi-A-record responses, resolution failures (fail-closed), and the IPv6-mapped-IPv4 forms. Fixes Conway-Research#183
Author
|
This is ready for maintainer review whenever you get a chance — happy to make any changes requested. |
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
Fixes #183.
isInternalNetwork()validated the hostname string, not the resolved IP address. An attacker-registered domain that resolves to a private IP (e.g.127.0.0.1) bypassed the SSRF check entirely, sinceisAllowedUri()never performed DNS resolution before allowing a fetch.Also fixes the related gaps called out in the issue:
::ffff:127.0.0.1and the compressed hex form::ffff:7f00:1) were not matched by any blocklist regex.2130706433for127.0.0.1) — verified this is actually already normalized to dotted-quad by the WHATWG URL parser before reachingisInternalNetwork, so no additional handling was needed there, but I left a note in this PR body in case reviewers want to double check that assumption on their Node version.Fix
isAllowedUri()is nowasyncand resolves the hostname viadns.lookup(hostname, { all: true }), checking every returned address against the private-range blocklist before allowing the request. Fails closed (blocks) if resolution errors.isInternalNetwork()now unwraps IPv6-mapped IPv4 forms before the regex check.fetchAgentCard, alreadyasync) toawaitthe call.Testing
tsc --noEmitpasses.isAllowedUricall sites acrossdata-layer.test.ts,discovery-data-uri.test.ts, andsocial.test.tstoawaitthe now-async function, withnode:dns/promisesmocked so tests stay hermetic (no real network access required).127.0.0.1), multi-A-record response where only one address is private, fail-closed behavior on DNS resolution error, and both IPv6-mapped-IPv4 forms.