diff --git a/docs/security-architecture.md b/docs/security-architecture.md index 3b3b20bf..a2a3300b 100644 --- a/docs/security-architecture.md +++ b/docs/security-architecture.md @@ -306,6 +306,20 @@ injected from API JSON (`innerHTML`), not via `file-raw`, so they are unaffected is **defense‑in‑depth, not the primary boundary** — the realpath containment is the control. +### SSE log‑tail route — intentional extra read roots + +The live file‑tail SSE route (`FileStreamManager`, used to stream a growing log +into the UI) does **not** use `validateSessionFilePath`; it has its own validator +with a deliberately **wider** allowlist: the session `workingDir` **plus two +read‑only log roots — `/var/log` and `~/logs`** — so operators can tail +system/app logs. `/tmp` is intentionally excluded (world‑writable). Like the +other routes it `realpath`s the target and re‑checks right before spawning `tail` +(TOCTOU guard), and it is read‑only. This is the one place the per‑session +boundary is intentionally relaxed; on a password‑protected remote deployment an +authenticated user can therefore read `/var/log` and `~/logs` outside their +session dir. (Security review M5: this divergence is by design and is now +documented here rather than silently diverging from the per‑session claim above.) + ### Known limitation — `workingDir` scope The file‑route boundary is the session's `workingDir`, and `POST /api/sessions` diff --git a/src/file-stream-manager.ts b/src/file-stream-manager.ts index 342350f7..665c4c5c 100644 --- a/src/file-stream-manager.ts +++ b/src/file-stream-manager.ts @@ -395,8 +395,12 @@ export class FileStreamManager extends EventEmitter { // Normalize the working directory const normalizedWorkingDir = resolve(workingDir); - // Check if the resolved path is within the working directory - // or common log directories (/tmp intentionally excluded — world-writable) + // Allowed read roots for log tailing: the session working dir plus the + // INTENTIONAL log directories (/var/log, ~/logs). This is wider than the + // per-session boundary used by validateSessionFilePath — a deliberate, + // tested design choice for tailing system/app logs, documented as such in + // docs/security-architecture.md (security review M5). /tmp is excluded + // (world-writable). const allowedPaths = [normalizedWorkingDir, '/var/log', resolve(homedir(), 'logs')]; const isAllowed = allowedPaths.some((allowed) => { diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index e30c2592..48d2cdde 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -920,6 +920,7 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { private sessionExists(muxName: string): boolean { if (IS_TEST_MODE) return false; + if (!isValidMuxName(muxName)) return false; try { execSync(`${this.tmux()} has-session -t "${muxName}" 2>/dev/null`, { @@ -1060,13 +1061,15 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { } } - // Strategy 3: Kill tmux session by name - try { - execSync(`${this.tmux()} kill-session -t "${session.muxName}" 2>/dev/null`, { - timeout: EXEC_TIMEOUT_MS, - }); - } catch { - // Session may already be dead + // Strategy 3: Kill tmux session by name (guard the name before it reaches the shell) + if (isValidMuxName(session.muxName)) { + try { + execSync(`${this.tmux()} kill-session -t "${session.muxName}" 2>/dev/null`, { + timeout: EXEC_TIMEOUT_MS, + }); + } catch { + // Session may already be dead + } } // Strategy 4: Direct kill by PID as final fallback @@ -1166,6 +1169,14 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { for (const [sessionName, pid] of active) { if (!sessionName.startsWith('codeman-') && !sessionName.startsWith('claudeman-')) continue; + // Only admit names that pass the safe-name pattern. A foreign process on the + // shared `tmux -L codeman` socket could create a `codeman-…` session whose name + // contains shell metacharacters; rejecting it here keeps it out of this.sessions + // and away from the name-interpolating tmux call sites (M1). + if (!isValidMuxName(sessionName)) { + console.warn(`[TmuxManager] Skipping discovered tmux session with unsafe name: ${sessionName}`); + continue; + } if (knownMuxNames.has(sessionName)) continue; const fragment = sessionName.replace(/^(?:codeman|claudeman)-/, ''); diff --git a/src/utils/index.ts b/src/utils/index.ts index 298dd5f0..89d8d89d 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -22,6 +22,7 @@ export { execPattern, } from './regex-patterns.js'; export { MAX_SESSION_TOKENS } from './token-validation.js'; +export { isSafePushEndpoint } from './push-endpoint-validation.js'; export { stringSimilarity, fuzzyPhraseMatch, todoContentHash } from './string-similarity.js'; export { assertNever } from './type-safety.js'; export { wrapWithNice } from './nice-wrapper.js'; diff --git a/src/utils/push-endpoint-validation.ts b/src/utils/push-endpoint-validation.ts new file mode 100644 index 00000000..11ffa15e --- /dev/null +++ b/src/utils/push-endpoint-validation.ts @@ -0,0 +1,69 @@ +/** + * @fileoverview SSRF guard for web-push subscription endpoints (security review M7). + * + * A push `endpoint` is an attacker-suppliable URL that the server fetches via + * `webpush.sendNotification`. On the no-auth loopback default a local page (or any + * non-browser client) could register an endpoint pointing at the cloud metadata + * service (169.254.169.254) or an internal host, turning the server into an SSRF + * proxy. We require https and reject IP-literal hosts in private/loopback/ + * link-local/reserved ranges. DNS-named hosts are allowed (every real push service + * — FCM, Mozilla, Apple, WNS — uses a public DNS name); this is checked both at + * subscribe time (schema) and again at send time (defense-in-depth). + * + * Note: a hostname that *resolves* to an internal IP (DNS rebinding) is not caught + * here without async resolution; the realistic, documented vector (a direct + * internal IP literal) is closed. + */ +import { isIP } from 'node:net'; + +/** True if `host` is an IP literal in a private, loopback, link-local, or reserved range. */ +function isPrivateOrReservedIp(host: string): boolean { + const kind = isIP(host); + if (kind === 0) return false; // not an IP literal — a DNS name + + if (kind === 4) { + const [a, b] = host.split('.').map(Number); + if (a === 0 || a === 10 || a === 127) return true; // unspecified, private, loopback + if (a === 169 && b === 254) return true; // link-local (incl. 169.254.169.254 metadata) + if (a === 172 && b >= 16 && b <= 31) return true; // private + if (a === 192 && b === 168) return true; // private + if (a === 100 && b >= 64 && b <= 127) return true; // CGNAT (RFC 6598) + if (a >= 224) return true; // multicast + reserved (224.0.0.0+) + return false; + } + + // IPv6 + const h = host.toLowerCase(); + if (h === '::1' || h === '::') return true; // loopback, unspecified + if (h.startsWith('fe8') || h.startsWith('fe9') || h.startsWith('fea') || h.startsWith('feb')) return true; // fe80::/10 link-local + if (h.startsWith('fc') || h.startsWith('fd')) return true; // fc00::/7 unique-local + // IPv4-mapped (::ffff:a.b.c.d). URL/Node may normalize the dotted tail to hex + // (::ffff:7f00:1), so handle both forms and re-check the embedded IPv4. + const mappedDotted = h.match(/^::ffff:(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/); + if (mappedDotted) return isPrivateOrReservedIp(mappedDotted[1]); + const mappedHex = h.match(/^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/); + if (mappedHex) { + const hi = parseInt(mappedHex[1], 16); + const lo = parseInt(mappedHex[2], 16); + return isPrivateOrReservedIp(`${(hi >> 8) & 0xff}.${hi & 0xff}.${(lo >> 8) & 0xff}.${lo & 0xff}`); + } + return false; +} + +/** + * Validate a web-push endpoint URL is safe to fetch server-side. + * Requires an https URL whose host is not an internal/reserved IP literal. + */ +export function isSafePushEndpoint(endpoint: string): boolean { + let url: URL; + try { + url = new URL(endpoint); + } catch { + return false; + } + if (url.protocol !== 'https:') return false; + if (!url.hostname) return false; + // URL.hostname wraps IPv6 literals in brackets ([::1]); strip them for isIP(). + const host = url.hostname.replace(/^\[|\]$/g, ''); + return !isPrivateOrReservedIp(host); +} diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 3867b07b..0c3fa450 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -8,7 +8,7 @@ */ import { z } from 'zod'; -import { SAFE_PATH_PATTERN } from '../utils/index.js'; +import { SAFE_PATH_PATTERN, isSafePushEndpoint } from '../utils/index.js'; // ========== Path Validation ========== @@ -531,7 +531,11 @@ export const RespawnEnableSchema = z.object({ /** POST /api/push/subscribe */ export const PushSubscribeSchema = z.object({ - endpoint: z.string().url().max(2000), + endpoint: z + .string() + .url() + .max(2000) + .refine(isSafePushEndpoint, { message: 'endpoint must be an https URL to a public (non-internal) host' }), keys: z.object({ p256dh: z.string().min(1).max(500), auth: z.string().min(1).max(500), diff --git a/src/web/server.ts b/src/web/server.ts index c4a727c9..b9f7f75f 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -97,7 +97,13 @@ import { type ImageDetectedEvent, DEFAULT_NICE_CONFIG, } from '../types.js'; -import { CleanupManager, KeyedDebouncer, StaleExpirationMap, startEventLoopMonitor } from '../utils/index.js'; +import { + CleanupManager, + KeyedDebouncer, + StaleExpirationMap, + startEventLoopMonitor, + isSafePushEndpoint, +} from '../utils/index.js'; import type { EventLoopMonitorHandle } from '../utils/index.js'; import { MAX_CONCURRENT_SESSIONS, MAX_SSE_CLIENTS } from '../config/map-limits.js'; import { SseEvent } from './sse-events.js'; @@ -1639,6 +1645,14 @@ export class WebServer extends EventEmitter { // Check per-subscription preferences if (sub.pushPreferences[event] === false) continue; + // Re-validate the stored endpoint before fetching it server-side (SSRF, M7). + // Defense-in-depth: subscribe-time validation already rejects unsafe URLs. + if (!isSafePushEndpoint(sub.endpoint)) { + console.warn('[push] skipping notification to unsafe endpoint:', sub.endpoint); + this.pushStore.removeByEndpoint(sub.endpoint); + continue; + } + const pushSub = { endpoint: sub.endpoint, keys: sub.keys, diff --git a/test/push-endpoint-validation.test.ts b/test/push-endpoint-validation.test.ts new file mode 100644 index 00000000..d9cf2ee1 --- /dev/null +++ b/test/push-endpoint-validation.test.ts @@ -0,0 +1,45 @@ +/** + * SSRF guard for web-push endpoints (security review M7). + */ +import { describe, it, expect } from 'vitest'; +import { isSafePushEndpoint } from '../src/utils/push-endpoint-validation.js'; + +describe('isSafePushEndpoint (SSRF guard, M7)', () => { + it('accepts real https push-service endpoints (public DNS hosts)', () => { + expect(isSafePushEndpoint('https://fcm.googleapis.com/fcm/send/abc123')).toBe(true); + expect(isSafePushEndpoint('https://updates.push.services.mozilla.com/wpush/v2/abc')).toBe(true); + expect(isSafePushEndpoint('https://web.push.apple.com/abc')).toBe(true); + expect(isSafePushEndpoint('https://foo.notify.windows.com/w/?token=x')).toBe(true); + }); + + it('accepts a public IP literal over https', () => { + expect(isSafePushEndpoint('https://93.184.216.34/x')).toBe(true); + }); + + it('rejects non-https schemes', () => { + expect(isSafePushEndpoint('http://fcm.googleapis.com/x')).toBe(false); + expect(isSafePushEndpoint('ftp://example.com/x')).toBe(false); + }); + + it('rejects the cloud-metadata IP and internal IPv4 ranges', () => { + expect(isSafePushEndpoint('https://169.254.169.254/latest/meta-data/')).toBe(false); + expect(isSafePushEndpoint('https://127.0.0.1/x')).toBe(false); + expect(isSafePushEndpoint('https://10.0.0.5/x')).toBe(false); + expect(isSafePushEndpoint('https://192.168.1.10/x')).toBe(false); + expect(isSafePushEndpoint('https://172.16.0.1/x')).toBe(false); + expect(isSafePushEndpoint('https://100.64.0.1/x')).toBe(false); + expect(isSafePushEndpoint('https://0.0.0.0/x')).toBe(false); + }); + + it('rejects internal IPv6 (incl. bracketed + IPv4-mapped)', () => { + expect(isSafePushEndpoint('https://[::1]/x')).toBe(false); + expect(isSafePushEndpoint('https://[fe80::1]/x')).toBe(false); + expect(isSafePushEndpoint('https://[fd00::1]/x')).toBe(false); + expect(isSafePushEndpoint('https://[::ffff:127.0.0.1]/x')).toBe(false); + }); + + it('rejects garbage / empty input', () => { + expect(isSafePushEndpoint('not a url')).toBe(false); + expect(isSafePushEndpoint('')).toBe(false); + }); +});