From 486f01710cdf9a05f85e7e8552484d0b727fe414 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Tue, 23 Jun 2026 21:58:52 -0300 Subject: [PATCH] fix(security): don't trust loopback socket as local when behind reverse proxy (#4632) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Integrated into release/v3.8.36 — port rebuilt clean, release-green --- scripts/dev/peer-stamp.mjs | 31 ++++- src/server/authz/headers.ts | 16 +++ src/server/authz/peerStamp.ts | 65 ++++++++++ src/server/authz/pipeline.ts | 28 +++-- src/server/authz/policies/management.ts | 25 +++- .../route-guard-loopback-via-proxy.test.ts | 112 ++++++++++++++++++ 6 files changed, 260 insertions(+), 17 deletions(-) create mode 100644 tests/unit/route-guard-loopback-via-proxy.test.ts diff --git a/scripts/dev/peer-stamp.mjs b/scripts/dev/peer-stamp.mjs index b4e18d213a..4951abcbdb 100644 --- a/scripts/dev/peer-stamp.mjs +++ b/scripts/dev/peer-stamp.mjs @@ -20,6 +20,20 @@ import { randomUUID } from "node:crypto"; */ export const PEER_IP_HEADER = "x-omniroute-peer-ip"; +/** + * Companion header to PEER_IP_HEADER: `|1` when the inbound TCP request + * carried forwarding headers (`x-forwarded-for` / `x-real-ip`), `|0` + * otherwise. Required so the middleware can tell that a loopback socket is the + * reverse-proxy hop (nginx / Caddy / Cloudflare Tunnel) and NOT trust it as + * local — without this, a leaked JWT over a public tunnel would reach the + * LOCAL_ONLY routes that spawn child processes (Hard Rules #15 + #17; + * port of upstream decolua/9router commit da667836). + * + * Keep VIA_PROXY_HEADER in sync with VIA_PROXY_HEADER in + * src/server/authz/headers.ts (the TS side cannot import this .mjs). + */ +export const VIA_PROXY_HEADER = "x-omniroute-via-proxy"; + /** Generate (once) and return the per-process stamp token, persisting it in env * so the middleware running in the same process reads the identical value. */ export function ensurePeerStampToken() { @@ -27,17 +41,26 @@ export function ensurePeerStampToken() { return process.env.OMNIROUTE_PEER_STAMP_TOKEN; } -/** Strip any client-supplied PEER_IP_HEADER and stamp the real TCP peer IP, - * token-prefixed. Never throws — a stamping failure must not block a request - * (it degrades to "locality unknown" → fail closed in the middleware). */ +/** Strip any client-supplied PEER_IP_HEADER + VIA_PROXY_HEADER and stamp the + * real TCP peer IP plus a token-protected via-proxy marker. Never throws — a + * stamping failure must not block a request (it degrades to "locality + * unknown" → fail closed in the middleware). */ export function stampPeerIp(req) { try { if (!req || !req.headers) return; // Node lowercases incoming header names; delete kills any client value. delete req.headers[PEER_IP_HEADER]; + delete req.headers[VIA_PROXY_HEADER]; const ip = req.socket && req.socket.remoteAddress; if (ip) { - req.headers[PEER_IP_HEADER] = `${ensurePeerStampToken()}|${ip}`; + const token = ensurePeerStampToken(); + req.headers[PEER_IP_HEADER] = `${token}|${ip}`; + // Forwarding headers present = request arrived via a reverse proxy; the + // loopback socket is the proxy hop, not the end-user, so it must not be + // trusted as local. Token-prefix the marker so a remote caller cannot + // forge it (or its absence) on a non-proxied request. + const viaProxy = !!(req.headers["x-forwarded-for"] || req.headers["x-real-ip"]); + req.headers[VIA_PROXY_HEADER] = `${token}|${viaProxy ? "1" : "0"}`; } } catch { /* never block a request on peer stamping */ diff --git a/src/server/authz/headers.ts b/src/server/authz/headers.ts index a7a3ec574e..002e679739 100644 --- a/src/server/authz/headers.ts +++ b/src/server/authz/headers.ts @@ -37,6 +37,22 @@ export const CLI_TOKEN_HEADER = "x-omniroute-cli-token"; */ export const PEER_IP_HEADER = "x-omniroute-peer-ip"; +/** + * Trusted "request arrived via a reverse proxy" marker stamped by the custom + * Node server alongside PEER_IP_HEADER, formatted as `|1` when the + * inbound TCP request carried forwarding headers (`x-forwarded-for` / + * `x-real-ip`) and `|0` otherwise. The middleware combines this with + * the stamped peer IP so a loopback / private-LAN socket that is actually the + * proxy hop (e.g. OmniRoute behind nginx / Caddy / Cloudflare Tunnel) is NOT + * trusted as local — closing the upstream da667836 vulnerability that would + * otherwise let a leaked JWT over a public tunnel reach LOCAL_ONLY routes + * that spawn child processes. Token-validated like PEER_IP_HEADER, so a + * remote caller cannot forge it. Stripped from forwarded headers before + * route handlers see it. + * Keep in sync with VIA_PROXY_HEADER in scripts/dev/peer-stamp.mjs. + */ +export const VIA_PROXY_HEADER = "x-omniroute-via-proxy"; + /** * Trusted locality verdict ("loopback" | "lan" | "remote") that the pipeline * computes from the stamped real peer IP and forwards to route handlers. Route diff --git a/src/server/authz/peerStamp.ts b/src/server/authz/peerStamp.ts index df099aa94c..ba80df0189 100644 --- a/src/server/authz/peerStamp.ts +++ b/src/server/authz/peerStamp.ts @@ -1,4 +1,5 @@ import { timingSafeEqual } from "node:crypto"; +import { classifyHostLocality } from "./routeGuard"; /** * Resolve the real peer IP from the trusted `|` stamp that the custom @@ -32,3 +33,67 @@ export function resolveStampedPeer( } return ip; } + +/** + * Resolve the trusted "request arrived via a reverse proxy" marker stamped by + * the custom Node server (`scripts/dev/peer-stamp.mjs::stampPeerIp`). The stamp + * is `|1` when forwarding headers (`x-forwarded-for` / `x-real-ip`) were + * present on the inbound TCP request, and `|0` otherwise. + * + * Returns true ONLY when the token constant-time-matches this process's stamp + * token AND the payload is exactly "1". Any other value — no stamp, forged + * token, "0", junk — returns false (the safe default: assume no proxy hop). + * + * SECURITY: paired with `resolveStampedPeer()` to close the upstream + * decolua/9router da667836 vulnerability — when OmniRoute itself runs behind + * an external reverse proxy (nginx / Caddy / Cloudflare Tunnel), + * `req.socket.remoteAddress` is the proxy hop (usually 127.0.0.1), not the + * end-user. Without this signal, `classifyHostLocality()` would return + * "loopback" for every remote caller arriving via the proxy, granting access + * to the LOCAL_ONLY tier that gates spawn-capable routes (Hard Rules #15 + + * #17). Consume via `classifyStampedPeerLocality()` below. + */ +export function resolveStampedViaProxy( + headerValue: string | null, + token: string | undefined +): boolean { + if (!headerValue || !token) return false; + const sep = headerValue.indexOf("|"); + if (sep <= 0) return false; + const provided = headerValue.slice(0, sep); + const payload = headerValue.slice(sep + 1); + if (provided.length !== token.length) return false; + try { + if (!timingSafeEqual(Buffer.from(provided), Buffer.from(token))) return false; + } catch { + return false; + } + return payload === "1"; +} + +/** + * The trusted locality verdict consumed by the LOCAL_ONLY route guard. Wraps + * `resolveStampedPeer()` + `resolveStampedViaProxy()` + `classifyHostLocality()` + * so the pipeline has a single boundary helper: + * + * 1. Resolve the real peer IP from PEER_IP_HEADER (or fail closed → remote). + * 2. If the via-proxy marker is present, the loopback / private-LAN socket + * is the proxy hop, not the end-user — downgrade to "remote". + * (Public-IP sockets are already remote, so the marker is a no-op there.) + * 3. Otherwise classify the raw IP normally (loopback / lan / remote). + * + * Pure; both header values are token-validated, so an attacker who knows the + * header names but not the per-process token cannot influence the verdict in + * any direction. + */ +export function classifyStampedPeerLocality( + peerHeader: string | null, + viaProxyHeader: string | null, + token: string | undefined +): "loopback" | "lan" | "remote" { + const ip = resolveStampedPeer(peerHeader, token); + const viaProxy = resolveStampedViaProxy(viaProxyHeader, token); + const locality = classifyHostLocality(ip); + if (viaProxy && locality !== "remote") return "remote"; + return locality; +} diff --git a/src/server/authz/pipeline.ts b/src/server/authz/pipeline.ts index 178ac5dd10..81b039b497 100644 --- a/src/server/authz/pipeline.ts +++ b/src/server/authz/pipeline.ts @@ -6,8 +6,7 @@ import { checkBodySize, getBodySizeLimit } from "../../shared/middleware/bodySiz import { generateRequestId } from "../../shared/utils/requestId"; import { applyCorsHeaders } from "../cors/origins"; import { classifyRoute } from "./classify"; -import { classifyHostLocality } from "./routeGuard"; -import { resolveStampedPeer } from "./peerStamp"; +import { classifyStampedPeerLocality } from "./peerStamp"; import { clientApiPolicy } from "./policies/clientApi"; import { managementPolicy } from "./policies/management"; import { publicPolicy } from "./policies/public"; @@ -21,6 +20,7 @@ import { AUTHZ_HEADER_ROUTE_CLASS, AUTHZ_TRUSTED_HEADERS, PEER_IP_HEADER, + VIA_PROXY_HEADER, } from "./headers"; import type { AuthSubject, RouteClass, RouteClassification } from "./types"; import type { AuthOutcome, RoutePolicy } from "./context"; @@ -232,21 +232,29 @@ export async function runAuthzPipeline( for (const trusted of AUTHZ_TRUSTED_HEADERS) { requestHeaders.delete(trusted); } - // The trusted peer-IP stamp is read by the policy from the ORIGINAL request - // (above); strip it from the forwarded headers so the per-process token never - // reaches route handlers or upstream providers. + // The trusted peer-IP + via-proxy stamps are read by the policy from the + // ORIGINAL request (above); strip them from the forwarded headers so the + // per-process token never reaches route handlers or upstream providers. requestHeaders.delete(PEER_IP_HEADER); + requestHeaders.delete(VIA_PROXY_HEADER); requestHeaders.set(AUTHZ_HEADER_ROUTE_CLASS, classification.routeClass); requestHeaders.set(AUTHZ_HEADER_REQUEST_ID, requestId); // Stamp a trusted, non-secret locality verdict derived from the real stamped - // peer IP. Route handlers (e.g. cliTokenAuth) read this instead of re-deriving - // locality from the spoofable Host header. The client-supplied value (if any) - // was already removed by the AUTHZ_TRUSTED_HEADERS strip above. + // peer IP AND the via-proxy marker. Route handlers (e.g. cliTokenAuth) read + // this instead of re-deriving locality from the spoofable Host header. The + // client-supplied values (if any) were already removed by the + // AUTHZ_TRUSTED_HEADERS strip above. When the via-proxy marker is set, a + // loopback socket is the proxy hop, not the end-user — verdict is downgraded + // to "remote" so the LOCAL_ONLY gate is not bypassed by a request arriving + // through an external reverse proxy (nginx / Caddy / Cloudflare Tunnel). + // See peerStamp.ts and the upstream da667836 reference for the full rationale. requestHeaders.set( AUTHZ_HEADER_PEER_LOCALITY, - classifyHostLocality( - resolveStampedPeer(request.headers.get(PEER_IP_HEADER), process.env.OMNIROUTE_PEER_STAMP_TOKEN) + classifyStampedPeerLocality( + request.headers.get(PEER_IP_HEADER), + request.headers.get(VIA_PROXY_HEADER), + process.env.OMNIROUTE_PEER_STAMP_TOKEN ) ); diff --git a/src/server/authz/policies/management.ts b/src/server/authz/policies/management.ts index 42325876ae..9b518c35dd 100644 --- a/src/server/authz/policies/management.ts +++ b/src/server/authz/policies/management.ts @@ -8,8 +8,8 @@ import { extractApiKey, isValidApiKey } from "../../../sse/services/auth"; import { getApiKeyMetadata } from "../../../lib/db/apiKeys"; import { hasManageScope } from "../../../lib/api/requireManagementAuth"; import { evaluateAccessTokenAuth } from "../accessTokenAuth"; -import { CLI_TOKEN_HEADER, PEER_IP_HEADER } from "../headers"; -import { resolveStampedPeer } from "../peerStamp"; +import { CLI_TOKEN_HEADER, PEER_IP_HEADER, VIA_PROXY_HEADER } from "../headers"; +import { resolveStampedPeer, resolveStampedViaProxy } from "../peerStamp"; import { isAlwaysProtectedPath, isLocalOnlyBypassableByManageScope, @@ -35,15 +35,34 @@ function requestPeerAddress(ctx: PolicyContext): string | null { return ctx.request.ip ?? ctx.request.socket?.remoteAddress ?? null; } +/** + * True when the inbound TCP request carried forwarding headers + * (`x-forwarded-for` / `x-real-ip`), as stamped by the custom Node server. When + * set, the socket peer is the reverse-proxy hop, not the end-user — so a + * loopback / private-LAN socket must NOT be trusted as local (Hard Rules #15 + + * #17, port of decolua/9router da667836). Token-validated; an attacker who + * knows the header name but not the per-process token cannot influence it. + */ +function isViaProxyRequest(ctx: PolicyContext): boolean { + return resolveStampedViaProxy( + ctx.request.headers?.get?.(VIA_PROXY_HEADER) ?? null, + process.env.OMNIROUTE_PEER_STAMP_TOKEN + ); +} + function isLoopbackRequest(ctx: PolicyContext): boolean { + if (isViaProxyRequest(ctx)) return false; const peerAddress = requestPeerAddress(ctx); return peerAddress ? isLoopbackHost(peerAddress) : false; } // Owner-authorized (2026-05-30): allow LOCAL_ONLY *paths* from a trusted private // LAN, based on the real socket peer IP (not spoofable). Does NOT relax the -// CLI-token gate, which stays strictly loopback. +// CLI-token gate, which stays strictly loopback. Also falls back to "not LAN" +// when a reverse-proxy hop is detected (the apparent LAN IP would be the proxy, +// not the end-user — see isViaProxyRequest above). function isPrivateLanRequest(ctx: PolicyContext): boolean { + if (isViaProxyRequest(ctx)) return false; const peerAddress = requestPeerAddress(ctx); return peerAddress ? isPrivateLanHost(peerAddress) : false; } diff --git a/tests/unit/route-guard-loopback-via-proxy.test.ts b/tests/unit/route-guard-loopback-via-proxy.test.ts new file mode 100644 index 0000000000..77552ec260 --- /dev/null +++ b/tests/unit/route-guard-loopback-via-proxy.test.ts @@ -0,0 +1,112 @@ +/** + * Security regression: when OmniRoute itself runs behind an external reverse + * proxy (nginx / Caddy / Cloudflare Tunnel), `req.socket.remoteAddress` is the + * proxy hop — usually 127.0.0.1 — not the real end-user. + * + * Previously, the custom server stamped the loopback socket as the trusted + * peer IP, so `classifyHostLocality()` returned "loopback" for every remote + * caller arriving via the proxy → the LOCAL_ONLY route guard (which gates + * spawn-capable routes like `/api/cli-tools/runtime/*`, `/api/services/*`, + * `/api/plugins/*`, `/api/system/version`) was effectively bypassed. A leaked + * JWT over the public tunnel could trigger child-process spawning. + * + * Fix (mirrors upstream decolua/9router commit da667836): the custom server + * detects forwarding headers (`x-forwarded-for` / `x-real-ip`) and stamps a + * token-protected `via-proxy` marker. When the marker is present, locality + * derived from a loopback socket is downgraded to "remote" (fail closed). + * + * Hard Rules #15, #17 + Rule #18 (TDD before fix). + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import { + resolveStampedPeer, + resolveStampedViaProxy, + classifyStampedPeerLocality, +} from "../../src/server/authz/peerStamp.ts"; + +const TOK = "process-secret-token-abc"; + +test("resolveStampedViaProxy: returns true only for the correctly-tokened stamp", () => { + assert.equal(resolveStampedViaProxy(`${TOK}|1`, TOK), true); + assert.equal(resolveStampedViaProxy(`${TOK}|0`, TOK), false); + assert.equal(resolveStampedViaProxy(null, TOK), false); + assert.equal(resolveStampedViaProxy("", TOK), false); +}); + +test("resolveStampedViaProxy: rejects forged / un-tokened values", () => { + assert.equal(resolveStampedViaProxy("1", TOK), false, "raw client value (no token)"); + assert.equal(resolveStampedViaProxy("wrong-token|1", TOK), false, "forged token"); + assert.equal(resolveStampedViaProxy(`${TOK}|1`, undefined), false, "no process token"); +}); + +test("classifyStampedPeerLocality: loopback socket WITHOUT a proxy stamp stays loopback", () => { + // Local CLI / dashboard hit — the normal happy path. + assert.equal( + classifyStampedPeerLocality(`${TOK}|127.0.0.1`, null, TOK), + "loopback" + ); + assert.equal(classifyStampedPeerLocality(`${TOK}|::1`, null, TOK), "loopback"); +}); + +test("classifyStampedPeerLocality: loopback socket WITH a proxy stamp is REMOTE (fail closed)", () => { + // OmniRoute is behind nginx/Caddy/Cloudflare; the socket peer is the proxy. + // The real end-user is somewhere on the public internet → must not be trusted + // as local, otherwise the LOCAL_ONLY spawn-capable surface is reachable from + // a tunnel. + assert.equal( + classifyStampedPeerLocality(`${TOK}|127.0.0.1`, `${TOK}|1`, TOK), + "remote", + "loopback socket + via-proxy stamp must NOT be classified as local" + ); + assert.equal( + classifyStampedPeerLocality(`${TOK}|::1`, `${TOK}|1`, TOK), + "remote" + ); + assert.equal( + classifyStampedPeerLocality(`${TOK}|::ffff:127.0.0.1`, `${TOK}|1`, TOK), + "remote" + ); +}); + +test("classifyStampedPeerLocality: private-LAN socket WITH a proxy stamp is still REMOTE", () => { + // Caddy/nginx running on a LAN box in front of OmniRoute. We do not know how + // the proxy is exposed (it could be tunneled to the public internet), so any + // proxy hop downgrades locality to remote. + assert.equal( + classifyStampedPeerLocality(`${TOK}|192.168.0.15`, `${TOK}|1`, TOK), + "remote" + ); +}); + +test("classifyStampedPeerLocality: public-IP socket is remote regardless of stamp", () => { + assert.equal( + classifyStampedPeerLocality(`${TOK}|8.8.8.8`, null, TOK), + "remote" + ); + assert.equal( + classifyStampedPeerLocality(`${TOK}|8.8.8.8`, `${TOK}|1`, TOK), + "remote" + ); +}); + +test("classifyStampedPeerLocality: missing / forged peer stamp fails closed to remote", () => { + assert.equal(classifyStampedPeerLocality(null, null, TOK), "remote"); + assert.equal(classifyStampedPeerLocality("forged|127.0.0.1", null, TOK), "remote"); +}); + +test("classifyStampedPeerLocality: untrusted (un-tokened) via-proxy header is ignored", () => { + // A remote attacker who guesses the via-proxy header name but cannot mint the + // token must NOT be able to flip the locality verdict by themselves; the + // safety direction is OK (downgrade), but the inverse — pretending no proxy + // exists when one does — would only be exploitable if the attacker controlled + // BOTH headers, which the token gate prevents. We assert the bypass attempt + // (un-tokened via-proxy hint) does not leak into the verdict for the normal + // local-CLI case. + assert.equal( + classifyStampedPeerLocality(`${TOK}|127.0.0.1`, "1", TOK), + "loopback", + "un-tokened via-proxy hint is ignored (== false) — local CLI keeps loopback" + ); +});