Repository navigation
Conversation
Refs: google-gemini#28555 - Updated isBlockedHost to use async DNS resolution via isPrivateIpAsync - Refactored WebFetchTool callers to await block checks - Ensures domains pointing to loopback/private IPs are correctly blocked
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 critical 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/M
|
🛑 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. |
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the private IP check in WebFetchTool to use the asynchronous isPrivateIpAsync helper, adds IPv6 loopback (::1) to the blocked hosts list, and fixes a bug where loopback hosts were incorrectly returned as non-private. The review feedback highlights critical Server-Side Request Forgery (SSRF) vulnerabilities, specifically pointing out that pre-flight checks are susceptible to DNS Rebinding (TOCTOU) and 3xx redirect bypasses. It is highly recommended to implement socket-level validation using a custom connection manager (such as an undici connector) to secure the fetch operations.
|
|
||
| if (isLoopbackHost(hostname)) { | ||
| return false; | ||
| return true; |
There was a problem hiding this comment.
DNS Rebinding (TOCTOU) SSRF Vulnerability
Performing an asynchronous pre-flight DNS check (isPrivateIpAsync) before executing fetch is vulnerable to a Time-of-Check to Time-of-Use (TOCTOU) DNS Rebinding attack.
An attacker can configure a malicious DNS server with a very low TTL (Time-To-Live) that resolves to a public IP during the isPrivateIpAsync check (passing the validation), but resolves to a private/loopback IP (e.g., 127.0.0.1 or 169.254.169.254) when fetch performs its own DNS resolution to establish the connection.
To completely prevent DNS Rebinding, you should intercept DNS resolution at the socket connection level using a custom undici connector. This ensures that the IP address being connected to is validated at the exact moment the connection is established.
Here is a secure implementation using buildConnector from undici:
import { Agent, buildConnector } from 'undici';
import dns from 'node:dns';
const safeConnector = buildConnector({
lookup: (hostname, options, callback) => {
dns.lookup(hostname, options, (err, address, family) => {
if (err) {
callback(err);
return;
}
const addresses = Array.isArray(address)
? address
: [{ address, family }];
const hasPrivate = addresses.some(addr => isAddressPrivate(addr.address));
if (hasPrivate) {
callback(new PrivateIpError('Access to private network is blocked'));
} else {
callback(null, address, family);
}
});
}
});
// Configure the global dispatcher with the safe connector
setGlobalDispatcher(
new Agent({
connect: safeConnector,
headersTimeout: defaultHeadersTimeout,
bodyTimeout: defaultBodyTimeout,
}),
);This centralized socket-level validation is a robust, "secure by default" defense-in-depth approach that renders pre-flight checks obsolete and fully secures the web-fetch tool.
| private async isBlockedHost(urlStr: string): Promise<boolean> { | ||
| try { | ||
| const url = new URL(urlStr); | ||
| const hostname = url.hostname.toLowerCase(); | ||
| if (hostname === 'localhost' || hostname === '127.0.0.1') { | ||
| if ( | ||
| hostname === 'localhost' || | ||
| hostname === '127.0.0.1' || | ||
| hostname === '::1' | ||
| ) { | ||
| return true; | ||
| } | ||
| return isPrivateIp(urlStr); | ||
| return await isPrivateIpAsync(urlStr); |
There was a problem hiding this comment.
This code is vulnerable to Server-Side Request Forgery (SSRF) bypasses due to both DNS Rebinding (Time-of-Check to Time-of-Use) and automatic 3xx redirect handling.
- DNS Rebinding: The
isBlockedHostcheck is performed on the hostname, but the actualfetchrequest performs its own DNS resolution. A malicious DNS server could return a public IP during the initialisBlockedHostcheck and then a private IP (e.g.,127.0.0.1or169.254.169.254) whenfetchestablishes the connection, bypassing the protection. - 3xx Redirect Bypass: The
isBlockedHostcheck is only applied to the initial URL.fetch(viaundici) automatically follows HTTP redirects (3xxstatus codes). If a public URL redirects to a private/loopback IP, the redirect will be followed without re-runningisBlockedHost, completely bypassing the SSRF protection.
To mitigate these issues, the IP address validation must be performed at the connection level. This can be done by configuring a custom DNS lookup or connect function in the global undici dispatcher to reject private IPs. Additionally, consider setting redirect: 'manual' in fetch options and manually validating each redirect URL in the chain, or ensure the connection-level validation covers redirected IPs.
gemini-cli
Bot
added
priority/p2
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
|
This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
This branch had an error being deployed
1 participant
Description
Fixes #28555
This PR addresses a critical Server-Side Request Forgery (SSRF) vulnerability (CVSS 8.6) in the
web-fetchtool where malicious actors could bypass DNS protections by using a custom domain pointing to a private or loopback IP address (e.g.,169.254.169.254).The previous implementation relied on synchronous IP checks (
isPrivateIp) which only caught explicit private IP addresses but did not resolve hostnames to their underlying IPs.Changes
isBlockedHostinweb-fetch.tsto utilize the asynchronousisPrivateIpAsyncfromfetch.ts, ensuring that all domains are properly resolved and checked for private/loopback IP resolution before being fetched.filterAndValidateUrlsto an async function to properly await theisBlockedHostchecks.WebFetchTool(execute,executeExperimental,executeFallbackForUrl) toawaitthe filtered URLs.web-fetch.test.tsto mock the new asynchronous implementation and ensure accurate coverage of the IP resolution logic.Security Impact
This fix prevents attackers from accessing internal services, metadata endpoints, or bypassing network restrictions through the Gemini CLI's web-fetch capability.
Checklist