From 5d3c659d0517ffb89a5b94cff610be3d18b067de Mon Sep 17 00:00:00 2001 From: Chris Sherwood Date: Sun, 8 Mar 2026 21:44:11 -0700 Subject: [PATCH] fix(security): narrow SSRF scope to allow RFC1918 LAN addresses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NOMAD is a LAN appliance — blocking RFC1918 private ranges (10.x, 172.16-31.x, 192.168.x) would prevent users from downloading content from local network mirrors. Narrowed to only block loopback (localhost, 127.x, 0.0.0.0, ::1) and link-local (169.254.x, fe80::) addresses. Restored require_tld: false for LAN hostnames without TLDs. Co-Authored-By: Claude Opus 4.6 --- admin/app/validators/common.ts | 34 ++++++++++++------------ admin/docs/security-audit-v1.md | 46 ++++++++++----------------------- 2 files changed, 31 insertions(+), 49 deletions(-) diff --git a/admin/app/validators/common.ts b/admin/app/validators/common.ts index 27a441c..024c206 100644 --- a/admin/app/validators/common.ts +++ b/admin/app/validators/common.ts @@ -1,31 +1,31 @@ import vine from '@vinejs/vine' /** - * Checks whether a URL points to a private/internal network address. - * Used to prevent SSRF — the server should not fetch from localhost, - * private RFC1918 ranges, link-local, or cloud metadata endpoints. + * Checks whether a URL points to a loopback or link-local address. + * Used to prevent SSRF — the server should not fetch from localhost + * or link-local/metadata endpoints (e.g. cloud instance metadata at 169.254.x.x). * - * Throws an error if the URL is internal/private. + * RFC1918 private ranges (10.x, 172.16-31.x, 192.168.x) are intentionally + * ALLOWED because NOMAD is a LAN appliance and users may host content + * mirrors on their local network. + * + * Throws an error if the URL is a loopback or link-local address. */ export function assertNotPrivateUrl(urlString: string): void { const parsed = new URL(urlString) const hostname = parsed.hostname.toLowerCase() - const privatePatterns = [ + const blockedPatterns = [ /^localhost$/, /^127\.\d+\.\d+\.\d+$/, - /^10\.\d+\.\d+\.\d+$/, - /^172\.(1[6-9]|2\d|3[01])\.\d+\.\d+$/, - /^192\.168\.\d+\.\d+$/, - /^169\.254\.\d+\.\d+$/, // Link-local / cloud metadata /^0\.0\.0\.0$/, + /^169\.254\.\d+\.\d+$/, // Link-local / cloud metadata /^\[::1\]$/, - /^\[?fe80:/i, - /^\[?fd[0-9a-f]{2}:/i, // Unique local IPv6 + /^\[?fe80:/i, // IPv6 link-local ] - if (privatePatterns.some((re) => re.test(hostname))) { - throw new Error(`Download URL must not point to a private/internal address: ${hostname}`) + if (blockedPatterns.some((re) => re.test(hostname))) { + throw new Error(`Download URL must not point to a loopback or link-local address: ${hostname}`) } } @@ -33,7 +33,7 @@ export const remoteDownloadValidator = vine.compile( vine.object({ url: vine .string() - .url() + .url({ require_tld: false }) // Allow LAN URLs (e.g. http://my-nas:8080/file.zim) .trim(), }) ) @@ -42,7 +42,7 @@ export const remoteDownloadWithMetadataValidator = vine.compile( vine.object({ url: vine .string() - .url() + .url({ require_tld: false }) // Allow LAN URLs .trim(), metadata: vine .object({ @@ -59,7 +59,7 @@ export const remoteDownloadValidatorOptional = vine.compile( vine.object({ url: vine .string() - .url() + .url({ require_tld: false }) // Allow LAN URLs .trim() .optional(), }) @@ -97,7 +97,7 @@ const resourceUpdateInfoBase = vine.object({ resource_type: vine.enum(['zim', 'map'] as const), installed_version: vine.string().trim(), latest_version: vine.string().trim().minLength(1), - download_url: vine.string().url().trim(), + download_url: vine.string().url({ require_tld: false }).trim(), }) export const applyContentUpdateValidator = vine.compile(resourceUpdateInfoBase) diff --git a/admin/docs/security-audit-v1.md b/admin/docs/security-audit-v1.md index 9afa638..9638df0 100644 --- a/admin/docs/security-audit-v1.md +++ b/admin/docs/security-audit-v1.md @@ -109,47 +109,29 @@ if (!fullPath.startsWith(path.resolve(basePath))) { ### 4. SSRF — Download Endpoints (HIGH) -**File:** `admin/app/validators/common.ts:3-12` +**File:** `admin/app/validators/common.ts` **Endpoints:** `POST /api/zim/download-remote`, `POST /api/maps/download-remote`, `POST /api/maps/download-base-assets`, `POST /api/maps/download-remote-preflight` -The URL validator uses `require_tld: false`, allowing internal/private URLs: - -```typescript -url: vine.string().url({ require_tld: false }) -``` - -An attacker on the LAN can make NOMAD fetch from: +The download endpoints accept user-supplied URLs and the server fetches from them. Without validation, an attacker on the LAN (or via CSRF since `shield.ts` disables CSRF protection) could make NOMAD fetch from co-located services: - `http://localhost:3306` (MySQL) - `http://localhost:6379` (Redis) - `http://169.254.169.254/` (cloud metadata — if NOMAD is ever cloud-hosted) -- Any internal network service -**Fix:** Add a URL validation helper that rejects private/internal IPs: +**Fix:** Added `assertNotPrivateUrl()` that blocks loopback and link-local addresses before any download is initiated. Called in all download controllers. + +**Scope note:** RFC1918 private addresses (10.x, 172.16-31.x, 192.168.x) are intentionally **allowed** because NOMAD is a LAN appliance and users may host content mirrors on their local network. The `require_tld: false` VineJS option is preserved so URLs like `http://my-nas:8080/file.zim` remain valid. ```typescript -function isPrivateUrl(urlString: string): boolean { - const url = new URL(urlString) - const hostname = url.hostname - // Block localhost, private ranges, link-local, metadata - const blocked = [ - /^localhost$/i, - /^127\./, - /^10\./, - /^172\.(1[6-9]|2\d|3[01])\./, - /^192\.168\./, - /^169\.254\./, - /^0\./, - /^\[::1\]$/, - /^\[fe80:/i, - ] - return blocked.some(re => re.test(hostname)) -} +const blockedPatterns = [ + /^localhost$/, + /^127\.\d+\.\d+\.\d+$/, + /^0\.0\.0\.0$/, + /^169\.254\.\d+\.\d+$/, // Link-local / cloud metadata + /^\[::1\]$/, + /^\[?fe80:/i, // IPv6 link-local +] ``` -Alternatively, maintain an allowlist of known-good download domains (e.g., `download.kiwix.org`, `github.com`, `api.projectnomad.us`). - -**Note:** The `require_tld: false` setting was intentionally added with the comment "Allow local URLs" — but this should only be needed for dev/testing. Consider making it configurable via environment variable, or keeping the allowlist approach for production. - --- ## FIX AFTER LAUNCH (Medium Priority) @@ -186,7 +168,7 @@ The `updateSetting` endpoint validates the key against an enum, but `getSetting` The `download_url` comes directly from the client request body. An attacker can supply any URL and NOMAD will download from it. The URL should be looked up server-side from the content manifest instead. -**Fix:** Validate `download_url` against the cached manifest, or apply the same SSRF protections as finding #4. +**Fix:** Validate `download_url` against the cached manifest, or apply the same loopback/link-local protections as finding #4 (already applied in this PR). ---