🛡️ Sentinel: [HIGH] Fix SSRF DNS Rebinding vulnerability #385
Conversation
- Modified `server/ssrf.ts` to check ALL resolved IP addresses using `dns.lookup({ all: true })`.
- Updated `safeFetch` to reject requests if any resolved IP is unsafe.
- Added regression test in `server/__tests__/ssrf.test.ts` for mixed safe/unsafe IPs.
Co-authored-by: Doezer <11655673+Doezer@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello @Doezer, 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 significantly enhances the application's security by patching a critical SSRF vulnerability. The core change involves a more thorough validation of IP addresses during DNS resolution, preventing attackers from bypassing internal network access controls through DNS rebinding. By checking all resolved IPs instead of just the first, the system now offers a stronger defense against malicious redirects to sensitive internal services. Highlights
Changelog
Activity
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to address a critical SSRF vulnerability related to DNS rebinding by switching to dns.lookup({ all: true }) and validating all resolved IP addresses. However, the protection remains vulnerable to a TOCTOU (Time-of-Check Time-of-Use) attack for HTTPS requests because the validated IP address is not pinned for the actual network call. Additionally, two critical edge cases have been identified where an empty DNS resolution result could allow the vulnerability to persist, requiring further attention to ensure the fix is robust.
| if (addresses.length > 0) { | ||
| address = addresses[0].address; | ||
| family = addresses[0].family; | ||
| } |
There was a problem hiding this comment.
If dns.lookup resolves to an empty array of addresses, the if (addresses.length > 0) condition will be false. In this case, address will retain its initial value of hostname. For HTTP requests, this means the final fetch call will use the original hostname instead of a pinned IP address, re-introducing the Time-of-Check Time-of-Use (TOCTOU) DNS rebinding vulnerability this PR aims to fix. A resolution to an empty list of addresses should be treated as a failure.
if (addresses.length > 0) {
address = addresses[0].address;
family = addresses[0].family;
} else {
// If DNS resolves to an empty array, it's a resolution failure.
throw new Error(`Failed to resolve hostname: ${hostname}`);
}| const addresses = await dns.lookup(hostname, { all: true }); | ||
|
|
||
| // Check all resolved addresses to prevent DNS rebinding attacks | ||
| for (const { address } of addresses) { | ||
| if (!isSafeIp(address, allowPrivate)) { | ||
| throw new Error("Invalid or unsafe URL"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of DNS rebinding protection in safeFetch is incomplete for HTTPS requests. While the code correctly validates all resolved IP addresses (lines 191-198), it only 'pins' the IP address for HTTP requests (lines 228-238, outside this hunk but part of the logic). For HTTPS requests, the function uses the original urlStr (which contains the hostname) for the actual fetch call. This creates a Time-of-Check Time-of-Use (TOCTOU) vulnerability: an attacker can configure a malicious DNS server to return a safe IP during the dns.lookup call and then return an internal/unsafe IP (like 127.0.0.1) when the underlying HTTP client performs its own resolution for the actual request. To fully remediate this for HTTPS, you should use a custom DNS resolver or an agent that pins the connection to the validated IP address while maintaining the original hostname for SNI and certificate validation.
| const addresses = await dns.lookup(hostname, { all: true }); | ||
| // Check all resolved addresses to prevent DNS rebinding attacks | ||
| for (const { address } of addresses) { | ||
| if (!isSafeIp(address, options.allowPrivate)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
If dns.lookup resolves to an empty array of addresses, this function will currently return true. This is incorrect, as a hostname that doesn't resolve to any IP (or resolves to an empty set) should be considered unsafe. This could happen for hostnames with only AAAA records on a network without IPv6 connectivity, for example. The function should return false if no addresses are found.
const addresses = await dns.lookup(hostname, { all: true });
if (addresses.length === 0) {
return false;
}
// Check all resolved addresses to prevent DNS rebinding attacks
for (const { address } of addresses) {
if (!isSafeIp(address, options.allowPrivate)) {
return false;
}
}
return true;
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on enhancing security by preventing DNS rebinding attacks in the isSafeUrl and safeFetch functions within server/ssrf.ts. The changes involve modifying dns.lookup to retrieve all resolved IP addresses for a given hostname and then iterating through them to ensure none are unsafe. Corresponding unit tests in server/__tests__/ssrf.test.ts were updated and expanded to cover these new security checks, including scenarios for DNS rebinding and metadata service IP blocking. Minor formatting adjustments were also made across several client-side and test files. The review comments highlight a critical client-side XSS vulnerability in client/src/components/RssFeedList.tsx due to unvalidated item.link usage, recommending a safeUrl utility. Additionally, the reviewer suggests further hardening the DNS resolution logic in server/ssrf.ts by adding explicit checks for empty address arrays returned by dns.lookup in both isSafeUrl and safeFetch, along with new unit tests for this edge case.
| const addresses = await dns.lookup(hostname, { all: true }); | ||
| // Check all resolved addresses to prevent DNS rebinding attacks | ||
| for (const { address } of addresses) { | ||
| if (!isSafeIp(address, options.allowPrivate)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
There's a potential vulnerability here if dns.lookup resolves to an empty array of addresses. In that case, the for loop will not execute, and the function will return true, incorrectly marking a non-resolving (or maliciously-resolving) URL as safe. You should add a check for an empty addresses array and return false in that case.
const addresses = await dns.lookup(hostname, { all: true });
if (addresses.length === 0) {
return false;
}
// Check all resolved addresses to prevent DNS rebinding attacks
for (const { address } of addresses) {
if (!isSafeIp(address, options.allowPrivate)) {
return false;
}
}
return true;| try { | ||
| const lookup = await dns.lookup(hostname); | ||
| address = lookup.address; | ||
| family = lookup.family; | ||
| const addresses = await dns.lookup(hostname, { all: true }); | ||
|
|
||
| // Check all resolved addresses to prevent DNS rebinding attacks | ||
| for (const { address } of addresses) { | ||
| if (!isSafeIp(address, allowPrivate)) { | ||
| throw new Error("Invalid or unsafe URL"); | ||
| } | ||
| } | ||
|
|
||
| // Use the first resolved address for HTTP pinning | ||
| if (addresses.length > 0) { | ||
| address = addresses[0].address; | ||
| family = addresses[0].family; | ||
| } | ||
| } catch (error) { | ||
| if (error instanceof Error && error.message === "Invalid or unsafe URL") { | ||
| throw error; | ||
| } | ||
| throw new Error(`Failed to resolve hostname: ${hostname}`); | ||
| } |
There was a problem hiding this comment.
If dns.lookup resolves to an empty array, addresses will be empty. The code will then proceed without setting address to a resolved IP. Later, for HTTP requests, safeUrl.hostname will be set to the original hostname, bypassing the IP pinning protection against TOCTOU vulnerabilities. You should handle the case where addresses is empty, for example by throwing an error.
try {
const addresses = await dns.lookup(hostname, { all: true });
if (addresses.length === 0) {
// Throw an error that will be caught and re-wrapped below.
throw new Error("ENOTFOUND");
}
// Check all resolved addresses to prevent DNS rebinding attacks
for (const { address } of addresses) {
if (!isSafeIp(address, allowPrivate)) {
throw new Error("Invalid or unsafe URL");
}
}
// Use the first resolved address for HTTP pinning
address = addresses[0].address;
family = addresses[0].family;
} catch (error) {
if (error instanceof Error && error.message === "Invalid or unsafe URL") {
throw error;
}
throw new Error(`Failed to resolve hostname: ${hostname}`);
}| viewMode === "grid" ? "md:grid-cols-2 lg:grid-cols-3" : "grid-cols-1" | ||
| )} | ||
| > | ||
| {items.map((item) => |
There was a problem hiding this comment.
The component is vulnerable to stored Cross-Site Scripting (XSS). The item.link value, which originates from an external RSS feed, is used directly in an href attribute on line 140 without validation. If an attacker provides a malicious RSS feed with a javascript: URL, the script will execute in the user's browser when the link is clicked. This can lead to session token theft or other client-side attacks.
Remediation:
Validate the item.link URL to ensure it uses a safe protocol (e.g., http: or https:). A safeUrl utility function, similar to the one in client/src/pages/xrel-releases.tsx, should be used before rendering the link.
Example usage: href={safeUrl(item.link)}
| it("should block hostnames that resolve to both safe and unsafe IPs (DNS Rebinding)", async () => { | ||
| // Mock DNS lookup to return both a safe public IP and an unsafe loopback IP | ||
| vi.mocked(dns.lookup).mockResolvedValueOnce([ | ||
| { address: "142.250.185.46", family: 4 }, // safe | ||
| { address: "127.0.0.1", family: 4 }, // unsafe if allowPrivate: false | ||
| ]); | ||
|
|
||
| expect(await isSafeUrl("http://rebinding-attack.com", { allowPrivate: false })).toBe(false); | ||
| }); |
There was a problem hiding this comment.
The new tests for DNS rebinding are great. To improve test coverage for the edge case I pointed out in server/ssrf.ts, consider adding test cases for when dns.lookup resolves to an empty array.
For isSafeUrl, you could add:
it("should return false if hostname resolves to no IPs", async () => {
vi.mocked(dns.lookup).mockResolvedValueOnce([]);
expect(await isSafeUrl("http://empty-resolve.com")).toBe(false);
});And for safeFetch:
it("should reject if hostname resolves to no IPs", async () => {
vi.mocked(dns.lookup).mockResolvedValueOnce([]);
await expect(safeFetch("http://empty-resolve.com")).rejects.toThrow(
"Failed to resolve hostname"
);
});1 participant
🚨 Severity: HIGH
💡 Vulnerability: SSRF Time-of-Check Time-of-Use (TOCTOU) via DNS Rebinding. The previous implementation checked only the first resolved IP address. An attacker could configure a domain to resolve to both a safe IP (passing the check) and an unsafe IP (accessed by fetch), or rebind the DNS between check and use.
🎯 Impact: Attackers could bypass SSRF protection to access internal services (e.g., localhost, metadata services) by using a domain that resolves to mixed safe/unsafe IPs.
🔧 Fix: Updated
isSafeUrlandsafeFetchinserver/ssrf.tsto usedns.lookup({ all: true })and validate all resolved IP addresses. If any resolved IP is unsafe, the request is blocked.✅ Verification: Added a new test case in
server/__tests__/ssrf.test.tsthat mocksdns.lookupreturning both safe and unsafe IPs, confirming the function now returnsfalse/throws error.PR created automatically by Jules for task 6441558472011113379 started by @Doezer