fix(web-fetch): prevent SSRF via DNS hostnames and redirects#27739
fix(web-fetch): prevent SSRF via DNS hostnames and redirects#27739g0w6y wants to merge 2 commits into
Conversation
The web_fetch tool guarded outbound requests with isBlockedHost, which inspects only the entry url hostname string. A non IP hostname that resolves to a private or link local address, for example metadata.google.internal, passed the guard, and redirects were followed with no revalidation, so an allowed host could redirect to an internal address. In both cases the internal response body was returned to the caller. Add safeFetchFollowingRedirects, which validates the initial url and every redirect hop against the literal address and the resolved address using the existing isPrivateIpAsync, and follows redirects manually. web_fetch now uses it for both fallback fetch paths. Add tests for assertUrlIsPublic and the guarded fetch. This was reported to the Google Bug Hunters program.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a Server Side Request Forgery (SSRF) vulnerability in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/L
|
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
There was a problem hiding this comment.
Code Review
This pull request introduces SSRF protection for web fetching by replacing fetchWithTimeout with a new safeFetchFollowingRedirects utility, which manually follows redirects and validates that each hop targets a public IP address. It also adds corresponding unit tests. The review feedback highlights critical security and correctness issues in the manual redirect implementation, specifically regarding cross-origin header leakage (CWE-200), potential protocol smuggling/SSRF bypass due to lack of protocol validation, and HTTP spec compliance for redirect methods (e.g., handling 303 redirects by changing the method to GET). A comprehensive code suggestion is provided to address these issues.
Address review feedback on the SSRF fix: - reject redirect targets whose protocol is not http or https, preventing protocol smuggling to file, gopher, or ftp - strip credential carrying headers (authorization, cookie, cookie2, proxy-authorization) when a redirect crosses to a different origin - downgrade the method to GET and drop the body and content headers for 303 responses and for non GET or HEAD 301 and 302 responses, per RFC 9110 Redirect logic is extracted into applyRedirect and covered by unit tests.
|
Thanks for the review. Addressed all three points in the follow up commit:
The redirect logic is extracted into applyRedirect with unit tests covering protocol rejection, header stripping on cross vs same origin, and the method downgrade cases. |
What this fixes
The
web_fetchtool guards outbound requests withisBlockedHost(
packages/core/src/tools/web-fetch.ts), which inspects only the hostnamestring of the entry url and delegates to the synchronous
isPrivateIp. Twogaps let a request reach a private or internal target:
isPrivateIpreturns false for anything that is not an IPliteral, so a DNS hostname that resolves to a private or link local address
passes the guard. Examples are
metadata.google.internal, an attacker owneddomain whose A record points to
169.254.169.254or127.0.0.1, andwildcard resolvers such as
169.254.169.254.nip.io.HTTP redirects with the default behaviour and no revalidation, so an allowed
host can redirect to an internal address.
In both cases the internal response body is returned to the caller, so the
attacker reads internal or cloud metadata content. On a Google Cloud host this
includes the service account token at
http://metadata.google.internal/computeMetadata/v1/instance/service-accounts/default/token.The repository already contains
isPrivateIpAsync, which resolves the hostnameand checks the address, but
web_fetchnever used it.The change
Add
assertUrlIsPublicandsafeFetchFollowingRedirectstopackages/core/src/utils/fetch.ts.assertUrlIsPublicrejects a url whose literal hostname is private orloopback, and whose resolved address is private or link local, reusing the
existing
isPrivateIpandisPrivateIpAsync.safeFetchFollowingRedirectsfollows redirects manually withredirect: 'manual'and callsassertUrlIsPublicon the initial url and onevery redirect hop before connecting, so a hostname that resolves inward and
a redirect that points inward are both refused. It caps the redirect chain.
web_fetchnow usessafeFetchFollowingRedirectsfor both fallback fetchpaths. The existing synchronous
isBlockedHostpre check is kept as a fastfirst filter.
Test cases
Added
packages/core/src/utils/fetch.ssrf.test.ts:assertUrlIsPublicrejects literal private and loopback addresses:127.0.0.1,169.254.169.254,10.0.0.5,192.168.1.1,localhost.assertUrlIsPublicrejects a hostname that resolves to a private address(dns lookup mocked), including
metadata.google.internalresolving to169.254.169.254.assertUrlIsPublicallows a hostname that resolves to a public address.safeFetchFollowingRedirectsrefuses a private target, so the internal bodyis never read.
Manual end to end check that the redirect hop is revalidated. With a local
internal server holding a secret and a redirector that points at it, a fetch
that passes the entry check is still refused when the redirect target resolves
to a private address, and the secret is never returned.
Suggested follow up
For full protection against DNS rebinding between the check and the connection,
the validated address can be pinned at connect time with a custom dispatcher.
This change closes the reported bypasses without that larger refactor.
Security
This addresses a Server Side Request Forgery issue (CWE 918). This was reported
to the Google Bug Hunters program.
Checklist
web_fetchfetch paths.isPrivateIpAsyncrather than introducing a parallel check.