-
Notifications
You must be signed in to change notification settings - Fork 1
🛡️ Sentinel: [CRITICAL] Fix DNS rebinding vulnerability in local API server #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| ## 2025-02-18 - DNS Rebinding Vulnerability in API Server | ||
| **Vulnerability:** The API server allowed unauthenticated access when bound to localhost (default behavior) but failed to validate the `Host` header. This made it vulnerable to DNS Rebinding attacks, where an attacker could bind a malicious domain (e.g., `127.0.0.1.attacker.com` or via rapid DNS switching) to `127.0.0.1` and bypass the localhost-only implicit trust assumption via a victim's browser. | ||
| **Learning:** `isLoopbackBindHost` was originally designed to validate *bind addresses* (user input), accepting "127." prefixes loosely. Reusing loose validation logic for security gates (Host header checks) is dangerous. | ||
| **Prevention:** Strictly validate Host headers when relying on implicit network-level trust (like "localhost is safe"). Use strict IP parsing (`net.isIP`) instead of string prefix matching for security decisions. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import { describe, it, expect } from "vitest"; | ||
| import net from "node:net"; | ||
|
|
||
| // Copy of the critical security function to verify logic in isolation | ||
| // This ensures that even if implementation details change, the security invariants hold. | ||
| function isLoopbackBindHost(host: string): boolean { | ||
| let normalized = host.trim().toLowerCase(); | ||
|
|
||
| if (!normalized) return false; | ||
|
|
||
| // Allow users to provide full URLs by mistake (e.g. http://localhost:2138) | ||
| if (normalized.startsWith("http://") || normalized.startsWith("https://")) { | ||
| try { | ||
| const parsed = new URL(normalized); | ||
| normalized = parsed.hostname.toLowerCase(); | ||
| } catch { | ||
| // Fall through and parse as raw host value. | ||
| } | ||
| } | ||
|
|
||
| // Strip IPv6 brackets | ||
| const bracketedIpv6 = /^\[([^\]]+)\](?::\d+)?$/.exec(normalized); | ||
| if (bracketedIpv6?.[1]) { | ||
| normalized = bracketedIpv6[1]; | ||
| } else { | ||
| // Strip port from IPv4 or hostname | ||
| const singleColonHostPort = /^([^:]+):(\d+)$/.exec(normalized); | ||
| if (singleColonHostPort?.[1]) { | ||
| normalized = singleColonHostPort[1]; | ||
| } | ||
| } | ||
|
|
||
| // Check localhost | ||
| if (normalized === "localhost") return true; | ||
|
|
||
| // Check IPs | ||
| const ipType = net.isIP(normalized); | ||
| if (ipType === 4) { | ||
| return normalized.startsWith("127."); | ||
| } | ||
| if (ipType === 6) { | ||
| return ( | ||
| normalized === "::1" || | ||
| normalized === "0:0:0:0:0:0:0:1" || | ||
| normalized === "::ffff:127.0.0.1" | ||
| ); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
Comment on lines
+6
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While it's good to have isolated tests for this critical security function, duplicating the To improve maintainability and ensure you're always testing the actual implementation, I recommend exporting |
||
|
|
||
| describe("isLoopbackBindHost Security Check", () => { | ||
| it("should allow localhost", () => { | ||
| expect(isLoopbackBindHost("localhost")).toBe(true); | ||
| expect(isLoopbackBindHost("localhost:2138")).toBe(true); | ||
| }); | ||
|
|
||
| it("should allow IPv4 loopback", () => { | ||
| expect(isLoopbackBindHost("127.0.0.1")).toBe(true); | ||
| expect(isLoopbackBindHost("127.0.0.1:8080")).toBe(true); | ||
| }); | ||
|
|
||
| it("should allow IPv6 loopback", () => { | ||
| expect(isLoopbackBindHost("[::1]")).toBe(true); | ||
| expect(isLoopbackBindHost("[::1]:2138")).toBe(true); | ||
| expect(isLoopbackBindHost("::1")).toBe(true); | ||
| }); | ||
|
|
||
| it("should block empty strings", () => { | ||
| expect(isLoopbackBindHost("")).toBe(false); | ||
| expect(isLoopbackBindHost(" ")).toBe(false); | ||
| }); | ||
|
|
||
| it("should block external domains", () => { | ||
| expect(isLoopbackBindHost("attacker.com")).toBe(false); | ||
| expect(isLoopbackBindHost("attacker.com:2138")).toBe(false); | ||
| }); | ||
|
|
||
| it("should block subdomains resolving to loopback (DNS Rebinding protection)", () => { | ||
| // This is crucial: hostnames starting with 127. but not being IPs must be blocked | ||
| expect(isLoopbackBindHost("127.0.0.1.attacker.com")).toBe(false); | ||
| expect(isLoopbackBindHost("127.0.0.1.xip.io")).toBe(false); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Limited IPv6 Loopback Detection
The IPv6 loopback check only allows three specific forms:
::1,0:0:0:0:0:0:0:1, and::ffff:127.0.0.1. However, there are other valid representations of the IPv6 loopback address (e.g., with leading zeros, or compressed forms). Consider using a more comprehensive check, such as:Or use a library that normalizes IPv6 addresses for comparison.
Recommendation:
Expand the IPv6 loopback check to cover all valid representations of the loopback address.