From c5d4fdd1420394438ff3545f9bd78d6f261a5e23 Mon Sep 17 00:00:00 2001 From: Chris Sherwood Date: Sun, 8 Mar 2026 21:34:56 -0700 Subject: [PATCH] fix(security): path traversal and SSRF protections from pre-launch audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes 4 high-severity findings from a comprehensive security audit: 1. Path traversal on ZIM file delete — resolve()+startsWith() containment 2. Path traversal on Map file delete — same pattern 3. Path traversal on docs read — same pattern (already used in rag_service) 4. SSRF on download endpoints — block private/internal IPs, require TLD Also adds assertNotPrivateUrl() to content update endpoints. Full audit report attached as admin/docs/security-audit-v1.md. Co-Authored-By: Claude Opus 4.6 --- .../collection_updates_controller.ts | 5 + admin/app/controllers/maps_controller.ts | 4 + admin/app/controllers/zim_controller.ts | 2 + admin/app/services/docs_service.ts | 11 +- admin/app/services/map_service.ts | 10 +- admin/app/services/zim_service.ts | 10 +- admin/app/validators/common.ts | 41 ++- admin/docs/security-audit-v1.md | 299 ++++++++++++++++++ 8 files changed, 367 insertions(+), 15 deletions(-) create mode 100644 admin/docs/security-audit-v1.md diff --git a/admin/app/controllers/collection_updates_controller.ts b/admin/app/controllers/collection_updates_controller.ts index a6cccdf..0e9140c 100644 --- a/admin/app/controllers/collection_updates_controller.ts +++ b/admin/app/controllers/collection_updates_controller.ts @@ -1,5 +1,6 @@ import { CollectionUpdateService } from '#services/collection_update_service' import { + assertNotPrivateUrl, applyContentUpdateValidator, applyAllContentUpdatesValidator, } from '#validators/common' @@ -13,12 +14,16 @@ export default class CollectionUpdatesController { async applyUpdate({ request }: HttpContext) { const update = await request.validateUsing(applyContentUpdateValidator) + assertNotPrivateUrl(update.download_url) const service = new CollectionUpdateService() return await service.applyUpdate(update) } async applyAllUpdates({ request }: HttpContext) { const { updates } = await request.validateUsing(applyAllContentUpdatesValidator) + for (const update of updates) { + assertNotPrivateUrl(update.download_url) + } const service = new CollectionUpdateService() return await service.applyAllUpdates(updates) } diff --git a/admin/app/controllers/maps_controller.ts b/admin/app/controllers/maps_controller.ts index 909cca7..8290d45 100644 --- a/admin/app/controllers/maps_controller.ts +++ b/admin/app/controllers/maps_controller.ts @@ -1,5 +1,6 @@ import { MapService } from '#services/map_service' import { + assertNotPrivateUrl, downloadCollectionValidator, filenameParamValidator, remoteDownloadValidator, @@ -25,12 +26,14 @@ export default class MapsController { async downloadBaseAssets({ request }: HttpContext) { const payload = await request.validateUsing(remoteDownloadValidatorOptional) + if (payload.url) assertNotPrivateUrl(payload.url) await this.mapService.downloadBaseAssets(payload.url) return { success: true } } async downloadRemote({ request }: HttpContext) { const payload = await request.validateUsing(remoteDownloadValidator) + assertNotPrivateUrl(payload.url) const filename = await this.mapService.downloadRemote(payload.url) return { message: 'Download started successfully', @@ -52,6 +55,7 @@ export default class MapsController { // For providing a "preflight" check in the UI before actually starting a background download async downloadRemotePreflight({ request }: HttpContext) { const payload = await request.validateUsing(remoteDownloadValidator) + assertNotPrivateUrl(payload.url) const info = await this.mapService.downloadRemotePreflight(payload.url) return info } diff --git a/admin/app/controllers/zim_controller.ts b/admin/app/controllers/zim_controller.ts index 17c692e..4bb00e3 100644 --- a/admin/app/controllers/zim_controller.ts +++ b/admin/app/controllers/zim_controller.ts @@ -1,5 +1,6 @@ import { ZimService } from '#services/zim_service' import { + assertNotPrivateUrl, downloadCategoryTierValidator, filenameParamValidator, remoteDownloadWithMetadataValidator, @@ -25,6 +26,7 @@ export default class ZimController { async downloadRemote({ request }: HttpContext) { const payload = await request.validateUsing(remoteDownloadWithMetadataValidator) + assertNotPrivateUrl(payload.url) const { filename, jobId } = await this.zimService.downloadRemote(payload.url) return { diff --git a/admin/app/services/docs_service.ts b/admin/app/services/docs_service.ts index 5669c2c..ef861a3 100644 --- a/admin/app/services/docs_service.ts +++ b/admin/app/services/docs_service.ts @@ -66,12 +66,19 @@ export class DocsService { const filename = _filename.endsWith('.md') ? _filename : `${_filename}.md` - const fileExists = await getFileStatsIfExists(path.join(this.docsPath, filename)) + // Prevent path traversal — resolved path must stay within the docs directory + const basePath = path.resolve(this.docsPath) + const fullPath = path.resolve(path.join(this.docsPath, filename)) + if (!fullPath.startsWith(basePath + path.sep)) { + throw new Error('Invalid document slug') + } + + const fileExists = await getFileStatsIfExists(fullPath) if (!fileExists) { throw new Error(`File not found: ${filename}`) } - const fileStream = await getFile(path.join(this.docsPath, filename), 'stream') + const fileStream = await getFile(fullPath, 'stream') if (!fileStream) { throw new Error(`Failed to read file stream: ${filename}`) } diff --git a/admin/app/services/map_service.ts b/admin/app/services/map_service.ts index 987c255..6f7cbfd 100644 --- a/admin/app/services/map_service.ts +++ b/admin/app/services/map_service.ts @@ -13,7 +13,7 @@ import { getFile, ensureDirectoryExists, } from '../utils/fs.js' -import { join } from 'path' +import { join, resolve, sep } from 'path' import urlJoin from 'url-join' import { RunDownloadJob } from '#jobs/run_download_job' import logger from '@adonisjs/core/services/logger' @@ -404,7 +404,13 @@ export class MapService implements IMapService { fileName += '.pmtiles' } - const fullPath = join(this.baseDirPath, 'pmtiles', fileName) + const basePath = resolve(join(this.baseDirPath, 'pmtiles')) + const fullPath = resolve(join(basePath, fileName)) + + // Prevent path traversal — resolved path must stay within the storage directory + if (!fullPath.startsWith(basePath + sep)) { + throw new Error('Invalid filename') + } const exists = await getFileStatsIfExists(fullPath) if (!exists) { diff --git a/admin/app/services/zim_service.ts b/admin/app/services/zim_service.ts index 1e8a55a..3eee1cb 100644 --- a/admin/app/services/zim_service.ts +++ b/admin/app/services/zim_service.ts @@ -16,7 +16,7 @@ import { listDirectoryContents, ZIM_STORAGE_PATH, } from '../utils/fs.js' -import { join } from 'path' +import { join, resolve, sep } from 'path' import { WikipediaOption, WikipediaState } from '../../types/downloads.js' import vine from '@vinejs/vine' import { wikipediaOptionsFileSchema } from '#validators/curated_collections' @@ -332,7 +332,13 @@ export class ZimService { fileName += '.zim' } - const fullPath = join(process.cwd(), ZIM_STORAGE_PATH, fileName) + const basePath = resolve(join(process.cwd(), ZIM_STORAGE_PATH)) + const fullPath = resolve(join(basePath, fileName)) + + // Prevent path traversal — resolved path must stay within the storage directory + if (!fullPath.startsWith(basePath + sep)) { + throw new Error('Invalid filename') + } const exists = await getFileStatsIfExists(fullPath) if (!exists) { diff --git a/admin/app/validators/common.ts b/admin/app/validators/common.ts index 369e7a7..27a441c 100644 --- a/admin/app/validators/common.ts +++ b/admin/app/validators/common.ts @@ -1,12 +1,39 @@ 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. + * + * Throws an error if the URL is internal/private. + */ +export function assertNotPrivateUrl(urlString: string): void { + const parsed = new URL(urlString) + const hostname = parsed.hostname.toLowerCase() + + const privatePatterns = [ + /^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$/, + /^\[::1\]$/, + /^\[?fe80:/i, + /^\[?fd[0-9a-f]{2}:/i, // Unique local IPv6 + ] + + if (privatePatterns.some((re) => re.test(hostname))) { + throw new Error(`Download URL must not point to a private/internal address: ${hostname}`) + } +} + export const remoteDownloadValidator = vine.compile( vine.object({ url: vine .string() - .url({ - require_tld: false, // Allow local URLs - }) + .url() .trim(), }) ) @@ -15,9 +42,7 @@ export const remoteDownloadWithMetadataValidator = vine.compile( vine.object({ url: vine .string() - .url({ - require_tld: false, // Allow local URLs - }) + .url() .trim(), metadata: vine .object({ @@ -34,9 +59,7 @@ export const remoteDownloadValidatorOptional = vine.compile( vine.object({ url: vine .string() - .url({ - require_tld: false, // Allow local URLs - }) + .url() .trim() .optional(), }) diff --git a/admin/docs/security-audit-v1.md b/admin/docs/security-audit-v1.md new file mode 100644 index 0000000..9afa638 --- /dev/null +++ b/admin/docs/security-audit-v1.md @@ -0,0 +1,299 @@ +# Project NOMAD Security Audit Report + +**Date:** 2026-03-08 +**Version audited:** v1.28.0 (main branch) +**Auditor:** Claude Code (automated + manual review) +**Target:** Pre-launch security review + +--- + +## Executive Summary + +Project NOMAD's codebase is **reasonably clean for a LAN appliance**, with no critical authentication bypasses or remote code execution vulnerabilities. However, there are **4 findings that should be fixed before public launch** — all are straightforward path traversal and SSRF issues with known fix patterns already used elsewhere in the codebase. + +| Severity | Count | Summary | +|----------|-------|---------| +| **HIGH** | 4 | Path traversal (3), SSRF (1) | +| **MEDIUM** | 5 | Dozzle shell, unvalidated settings read, content update URL injection, verbose errors, no rate limiting | +| **LOW** | 5 | CSRF disabled, CORS wildcard, debug logging, npm dep CVEs, hardcoded HMAC | +| **INFO** | 2 | No auth by design, Docker socket exposure by design | + +--- + +## Scans Performed + +| Scan | Tool | Result | +|------|------|--------| +| Dependency audit | `npm audit` | 2 CVEs (1 high, 1 moderate) | +| Secret scan | Manual grep (passwords, keys, tokens, certs) | Clean — all secrets from env vars | +| SAST | Semgrep (security-audit, OWASP, nodejs rulesets) | 0 findings (AdonisJS not in rulesets) | +| Docker config review | Manual review of compose, Dockerfiles, scripts | 2 actionable findings | +| Code review | Manual review of services, controllers, validators | 4 path traversal + 1 SSRF | +| API endpoint audit | Manual review of all 60+ routes | Attack surface documented | +| DAST (OWASP ZAP) | Skipped — Docker Desktop not running | Recommended as follow-up | + +--- + +## FIX BEFORE LAUNCH + +### 1. Path Traversal — ZIM File Delete (HIGH) + +**File:** `admin/app/services/zim_service.ts:329-342` +**Endpoint:** `DELETE /api/zim/:filename` + +The `filename` parameter flows into `path.join()` with no directory containment check. An attacker can delete `.zim` files outside the storage directory: + +``` +DELETE /api/zim/..%2F..%2Fsome-file.zim +``` + +**Fix:** Resolve the full path and verify it starts with the expected storage directory: + +```typescript +async delete(file: string): Promise { + let fileName = file + if (!fileName.endsWith('.zim')) { + fileName += '.zim' + } + + const basePath = join(process.cwd(), ZIM_STORAGE_PATH) + const fullPath = resolve(basePath, fileName) + + // Prevent path traversal + if (!fullPath.startsWith(basePath)) { + throw new Error('Invalid filename') + } + + // ... rest of delete logic +} +``` + +This pattern is already used correctly in `rag_service.ts:deleteFileBySource()`. + +--- + +### 2. Path Traversal — Map File Delete (HIGH) + +**File:** `admin/app/services/map_service.ts` (delete method) +**Endpoint:** `DELETE /api/maps/:filename` + +Identical pattern to the ZIM delete. Same fix — resolve path, verify `startsWith(basePath)`. + +--- + +### 3. Path Traversal — Documentation Read (HIGH) + +**File:** `admin/app/services/docs_service.ts:61-83` +**Endpoint:** `GET /docs/:slug` + +The `slug` parameter flows into `path.join(this.docsPath, filename)` with no containment check. An attacker can read arbitrary `.md` files on the filesystem: + +``` +GET /docs/..%2F..%2F..%2Fetc%2Fpasswd +``` + +Limited by the mandatory `.md` extension, but could still read sensitive markdown files outside the docs directory (like CLAUDE.md, README.md, etc.). + +**Fix:** + +```typescript +const basePath = this.docsPath +const fullPath = path.resolve(basePath, filename) + +if (!fullPath.startsWith(path.resolve(basePath))) { + throw new Error('Invalid document slug') +} +``` + +--- + +### 4. SSRF — Download Endpoints (HIGH) + +**File:** `admin/app/validators/common.ts:3-12` +**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: +- `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: + +```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)) +} +``` + +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) + +### 5. Dozzle Web Shell Access (MEDIUM) + +**File:** `install/management_compose.yaml:56` + +```yaml +- DOZZLE_ENABLE_SHELL=true +``` + +Dozzle on port 9999 is bound to all interfaces with shell access enabled. Anyone on the LAN can open a web shell into containers, including `nomad_admin` which has the Docker socket mounted. This creates a path from "LAN access" → "container shell" → "Docker socket" → "host root." + +**Fix:** Set `DOZZLE_ENABLE_SHELL=false`. Log viewing and container restart functionality are preserved. + +--- + +### 6. Unvalidated Settings Key Read (MEDIUM) + +**File:** `admin/app/controllers/settings_controller.ts` +**Endpoint:** `GET /api/system/settings?key=...` + +The `updateSetting` endpoint validates the key against an enum, but `getSetting` accepts any arbitrary key string. Currently harmless since the KV store only contains settings data, but could leak sensitive info if new keys are added. + +**Fix:** Apply the same enum validation to the read endpoint. + +--- + +### 7. Content Update URL Injection (MEDIUM) + +**File:** `admin/app/validators/common.ts:72-88` +**Endpoint:** `POST /api/content-updates/apply` + +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. + +--- + +### 8. Verbose Error Messages (MEDIUM) + +**Files:** `rag_controller.ts`, `docker_service.ts`, `system_update_service.ts` + +Several controllers return raw `error.message` in API responses, potentially leaking internal paths, stack details, or Docker error messages to the client. + +**Fix:** Return generic error messages in production. Log the details server-side. + +--- + +### 9. No Rate Limiting (MEDIUM) + +Zero rate limiting across all 60+ endpoints. While acceptable for a LAN appliance, some endpoints are particularly abusable: +- `POST /api/benchmark/run` — spins up Docker containers for CPU/memory/disk stress tests +- `POST /api/rag/upload` — file uploads (20MB limit per bodyparser config) +- `POST /api/system/services/affect` — can stop/start any service repeatedly + +**Fix:** Consider basic rate limiting on the benchmark and service control endpoints (e.g., 1 benchmark per minute, service actions throttled to prevent rapid cycling). + +--- + +## LOW PRIORITY / ACCEPTED RISK + +### 10. CSRF Protection Disabled (LOW) + +**File:** `admin/config/shield.ts` + +CSRF is disabled, meaning any website a LAN user visits could fire requests at NOMAD's API. This amplifies findings 1-4 — path traversal and SSRF could be triggered by a malicious webpage, not just direct LAN access. + +**Assessment:** Acceptable for a LAN appliance with no auth system. Enabling CSRF would require significant auth/session infrastructure changes. + +### 11. CORS Wildcard with Credentials (LOW) + +**File:** `admin/config/cors.ts` + +`origin: ['*']` with `credentials: true`. Standard for LAN appliances. + +### 12. npm Dependency CVEs (LOW) + +``` +tar <=7.5.9 HIGH Hardlink Path Traversal via Drive-Relative Linkpath +ajv <6.14.0 MODERATE ReDoS when using $data option +``` + +Both fixable via `npm audit fix`. Low practical risk since these are build/dev dependencies not directly exposed to user input. + +**Fix:** Run `npm audit fix` and commit the updated lockfile. + +### 13. Hardcoded HMAC Secret (LOW) + +**File:** `admin/app/services/benchmark_service.ts:35` + +The benchmark HMAC secret `'nomad-benchmark-v1-2026'` is hardcoded in open-source code. Anyone can forge leaderboard submissions. + +**Assessment:** Accepted risk. The leaderboard has compensating controls (rate limiting, plausibility validation, hardware fingerprint dedup). The secret stops casual abuse, not determined attackers. + +### 14. Production Debug Logging (LOW) + +**File:** `install/management_compose.yaml:22` + +```yaml +LOG_LEVEL=debug +``` + +Debug logging in production can expose internal state in log files. + +**Fix:** Change to `LOG_LEVEL=info` for production compose template. + +--- + +## INFORMATIONAL (By Design) + +### No Authentication + +All 60+ API endpoints are unauthenticated. This is by design — NOMAD is a LAN appliance and the network boundary is the access control. Issue #73 tracks the edge case of public IP interfaces. + +### Docker Socket Exposure + +The `nomad_admin` container mounts `/var/run/docker.sock`. This is necessary for NOMAD's core functionality (managing Docker containers). The socket is not exposed to the network — only the admin container can use it. + +--- + +## Recommendations Summary + +| Priority | Action | Effort | +|----------|--------|--------| +| **Before launch** | Fix 3 path traversals (ZIM delete, Map delete, Docs read) | ~30 min | +| **Before launch** | Add SSRF protection to download URL validators | ~1 hour | +| **Soon after** | Disable Dozzle shell access | 1 line change | +| **Soon after** | Validate settings key on read endpoint | ~15 min | +| **Soon after** | Sanitize error messages in responses | ~30 min | +| **Nice to have** | Run `npm audit fix` | 5 min | +| **Nice to have** | Change production log level to info | 1 line change | +| **Follow-up** | OWASP ZAP dynamic scan against NOMAD3 | ~1 hour | + +--- + +## What Went Right + +- **No hardcoded secrets** — all credentials properly use environment variables +- **No command injection** — Docker operations use the Docker API (dockerode), not shell commands +- **No SQL injection** — all database queries use AdonisJS Lucid ORM with parameterized queries +- **No eval/Function** — no dynamic code execution anywhere +- **RAG service already has the correct fix pattern** — `deleteFileBySource()` uses `resolve()` + `startsWith()` for path containment +- **Install script generates strong random passwords** — uses `/dev/urandom` for APP_KEY and DB passwords +- **No privileged containers** — GPU passthrough uses DeviceRequests, not --privileged +- **Health checks don't leak data** — internal-only calls