From 95d284f687886123a461508d187a1042b84710fa Mon Sep 17 00:00:00 2001 From: Sucukdeluxe Date: Fri, 19 Jun 2026 17:10:11 +0200 Subject: [PATCH] Sicherheit: Allowlist anhand des echten Socket-Peers statt X-Forwarded-For Die Allowlist-Pruefung nutzte extractDebugClientIp, das X-Forwarded-For zuerst auswertet - ein angreiferkontrollierter Header. Damit konnte ein entfernter Client auf einem 0.0.0.0-Bind die Allowlist komplett umgehen (X-Forwarded-For: 127.0.0.1 -> als Loopback gewertet). Die Allowlist bot so keinerlei Schutz; nur das Token blieb. Fix: Enforcement liest jetzt ausschliesslich req.socket.remoteAddress (getPeerIp), das vom Kernel aus der TCP-Verbindung gesetzt wird und nicht per Header faelschbar ist. X-Forwarded-For wird nur noch fuer das Trace-Log (Beobachtbarkeit) verwendet, nie fuer die Zugriffsentscheidung. Reine, exportierte Matcher-Funktion evaluateClientAllowed. Tests umgebaut auf den Threat-Model statt den faelschbaren Kanal: Unit-Tests fuer Loopback/exakt/CIDR/fail-closed + expliziter Forged-XFF-Test (socket.remoteAddress 8.8.8.8 + X-Forwarded-For 127.0.0.1 -> verweigert). Integration: Loopback verbindet, gespooftes X-Forwarded-For wird ignoriert, Auth bleibt erzwungen, Live-Reload. --- src/main/debug-server.ts | 23 ++++-- tests/debug-server-allowlist.test.ts | 113 ++++++++++++++------------- tests/debug-server.test.ts | 1 - 3 files changed, 76 insertions(+), 61 deletions(-) diff --git a/src/main/debug-server.ts b/src/main/debug-server.ts index a26fa9d..cba58a1 100644 --- a/src/main/debug-server.ts +++ b/src/main/debug-server.ts @@ -219,15 +219,23 @@ function matchIpRule(clientIp: string, rule: string): boolean { return false; } -function isClientAllowed(clientIp: string): boolean { +export function evaluateClientAllowed(clientIp: string, rules: string[]): boolean { const client = normalizeIp(clientIp); if (isLoopbackIp(client) || client === "") { return true; } - if (allowlist.length === 0) { + if (rules.length === 0) { return false; } - return allowlist.some((rule) => matchIpRule(client, rule)); + return rules.some((rule) => matchIpRule(client, rule)); +} + +export function getPeerIp(req: http.IncomingMessage): string { + return normalizeIp(req.socket?.remoteAddress || ""); +} + +function isClientAllowed(clientIp: string): boolean { + return evaluateClientAllowed(clientIp, allowlist); } function checkAuth(req: http.IncomingMessage): boolean { @@ -551,15 +559,16 @@ function handleRequest(req: http.IncomingMessage, res: http.ServerResponse): voi return; } - const clientIp = extractDebugClientIp(req); - if (!isClientAllowed(clientIp)) { + const peerIp = getPeerIp(req); + if (!isClientAllowed(peerIp)) { if (traceConfig.enabled && traceConfig.logDebugRequests) { logTraceEvent("WARN", "debug-http", "Durch Allowlist blockiert", { - clientIp, + peerIp, + forwardedFor: extractDebugClientIp(req), url: sanitizeRequestUrlForTrace(req.url || "/") }); } - jsonResponse(res, 403, { error: "Forbidden", reason: "Client-IP nicht in Allowlist", clientIp }); + jsonResponse(res, 403, { error: "Forbidden", reason: "Client-IP nicht in Allowlist", clientIp: peerIp }); return; } diff --git a/tests/debug-server-allowlist.test.ts b/tests/debug-server-allowlist.test.ts index e91f6f9..59a2388 100644 --- a/tests/debug-server-allowlist.test.ts +++ b/tests/debug-server-allowlist.test.ts @@ -10,7 +10,9 @@ import { stopDebugServer, restartDebugServer, writeDebugServerConfig, - getDebugServerRuntimeStatus + getDebugServerRuntimeStatus, + evaluateClientAllowed, + getPeerIp } from "../src/main/debug-server"; import type { DownloadManager } from "../src/main/download-manager"; @@ -45,13 +47,13 @@ async function waitForReady(url: string): Promise { throw new Error(`debug server not ready: ${url}`); } -async function startWithAllowlist(allowlist: string[]): Promise<{ baseUrl: string }> { +async function startWithAllowlist(allowlist: string[], host = "0.0.0.0"): Promise<{ baseUrl: string }> { const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), "rd-allow-")); tempDirs.push(baseDir); const port = await getFreePort(); fs.writeFileSync(path.join(baseDir, "debug_token.txt"), TOKEN, "utf8"); fs.writeFileSync(path.join(baseDir, "debug_port.txt"), String(port), "utf8"); - fs.writeFileSync(path.join(baseDir, "debug_host.txt"), "0.0.0.0", "utf8"); + fs.writeFileSync(path.join(baseDir, "debug_host.txt"), host, "utf8"); fs.writeFileSync(path.join(baseDir, "debug_allowlist.txt"), allowlist.join("\n"), "utf8"); const manager = {} as unknown as DownloadManager; startDebugServer(manager, baseDir); @@ -60,14 +62,6 @@ async function startWithAllowlist(allowlist: string[]): Promise<{ baseUrl: strin return { baseUrl }; } -function getAs(baseUrl: string, clientIp: string | null): Promise { - const headers: Record = {}; - if (clientIp) { - headers["X-Forwarded-For"] = clientIp; - } - return fetch(`${baseUrl}/health?token=${TOKEN}`, { headers }); -} - afterEach(() => { stopDebugServer(); while (tempDirs.length > 0) { @@ -82,58 +76,71 @@ afterEach(() => { } }); -describe("debug-server allowlist enforcement", () => { - it("always allows loopback regardless of allowlist", async () => { +describe("debug-server allowlist matcher (pure)", () => { + it("always allows loopback regardless of rules", () => { + expect(evaluateClientAllowed("127.0.0.1", [])).toBe(true); + expect(evaluateClientAllowed("::1", [])).toBe(true); + expect(evaluateClientAllowed("::ffff:127.0.0.1", ["8.8.8.8"])).toBe(true); + }); + + it("matches an exact allowlisted IP and rejects others", () => { + expect(evaluateClientAllowed("8.8.8.8", ["8.8.8.8"])).toBe(true); + expect(evaluateClientAllowed("9.9.9.9", ["8.8.8.8"])).toBe(false); + }); + + it("matches inside a CIDR and rejects outside it", () => { + expect(evaluateClientAllowed("10.0.0.42", ["10.0.0.0/24"])).toBe(true); + expect(evaluateClientAllowed("10.0.1.42", ["10.0.0.0/24"])).toBe(false); + }); + + it("fail-closed: empty rules reject every non-loopback client", () => { + expect(evaluateClientAllowed("203.0.113.7", [])).toBe(false); + expect(evaluateClientAllowed("8.8.8.8", [])).toBe(false); + }); + + it("derives the client IP from the socket peer, never from X-Forwarded-For", () => { + const forgedLoopback = { + socket: { remoteAddress: "8.8.8.8" }, + headers: { "x-forwarded-for": "127.0.0.1" } + } as unknown as http.IncomingMessage; + expect(getPeerIp(forgedLoopback)).toBe("8.8.8.8"); + expect(evaluateClientAllowed(getPeerIp(forgedLoopback), [])).toBe(false); + expect(evaluateClientAllowed(getPeerIp(forgedLoopback), ["9.9.9.9"])).toBe(false); + expect(evaluateClientAllowed(getPeerIp(forgedLoopback), ["8.8.8.8"])).toBe(true); + + const ipv6Mapped = { + socket: { remoteAddress: "::ffff:10.0.0.5" }, + headers: {} + } as unknown as http.IncomingMessage; + expect(getPeerIp(ipv6Mapped)).toBe("10.0.0.5"); + }); +}); + +describe("debug-server allowlist enforcement (wired)", () => { + it("allows a loopback connection and ignores a spoofed X-Forwarded-For", async () => { const { baseUrl } = await startWithAllowlist(["8.8.8.8"]); - const res = await getAs(baseUrl, null); - expect(res.status).toBe(200); + const plain = await fetch(`${baseUrl}/health?token=${TOKEN}`); + expect(plain.status).toBe(200); + const spoofed = await fetch(`${baseUrl}/health?token=${TOKEN}`, { + headers: { "X-Forwarded-For": "203.0.113.9" } + }); + expect(spoofed.status).toBe(200); }); - it("allows an exact allowlisted client IP", async () => { + it("still enforces the token for loopback clients", async () => { const { baseUrl } = await startWithAllowlist(["8.8.8.8"]); - const res = await getAs(baseUrl, "8.8.8.8"); - expect(res.status).toBe(200); - }); - - it("allows a client inside an allowlisted CIDR and blocks outside it", async () => { - const { baseUrl } = await startWithAllowlist(["10.0.0.0/24"]); - const inside = await getAs(baseUrl, "10.0.0.42"); - expect(inside.status).toBe(200); - const outside = await getAs(baseUrl, "10.0.1.42"); - expect(outside.status).toBe(403); - }); - - it("blocks a non-allowlisted remote client with 403 before auth", async () => { - const { baseUrl } = await startWithAllowlist(["8.8.8.8"]); - const res = await fetch(`${baseUrl}/health`, { headers: { "X-Forwarded-For": "9.9.9.9" } }); - expect(res.status).toBe(403); - const body = await res.json() as Record; - expect(body.reason).toContain("Allowlist"); - }); - - it("fail-closed: empty allowlist on 0.0.0.0 blocks every non-loopback client", async () => { - const { baseUrl } = await startWithAllowlist([]); - expect(getDebugServerRuntimeStatus().allowlistCount).toBe(0); - const loopback = await getAs(baseUrl, null); - expect(loopback.status).toBe(200); - const remote = await getAs(baseUrl, "203.0.113.7"); - expect(remote.status).toBe(403); - }); - - it("still enforces auth for allowlisted clients", async () => { - const { baseUrl } = await startWithAllowlist(["8.8.8.8"]); - const res = await fetch(`${baseUrl}/health`, { headers: { "X-Forwarded-For": "8.8.8.8" } }); + const res = await fetch(`${baseUrl}/health`); expect(res.status).toBe(401); }); - it("applies a new allowlist live via restartDebugServer", async () => { + it("reloads the allowlist live via restartDebugServer", async () => { const { baseUrl } = await startWithAllowlist(["8.8.8.8"]); - expect((await getAs(baseUrl, "9.9.9.9")).status).toBe(403); - writeDebugServerConfig({ allowlist: ["9.9.9.9"] }); + expect(getDebugServerRuntimeStatus().allowlistCount).toBe(1); + writeDebugServerConfig({ allowlist: ["9.9.9.9", "10.0.0.0/24"] }); const status = await restartDebugServer(); expect(status.running).toBe(true); + expect(status.allowlistCount).toBe(2); await waitForReady(`${baseUrl}/health?token=${TOKEN}`); - expect((await getAs(baseUrl, "9.9.9.9")).status).toBe(200); - expect((await getAs(baseUrl, "8.8.8.8")).status).toBe(403); + expect((await fetch(`${baseUrl}/health?token=${TOKEN}`)).status).toBe(200); }); }); diff --git a/tests/debug-server.test.ts b/tests/debug-server.test.ts index e508b4a..4aa0ac8 100644 --- a/tests/debug-server.test.ts +++ b/tests/debug-server.test.ts @@ -202,7 +202,6 @@ async function createFixture() { fs.writeFileSync(path.join(baseDir, "debug_token.txt"), token, "utf8"); fs.writeFileSync(path.join(baseDir, "debug_port.txt"), String(port), "utf8"); fs.writeFileSync(path.join(baseDir, "debug_host.txt"), "0.0.0.0", "utf8"); - fs.writeFileSync(path.join(baseDir, "debug_allowlist.txt"), "159.195.63.46\n", "utf8"); const debridLinkApiKeys = "key-a\nkey-b"; const debridLinkKeyIds = getDebridLinkApiKeyIds(debridLinkApiKeys);