From 213dfa8bc0baf2b95814af8ebc02cebe5edfbc2b Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 24 Jun 2026 15:38:29 -0700 Subject: [PATCH] fix(security): confine routing dotfiles, skills dir, slugs, and transcription exec Shared src/core/path-confine.ts consolidates the realpath-containment idiom (moved from sources-ops.ts) and adds isTrustedDotfile + isWriteTargetContained. - .gbrain-source (source-resolver) and .gbrain-mount (brain-resolver) walk-up dotfiles are now lstat trust-gated: a symlink, foreign-owned, or world-writable file is refused on multi-user hosts (#418), fail-closed on stat error. - resolveWorkspaceSkillsDir + every skills-dir tier (env, cwd_walk_up, repo_root, cwd_skills, install_path) route through realpath containment so a symlinked workspace/skills can't escape the declared workspace (#419). - resolveSourceId/resolveBrainId realpath both sides of the registered local_path / mount prefix match so a symlinked cwd can't misattribute source/brain. - validateSlug rejects NUL/control, bidi/RTL overrides, backslashes, and URL-encoded path separators at the shared putPage/updateSlug chokepoint; write-through confirms the file path stays within the source tree. - transcribeLargeFile uses execFileSync arg-arrays + fs.rmSync (no shell), so a path with shell metacharacters is never parsed by a shell (#245). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/brain-resolver.ts | 18 +- src/core/path-confine.ts | 121 ++++++++++ src/core/repo-root.ts | 30 ++- src/core/source-resolver.ts | 27 ++- src/core/sources-ops.ts | 28 +-- src/core/transcription.ts | 38 ++-- src/core/utils.ts | 25 +++ src/core/write-through.ts | 17 +- test/path-confine.test.ts | 319 +++++++++++++++++++++++++++ test/transcription-injection.test.ts | 65 ++++++ 10 files changed, 633 insertions(+), 55 deletions(-) create mode 100644 src/core/path-confine.ts create mode 100644 test/path-confine.test.ts create mode 100644 test/transcription-injection.test.ts diff --git a/src/core/brain-resolver.ts b/src/core/brain-resolver.ts index 906b04608..ac7ff2802 100644 --- a/src/core/brain-resolver.ts +++ b/src/core/brain-resolver.ts @@ -19,9 +19,10 @@ * parent's brainId instead of re-running this resolver. */ -import { readFileSync, existsSync } from 'fs'; +import { readFileSync, lstatSync, type Stats } from 'fs'; import { join, dirname, resolve } from 'path'; import { HOST_BRAIN_ID, loadMounts, validateMountId, type MountEntry } from './brain-registry.ts'; +import { isTrustedDotfile, realpathOrResolve } from './path-confine.ts'; const DOTFILE = '.gbrain-mount'; /** Same regex as brain-registry. Kept in sync. */ @@ -36,7 +37,13 @@ function readDotfileWalk(startDir: string): string | null { let dir = resolve(startDir); for (let i = 0; i < 50; i++) { const candidate = join(dir, DOTFILE); - if (existsSync(candidate)) { + // lstatSync (NOT statSync) + isTrustedDotfile: refuse a symlink, foreign- + // owned, or world-writable `.gbrain-mount` planted by another user in a + // shared ancestor dir (same multi-user-host hijack as #418, applied to the + // brain axis). Any stat error → skip and keep walking (fail-closed). + let st: Stats | null = null; + try { st = lstatSync(candidate); } catch { st = null; } + if (st && isTrustedDotfile(st)) { try { const content = readFileSync(candidate, 'utf8').trim().split('\n')[0].trim(); if (content === HOST_BRAIN_ID) return content; @@ -54,11 +61,14 @@ function readDotfileWalk(startDir: string): string | null { /** Longest-prefix match: find the mount whose `path` contains `cwd`. */ function longestPathPrefixMount(mounts: MountEntry[], cwd: string): MountEntry | null { - const cwdResolved = resolve(cwd); + // realpath both sides so a symlinked CWD can't forge a prefix match against a + // mount path it doesn't really live under (codex #9, brain-axis mirror of the + // source-resolver fix). Falls back to lexical resolve() for a stale mount path. + const cwdResolved = realpathOrResolve(cwd); let best: { mount: MountEntry; pathLen: number } | null = null; for (const m of mounts) { if (m.enabled === false) continue; - const p = resolve(m.path); + const p = realpathOrResolve(m.path); if (cwdResolved === p || cwdResolved.startsWith(p + '/')) { if (!best || p.length > best.pathLen) { best = { mount: m, pathLen: p.length }; diff --git a/src/core/path-confine.ts b/src/core/path-confine.ts new file mode 100644 index 000000000..9f7eced91 --- /dev/null +++ b/src/core/path-confine.ts @@ -0,0 +1,121 @@ +/** + * Shared symlink-safe path-confinement + dotfile-trust helpers. + * + * Consolidates the realpath-containment idiom that previously lived only in + * `sources-ops.ts` (`isPathContained`) and `validateUploadPath` + * (`operations.ts`), and adds `isTrustedDotfile` — the multi-user-host trust + * gate for walk-up routing dotfiles (`.gbrain-source` / `.gbrain-mount`). + * + * Threat model (POSIX multi-user host): an attacker who can write into a + * shared ancestor directory of the victim's CWD (`/tmp`, `/var/tmp`, + * `/dev/shm`, shared NFS/SMB, CI runner volumes, container bind-mounts) can + * plant a routing dotfile that silently retargets the victim's reads/writes + * to the attacker's source/brain. The walk-up resolvers must therefore refuse + * a dotfile they can't prove the victim (or root) owns. (#418/#419) + * + * Fail-closed: any stat/realpath error → not trusted / not contained. The one + * documented exception is platforms without numeric uid (Windows), where the + * multi-user-POSIX threat model does not apply and `isTrustedDotfile` trusts + * by default so existing single-user setups keep working. + */ + +import { realpathSync, existsSync, type Stats } from 'fs'; +import { resolve as resolvePath, relative, isAbsolute, dirname, basename, join } from 'path'; + +/** + * Symlink-safe path confinement: realpath BOTH sides, then a separator-aware + * prefix check. A plain `startsWith()` on un-resolved paths would let a + * `parent/skills` symlink → `/etc` (or `$GBRAIN_HOME/clones/` → `/etc`) + * bypass the boundary; resolving first defeats that. + * + * Returns true iff `child` exists AND its realpath is `parent`'s realpath or a + * real subtree of it. Returns false if either path is unresolvable (missing / + * permission) or the resolved child escapes — fail-closed. + */ +export function isPathContained(child: string, parent: string): boolean { + let resolvedChild: string; + let resolvedParent: string; + try { + resolvedChild = realpathSync(child); + resolvedParent = realpathSync(parent); + } catch { + return false; // missing / unresolvable path → not contained + } + // Append a separator so /foo doesn't match /foobar. + const parentWithSep = resolvedParent.endsWith('/') ? resolvedParent : resolvedParent + '/'; + return resolvedChild === resolvedParent || resolvedChild.startsWith(parentWithSep); +} + +/** + * Trust gate for a walk-up routing dotfile, given its `lstatSync` Stats. + * + * The caller MUST pass an `lstatSync` result, never `statSync` — `lstat` does + * not follow symlinks, so a planted symlink redirect is visible here as + * `isSymbolicLink()` instead of being followed-then-trusted. + * + * Rejects three classes of untrusted file: + * 1. symlinks — an attacker-planted redirect to a file they control; + * 2. foreign-owned — `uid` is neither the caller's nor root's (an attacker + * can't `chown` a file to the victim, so foreign ownership means planted; + * root-owned is trusted — root is the system admin and can write anywhere + * regardless); + * 3. world-writable (`mode & 0o002`) — anyone can clobber it later, even when + * ownership is currently legitimate. + * + * On platforms without `process.getuid` (Windows) returns true: the + * multi-user-POSIX threat model does not apply and ownership is unknowable. + */ +export function isTrustedDotfile(stats: Stats): boolean { + // No numeric uid (Windows) → can't verify ownership; threat model N/A. + if (typeof process.getuid !== 'function') return true; + // A symlink is an attacker redirect — never trust. (Requires an lstat Stats.) + if (stats.isSymbolicLink()) return false; + const myUid = process.getuid(); + // Foreign-owned (not me, not root) → planted. Root-owned is trusted. + if (stats.uid !== myUid && stats.uid !== 0) return false; + // World-writable → anyone can clobber it later, even when ownership is legit. + if ((stats.mode & 0o002) !== 0) return false; + return true; +} + +/** + * Resolve a path through symlinks, falling back to lexical `resolve()` when the + * path doesn't exist (stale registration). Used by the registered-path prefix + * matchers so a symlinked CWD can't create a false prefix match against a + * registered `local_path` / mount path while still tolerating a registered path + * that no longer exists on disk. + */ +export function realpathOrResolve(p: string): string { + try { + return realpathSync(p); + } catch { + return resolvePath(p); + } +} + +/** + * Containment check for a write TARGET that may not exist yet (a new page file). + * `isPathContained` requires the child to already exist; this instead realpaths + * the deepest EXISTING ancestor of `target` (catching a symlinked intermediate + * directory that escapes the tree) and re-attaches the not-yet-created tail + * lexically, then confirms the result stays within `root`. + * + * Defense-in-depth for the write-through FS sink (#1647-slug / codex #6): + * `validateSlug` already rejects `..`/backslash/control/%2e in the slug, so this + * guards a pre-existing hostile row or a symlinked source-tree subdirectory. + */ +export function isWriteTargetContained(target: string, root: string): boolean { + const resolvedRoot = realpathOrResolve(root); + let existing = resolvePath(target); + const tail: string[] = []; + for (let i = 0; i < 4096 && !existsSync(existing); i++) { + tail.unshift(basename(existing)); + const parent = dirname(existing); + if (parent === existing) break; // filesystem root + existing = parent; + } + const base = realpathOrResolve(existing); + const finalPath = tail.length ? join(base, ...tail) : base; + const rel = relative(resolvedRoot, finalPath); + return rel === '' || (!rel.startsWith('..') && !isAbsolute(rel)); +} diff --git a/src/core/repo-root.ts b/src/core/repo-root.ts index 9f6a92264..29f72007e 100644 --- a/src/core/repo-root.ts +++ b/src/core/repo-root.ts @@ -2,6 +2,7 @@ import { existsSync } from 'fs'; import { fileURLToPath } from 'url'; import { isAbsolute, join, resolve as resolvePath } from 'path'; import { RESOLVER_FILENAMES, hasResolverFile } from './resolver-filenames.ts'; +import { isPathContained } from './path-confine.ts'; /** * Walk up from `startDir` looking for a `skills/` directory that @@ -66,17 +67,23 @@ function resolveWorkspaceSkillsDir( sourceSubdir: SkillsDirSource, sourceRoot: SkillsDirSource, ): SkillsDirDetection | null { - // Preferred: workspace/skills with a resolver file inside it (gbrain-native). const subdir = join(workspace, 'skills'); + // Refuse a `skills/` that escapes the declared workspace via symlink (#419). + // isPathContained realpaths both ends, so `workspace/skills` → /etc is + // rejected, while a legit in-workspace symlink (`workspace/skills` → + // `workspace/_real-skills`) stays contained and is allowed. A non-contained + // candidate returns null so lower tiers can try, rather than trusting an escape. + const contained = isPathContained(subdir, workspace); + // Preferred: workspace/skills with a resolver file inside it (gbrain-native). if (hasResolverFile(subdir)) { - return { dir: subdir, source: sourceSubdir }; + return contained ? { dir: subdir, source: sourceSubdir } : null; } // Fallback: resolver file at workspace root (OpenClaw-native layout). // The skills/ subtree still governs file layout even when routing lives // at workspace root. Return the skills subdir so downstream file lookups // work; the resolver parser knows how to look one level up. if (hasResolverFile(workspace) && existsSync(subdir)) { - return { dir: subdir, source: sourceRoot }; + return contained ? { dir: subdir, source: sourceRoot } : null; } return null; } @@ -152,7 +159,10 @@ export function autoDetectSkillsDir( let dir = startDir; for (let i = 0; i < 10; i++) { const candidate = join(dir, 'skills'); - if (existsSync(candidate)) { + // Only accept a `skills/` contained within the ancestor it was found under + // (#419). An escaping symlink is skipped, and the walk continues upward + // rather than trusting a dir that resolves outside the boundary. + if (existsSync(candidate) && isPathContained(candidate, dir)) { return { dir: candidate, source: 'cwd_walk_up' }; } const parent = join(dir, '..'); @@ -177,7 +187,10 @@ export function autoDetectSkillsDir( // 3. gbrain repo walk from cwd. const repoRoot = findRepoRoot(startDir); if (repoRoot && isGbrainRepoRoot(repoRoot)) { - return { dir: join(repoRoot, 'skills'), source: 'repo_root' }; + const skillsDir = join(repoRoot, 'skills'); + if (isPathContained(skillsDir, repoRoot)) { + return { dir: skillsDir, source: 'repo_root' }; + } } // 4. ./skills fallback (with hasResolverFile gate). Functionally @@ -187,7 +200,7 @@ export function autoDetectSkillsDir( // In practice this tier never fires after 1b — cwd_walk_up matches // the same path first. Kept in the type union for back-compat. const cwdSkills = join(startDir, 'skills'); - if (hasResolverFile(cwdSkills)) { + if (hasResolverFile(cwdSkills) && isPathContained(cwdSkills, startDir)) { return { dir: cwdSkills, source: 'cwd_skills' }; } @@ -237,7 +250,10 @@ export function autoDetectSkillsDirReadOnly( const moduleDir = fileURLToPath(import.meta.url); const installRoot = findRepoRoot(moduleDir); if (installRoot && isGbrainRepoRoot(installRoot)) { - return { dir: join(installRoot, 'skills'), source: 'install_path' }; + const skillsDir = join(installRoot, 'skills'); + if (isPathContained(skillsDir, installRoot)) { + return { dir: skillsDir, source: 'install_path' }; + } } } catch { // fileURLToPath can throw on malformed import.meta.url (rare; some diff --git a/src/core/source-resolver.ts b/src/core/source-resolver.ts index 64bb33015..8b9f3bada 100644 --- a/src/core/source-resolver.ts +++ b/src/core/source-resolver.ts @@ -13,10 +13,11 @@ * commands (Steps 4/5), and the operation layer (Step 2+). */ -import { readFileSync, existsSync } from 'fs'; +import { readFileSync, lstatSync, type Stats } from 'fs'; import { join, dirname, resolve } from 'path'; import type { BrainEngine } from './engine.ts'; import { SOURCE_ID_RE, isValidSourceId } from './source-id.ts'; +import { isTrustedDotfile, realpathOrResolve } from './path-confine.ts'; const DOTFILE = '.gbrain-source'; // Canonical SOURCE_ID_RE imported from `source-id.ts` (single source of truth). @@ -34,7 +35,15 @@ function readDotfileWalk(startDir: string): string | null { // Guard against infinite loops on malformed paths. for (let i = 0; i < 50; i++) { const candidate = join(dir, DOTFILE); - if (existsSync(candidate)) { + // lstatSync (NOT statSync) so a planted symlink is seen here, not silently + // followed-then-trusted. Any stat error (ENOENT / permission) → skip this + // candidate and keep walking (fail-closed). On a multi-user host an + // attacker who can write a shared ancestor dir could otherwise plant a + // forged `.gbrain-source`; `isTrustedDotfile` refuses symlinks, + // foreign-owned, and world-writable files (#418). + let st: Stats | null = null; + try { st = lstatSync(candidate); } catch { st = null; } + if (st && isTrustedDotfile(st)) { try { const content = readFileSync(candidate, 'utf8').trim().split('\n')[0].trim(); // Silent-fallback tier per codex P1-F: invalid dotfile content @@ -105,10 +114,15 @@ export async function resolveSourceId( const registered = await engine.executeRaw<{ id: string; local_path: string }>( `SELECT id, local_path FROM sources WHERE local_path IS NOT NULL`, ); - const cwdResolved = resolve(cwd); + // realpath BOTH sides (not bare resolve) so a symlinked CWD can't forge a + // prefix match against a registered local_path it doesn't really live under + // (codex #9). realpathOrResolve falls back to lexical resolve() for a stale + // registration whose path no longer exists. Resolving both sides keeps a + // legitimately symlinked vault matching — only one-sided symlinks break. + const cwdResolved = realpathOrResolve(cwd); let best: { id: string; pathLen: number } | null = null; for (const r of registered) { - const p = resolve(r.local_path); + const p = realpathOrResolve(r.local_path); if (cwdResolved === p || cwdResolved.startsWith(p + '/')) { if (!best || p.length > best.pathLen) { best = { id: r.id, pathLen: p.length }; @@ -303,10 +317,11 @@ export async function resolveSourceWithTier( const registered = await engine.executeRaw<{ id: string; local_path: string }>( `SELECT id, local_path FROM sources WHERE local_path IS NOT NULL`, ); - const cwdResolved = resolve(cwd); + // realpath both sides — see the matching block in resolveSourceId (codex #9). + const cwdResolved = realpathOrResolve(cwd); let best: { id: string; path: string; pathLen: number } | null = null; for (const r of registered) { - const p = resolve(r.local_path); + const p = realpathOrResolve(r.local_path); if (cwdResolved === p || cwdResolved.startsWith(p + '/')) { if (!best || p.length > best.pathLen) { best = { id: r.id, path: p, pathLen: p.length }; diff --git a/src/core/sources-ops.ts b/src/core/sources-ops.ts index 3cb1c0a9a..c5caea122 100644 --- a/src/core/sources-ops.ts +++ b/src/core/sources-ops.ts @@ -37,8 +37,8 @@ */ import { existsSync, mkdirSync, renameSync, rmSync, lstatSync } from 'fs'; -import { realpathSync } from 'fs'; import { join, dirname, basename, resolve as resolvePath } from 'path'; +import { isPathContained } from './path-confine.ts'; import { randomBytes } from 'crypto'; import type { BrainEngine } from './engine.ts'; import { @@ -231,28 +231,10 @@ function makeTempCloneDir(id: string): string { return gbrainPath('clones', '.tmp', `${id}-${rand}`); } -/** - * Symlink-safe path confinement: realpath both sides, then lstat-walk to - * confirm `child` is a real subtree of `parent`. Mirrors validateUploadPath - * shape at src/core/operations.ts:61. String startsWith() would let - * $GBRAIN_HOME/clones/ → /etc bypass the confine. - * - * Returns true if `child` exists and is contained under `parent`. - * Returns false if the resolved path escapes, or either path is unresolvable. - */ -export function isPathContained(child: string, parent: string): boolean { - let resolvedChild: string; - let resolvedParent: string; - try { - resolvedChild = realpathSync(child); - resolvedParent = realpathSync(parent); - } catch { - return false; // missing path → not contained - } - // Append a separator to parent so /foo doesn't match /foobar. - const parentWithSep = resolvedParent.endsWith('/') ? resolvedParent : resolvedParent + '/'; - return resolvedChild === resolvedParent || resolvedChild.startsWith(parentWithSep); -} +// `isPathContained` moved to `src/core/path-confine.ts` (shared with the +// dotfile-trust + skills-dir confinement helpers). Re-exported here so existing +// importers (and recloneIfMissing below) keep working unchanged. +export { isPathContained }; /** * Did gbrain CREATE this clone (so re-clone/delete is safe)? Ownership, NOT diff --git a/src/core/transcription.ts b/src/core/transcription.ts index 7aed5f7b3..e87efdbb4 100644 --- a/src/core/transcription.ts +++ b/src/core/transcription.ts @@ -6,8 +6,8 @@ * For files >25MB: ffmpeg segmentation into <25MB chunks, transcribe each, concatenate. */ -import { statSync, readFileSync } from 'fs'; -import { basename, extname } from 'path'; +import { statSync, readFileSync, rmSync } from 'fs'; +import { basename, extname, join } from 'path'; // --------------------------------------------------------------------------- // Types @@ -169,14 +169,22 @@ async function transcribeLargeFile( ); } - // Segment into ~20MB chunks (with some overlap for better joining) - const { execSync } = await import('child_process'); - const tmpDir = execSync('mktemp -d').toString().trim(); + // Segment into ~20MB chunks (with some overlap for better joining). + // + // SECURITY (#245): use execFileSync with argument arrays — never a shell + // command string. audioPath is interpolated here, and a path containing + // shell metacharacters (`;`, `$()`, backticks, quotes) would be a command + // injection if passed through a shell. Argument arrays bypass the shell + // entirely, so audioPath is always a single literal argv element. + const { execFileSync } = await import('child_process'); + const tmpDir = execFileSync('mktemp', ['-d']).toString().trim(); try { // Get audio duration - const durationStr = execSync( - `ffprobe -v error -show_entries format=duration -of default=noprint_wrappers=1:nokey=1 "${audioPath}"`, + const durationStr = execFileSync( + 'ffprobe', + ['-v', 'error', '-show_entries', 'format=duration', + '-of', 'default=noprint_wrappers=1:nokey=1', audioPath], { encoding: 'utf-8' } ).trim(); const totalDuration = parseFloat(durationStr) || 0; @@ -188,8 +196,10 @@ async function transcribeLargeFile( // Split audio const ext = extname(audioPath); - execSync( - `ffmpeg -i "${audioPath}" -f segment -segment_time ${segmentSeconds} -c copy "${tmpDir}/segment_%03d${ext}"`, + execFileSync( + 'ffmpeg', + ['-i', audioPath, '-f', 'segment', '-segment_time', String(segmentSeconds), + '-c', 'copy', join(tmpDir, `segment_%03d${ext}`)], { stdio: 'pipe' } ); @@ -200,7 +210,7 @@ async function transcribeLargeFile( let timeOffset = 0; for (const seg of segments) { - const segPath = `${tmpDir}/${seg}`; + const segPath = join(tmpDir, seg); const result = await transcribeFile(segPath, provider, apiKey, config); // Offset timestamps result.segments = result.segments.map(s => ({ @@ -221,15 +231,15 @@ async function transcribeLargeFile( provider, }; } finally { - // Cleanup temp directory - try { execSync(`rm -rf "${tmpDir}"`); } catch {} + // Cleanup temp directory via fs.rmSync — never a shell remove command (#245). + try { rmSync(tmpDir, { recursive: true, force: true }); } catch {} } } async function checkFfmpeg(): Promise { try { - const { execSync } = await import('child_process'); - execSync('ffmpeg -version', { stdio: 'pipe' }); + const { execFileSync } = await import('child_process'); + execFileSync('ffmpeg', ['-version'], { stdio: 'pipe' }); return true; } catch { return false; diff --git a/src/core/utils.ts b/src/core/utils.ts index dd348438b..0e885b2ee 100644 --- a/src/core/utils.ts +++ b/src/core/utils.ts @@ -19,11 +19,36 @@ export function generateToken(prefix: string): string { /** * Validate and normalize a slug. Slugs are lowercased repo-relative paths. * Rejects empty slugs, path traversal (..), and leading /. + * + * SECURITY (#1647-slug / codex #6): also rejects a small set of dangerous + * characters that survive the `..` check but can still produce a hostile + * filename at the write-through FS sink or hide the real target — NUL/control + * bytes, Unicode bidirectional/RTL overrides, backslashes, and URL-encoded + * path separators/traversal. None appear in legitimate slugs (lowercase + * alphanumerics, hyphens, dots, underscores, slashes, unicode letters, and CJK + * all still pass), so this is pure hardening, not a behavior change. This is the + * shared chokepoint for both `putPage` and `updateSlug` on both engines. */ export function validateSlug(slug: string): string { if (!slug || /(^|\/)\.\.($|\/)/.test(slug) || /^\//.test(slug)) { throw new Error(`Invalid slug: "${slug}". Slugs cannot be empty, start with /, or contain path traversal.`); } + // Control / NUL bytes (C0 + DEL + C1). + if (/[\x00-\x1f\x7f-\x9f]/.test(slug)) { + throw new Error(`Invalid slug: "${slug}". Slugs cannot contain control characters.`); + } + // Unicode bidirectional / RTL overrides (visual-spoofing of the real path). + if (/[\u202a-\u202e\u2066-\u2069]/.test(slug)) { + throw new Error(`Invalid slug: "${slug}". Slugs cannot contain bidirectional/RTL override characters.`); + } + // Backslash (Windows-style separator / escape). + if (slug.includes('\\')) { + throw new Error(`Invalid slug: "${slug}". Backslashes are not allowed in slugs.`); + } + // URL-encoded path separators / traversal (%2e=., %2f=/, %5c=\). + if (/%2e|%2f|%5c/i.test(slug)) { + throw new Error(`Invalid slug: "${slug}". URL-encoded path separators are not allowed in slugs.`); + } return slug.toLowerCase(); } diff --git a/src/core/write-through.ts b/src/core/write-through.ts index 96a1aad9b..5ca2b565c 100644 --- a/src/core/write-through.ts +++ b/src/core/write-through.ts @@ -26,6 +26,7 @@ import { dirname, join } from 'path'; import { randomBytes } from 'crypto'; import type { BrainEngine } from './engine.ts'; import { serializePageToMarkdown, resolvePageFilePath } from './markdown.ts'; +import { isWriteTargetContained } from './path-confine.ts'; /** Minimal logger surface — structurally compatible with operations.ts `Logger`. */ export interface WriteThroughLogger { @@ -45,8 +46,10 @@ export interface WriteThroughResult { * — #2018: writing here would pollute that sibling's repo, so we skip. * - page_not_found_after_write: the DB row isn't readable back (the caller's * DB write failed or targeted a different source). + * - path_escapes_source_root: the computed file path resolves outside the + * source's working tree (hostile slug row / symlinked subtree) — refused. */ - skipped?: 'no_repo_configured' | 'repo_not_found' | 'source_repo_belongs_to_other_source' | 'page_not_found_after_write'; + skipped?: 'no_repo_configured' | 'repo_not_found' | 'source_repo_belongs_to_other_source' | 'page_not_found_after_write' | 'path_escapes_source_root'; /** Set when the render/write/rename itself threw (EACCES, ENOTDIR, disk full). */ error?: string; } @@ -83,6 +86,7 @@ export async function writePageThrough( // `local_path`, nesting this page there would pollute that sibling's // git repo (the reported bug). Skip instead. let filePath: string; + let writeRoot: string; const srcRows = await engine.executeRaw<{ local_path: string | null }>( `SELECT local_path FROM sources WHERE id = $1`, [sourceId], @@ -93,6 +97,7 @@ export async function writePageThrough( return { written: false, skipped: 'repo_not_found' }; } filePath = join(sourceLocalPath, `${slug}.md`); + writeRoot = sourceLocalPath; } else { const repoPath = await engine.getConfig('sync.repo_path'); if (!repoPath) { @@ -111,6 +116,16 @@ export async function writePageThrough( return { written: false, skipped: 'source_repo_belongs_to_other_source' }; } filePath = resolvePageFilePath(repoPath, slug, sourceId); + writeRoot = repoPath; + } + + // Defense-in-depth (#1647-slug / codex #6): confirm the computed file path + // stays within the source's working tree before any mkdir/write. validateSlug + // already rejects `..`/backslash/control/%2e in the slug at write time, so + // this guards a pre-existing hostile row or a symlinked intermediate dir + // under the source tree from escaping to an arbitrary filesystem location. + if (!isWriteTargetContained(filePath, writeRoot)) { + return { written: false, skipped: 'path_escapes_source_root' }; } const writtenPage = await engine.getPage(slug, { sourceId }); diff --git a/test/path-confine.test.ts b/test/path-confine.test.ts new file mode 100644 index 000000000..960d2f861 --- /dev/null +++ b/test/path-confine.test.ts @@ -0,0 +1,319 @@ +/** + * Security tests for the shared path-confinement + dotfile-trust helpers + * (src/core/path-confine.ts) and their integration into the source / brain + * resolvers and the skills-dir auto-detector. + * + * Threat model: a multi-user POSIX host where an attacker can write into a + * shared ancestor directory of the victim's CWD. The hardening must refuse a + * routing dotfile (`.gbrain-source` / `.gbrain-mount`) the victim doesn't own, + * and refuse a `skills/` directory that escapes its declared workspace via + * symlink. (#418 / #419, codex #9) + */ + +import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { + mkdtempSync, mkdirSync, writeFileSync, rmSync, symlinkSync, chmodSync, + lstatSync, type Stats, +} from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { + isTrustedDotfile, isPathContained, realpathOrResolve, isWriteTargetContained, +} from '../src/core/path-confine.ts'; +import { validateSlug } from '../src/core/utils.ts'; +import { resolveSourceId } from '../src/core/source-resolver.ts'; +import { resolveBrainId } from '../src/core/brain-resolver.ts'; +import { HOST_BRAIN_ID } from '../src/core/brain-registry.ts'; +import { autoDetectSkillsDir } from '../src/core/repo-root.ts'; +import type { BrainEngine } from '../src/core/engine.ts'; + +// ── helpers ──────────────────────────────────────────────── + +const created: string[] = []; +function scratch(prefix = 'path-confine-'): string { + const dir = mkdtempSync(join(tmpdir(), prefix)); + created.push(dir); + return dir; +} +afterEach(() => { + while (created.length) { + const p = created.pop()!; + try { rmSync(p, { recursive: true, force: true }); } catch { /* ignore */ } + } +}); + +function fakeStats(opts: { uid: number; mode: number; symlink?: boolean }): Stats { + return { + uid: opts.uid, + mode: opts.mode, + isSymbolicLink: () => opts.symlink === true, + } as unknown as Stats; +} + +/** Stub engine for resolveSourceId: registers `ids`, no local_paths, no default. */ +function stubEngine(ids: string[]): BrainEngine { + return { + kind: 'pglite', + executeRaw: async (sql: string, params?: unknown[]): Promise => { + if (sql.includes('SELECT id FROM sources WHERE id = $1')) { + const t = params?.[0]; + return (ids.includes(t as string) ? [{ id: t } as unknown as T] : []); + } + if (sql.includes('SELECT id, local_path FROM sources')) return [] as unknown as T[]; + return [] as unknown as T[]; + }, + getConfig: async () => null, + } as unknown as BrainEngine; +} + +// ── isTrustedDotfile (synthetic Stats — precise branch coverage) ── + +describe('isTrustedDotfile', () => { + const realGetuid = process.getuid; + beforeEach(() => { (process as { getuid?: () => number }).getuid = () => 1000; }); + afterEach(() => { (process as { getuid?: () => number }).getuid = realGetuid; }); + + test('own, not world-writable, not symlink → trusted', () => { + expect(isTrustedDotfile(fakeStats({ uid: 1000, mode: 0o644 }))).toBe(true); + }); + test('foreign-owned → NOT trusted', () => { + expect(isTrustedDotfile(fakeStats({ uid: 2000, mode: 0o644 }))).toBe(false); + }); + test('root-owned → trusted (root-as-root allowance)', () => { + expect(isTrustedDotfile(fakeStats({ uid: 0, mode: 0o644 }))).toBe(true); + }); + test('world-writable, even when owned → NOT trusted', () => { + expect(isTrustedDotfile(fakeStats({ uid: 1000, mode: 0o666 }))).toBe(false); + }); + test('symlink → NOT trusted regardless of ownership', () => { + expect(isTrustedDotfile(fakeStats({ uid: 1000, mode: 0o644, symlink: true }))).toBe(false); + }); + test('no getuid (Windows) → trusted by default', () => { + (process as { getuid?: () => number }).getuid = undefined; + expect(isTrustedDotfile(fakeStats({ uid: 2000, mode: 0o666 }))).toBe(true); + }); +}); + +describe('isTrustedDotfile — real fs (lstat)', () => { + test('a real owned file is trusted; a symlink and a world-writable file are not', () => { + if (typeof process.getuid !== 'function') return; // POSIX-only + const dir = scratch(); + const ownFile = join(dir, 'own'); + writeFileSync(ownFile, 'x'); + expect(isTrustedDotfile(lstatSync(ownFile))).toBe(true); + + const link = join(dir, 'link'); + symlinkSync(ownFile, link); + expect(isTrustedDotfile(lstatSync(link))).toBe(false); + + const ww = join(dir, 'ww'); + writeFileSync(ww, 'x'); + chmodSync(ww, 0o666); + expect(isTrustedDotfile(lstatSync(ww))).toBe(false); + }); +}); + +// ── isPathContained + realpathOrResolve ──────────────────── + +describe('isPathContained', () => { + test('real subdir is contained', () => { + const dir = scratch(); + const sub = join(dir, 'sub'); + mkdirSync(sub); + expect(isPathContained(sub, dir)).toBe(true); + }); + test('symlink escaping the parent is NOT contained', () => { + const dir = scratch(); + const outside = scratch('pc-outside-'); + const link = join(dir, 'escape'); + symlinkSync(outside, link); + expect(isPathContained(link, dir)).toBe(false); + }); + test('symlink resolving INSIDE the parent IS contained', () => { + const dir = scratch(); + const real = join(dir, 'real'); + mkdirSync(real); + const link = join(dir, 'inlink'); + symlinkSync(real, link); + expect(isPathContained(link, dir)).toBe(true); + }); + test('missing path → not contained (fail-closed)', () => { + const dir = scratch(); + expect(isPathContained(join(dir, 'nope'), dir)).toBe(false); + }); + test('sibling prefix does not match (/foo vs /foobar)', () => { + const base = scratch(); + const foo = join(base, 'foo'); const foobar = join(base, 'foobar'); + mkdirSync(foo); mkdirSync(foobar); + expect(isPathContained(foobar, foo)).toBe(false); + }); +}); + +describe('realpathOrResolve', () => { + test('resolves a symlink to its real target', () => { + const dir = scratch(); + const real = join(dir, 'real'); mkdirSync(real); + const link = join(dir, 'link'); symlinkSync(real, link); + expect(realpathOrResolve(link)).toBe(realpathOrResolve(real)); + }); + test('nonexistent path → lexical resolve (does not throw)', () => { + const p = join(scratch(), 'does', 'not', 'exist'); + expect(realpathOrResolve(p)).toBe(p); + }); +}); + +// ── validateSlug dangerous-char hardening (#1647-slug / codex #6) ── + +describe('validateSlug — dangerous-char guard', () => { + test('allows legitimate slugs (lowercase, unicode, dot, underscore, CJK, nested)', () => { + for (const ok of ['notes/2026', 'münchen', 'my_notes', 'a-b/c-d', '会议/纪要', 'readme.v2', 'Mixed-Case']) { + expect(() => validateSlug(ok)).not.toThrow(); + } + }); + test('rejects path traversal and leading slash (existing behavior preserved)', () => { + for (const bad of ['../etc/passwd', 'a/../../b', '/abs/path', '']) { + expect(() => validateSlug(bad)).toThrow(); + } + }); + test('rejects NUL / control bytes', () => { + expect(() => validateSlug("a" + String.fromCharCode(0x00) + "b")).toThrow(/control/); + expect(() => validateSlug("a" + String.fromCharCode(0x1f) + "b")).toThrow(/control/); + }); + test('rejects Unicode bidirectional / RTL overrides', () => { + expect(() => validateSlug("a" + String.fromCharCode(0x202e) + "b")).toThrow(/bidirectional|RTL/); + expect(() => validateSlug("a" + String.fromCharCode(0x2066) + "b")).toThrow(/bidirectional|RTL/); + }); + test('rejects backslashes', () => { + expect(() => validateSlug('a\\b')).toThrow(/[Bb]ackslash/); + }); + test('rejects URL-encoded path separators / traversal', () => { + for (const bad of ['a%2e%2e/b', 'a%2fb', 'a%5cb', 'A%2E%2Eb']) { + expect(() => validateSlug(bad)).toThrow(/URL-encoded/); + } + }); +}); + +// ── isWriteTargetContained (write-through FS sink) ────────── + +describe('isWriteTargetContained', () => { + test('a new file directly under root is contained', () => { + const root = scratch(); + expect(isWriteTargetContained(join(root, 'page.md'), root)).toBe(true); + }); + test('a new file in a new nested dir under root is contained', () => { + const root = scratch(); + expect(isWriteTargetContained(join(root, 'sub', 'deep', 'page.md'), root)).toBe(true); + }); + test('a ../ escape is refused', () => { + const root = scratch(); + expect(isWriteTargetContained(join(root, '..', 'escape.md'), root)).toBe(false); + }); + test('a symlinked intermediate dir escaping the root is refused', () => { + if (typeof process.getuid !== 'function') return; + const root = scratch(); + const outside = scratch('wt-outside-'); + symlinkSync(outside, join(root, 'link')); // root/link → outside + // target lands under the escaping symlink → real path is outside root + expect(isWriteTargetContained(join(root, 'link', 'page.md'), root)).toBe(false); + }); + test('a symlinked intermediate dir staying inside the root is allowed', () => { + const root = scratch(); + mkdirSync(join(root, 'real')); + symlinkSync(join(root, 'real'), join(root, 'link')); + expect(isWriteTargetContained(join(root, 'link', 'page.md'), root)).toBe(true); + }); +}); + +// ── source-resolver integration (#418) ───────────────────── + +describe('resolveSourceId — .gbrain-source dotfile trust', () => { + test('trusted dotfile resolves to its id (control)', async () => { + const dir = scratch(); + writeFileSync(join(dir, '.gbrain-source'), 'evil\n'); + expect(await resolveSourceId(stubEngine(['evil', 'default']), null, dir)).toBe('evil'); + }); + test('symlinked dotfile is REFUSED → falls through to default', async () => { + if (typeof process.getuid !== 'function') return; + const dir = scratch(); + const target = join(dir, 'target'); writeFileSync(target, 'evil\n'); + symlinkSync(target, join(dir, '.gbrain-source')); + expect(await resolveSourceId(stubEngine(['evil', 'default']), null, dir)).toBe('default'); + }); + test('world-writable dotfile is REFUSED → falls through to default', async () => { + if (typeof process.getuid !== 'function') return; + const dir = scratch(); + const df = join(dir, '.gbrain-source'); writeFileSync(df, 'evil\n'); chmodSync(df, 0o666); + expect(await resolveSourceId(stubEngine(['evil', 'default']), null, dir)).toBe('default'); + }); +}); + +// ── brain-resolver integration (#418, brain axis) ────────── + +describe('resolveBrainId — .gbrain-mount dotfile trust', () => { + const noMounts = () => []; + test('trusted dotfile resolves to its id (control)', () => { + const dir = scratch(); + writeFileSync(join(dir, '.gbrain-mount'), 'evil-brain\n'); + expect(resolveBrainId(null, dir, noMounts)).toBe('evil-brain'); + }); + test('symlinked .gbrain-mount is REFUSED → falls through to host', () => { + if (typeof process.getuid !== 'function') return; + const dir = scratch(); + const target = join(dir, 'm'); writeFileSync(target, 'evil-brain\n'); + symlinkSync(target, join(dir, '.gbrain-mount')); + expect(resolveBrainId(null, dir, noMounts)).toBe(HOST_BRAIN_ID); + }); + test('world-writable .gbrain-mount is REFUSED → falls through to host', () => { + if (typeof process.getuid !== 'function') return; + const dir = scratch(); + const df = join(dir, '.gbrain-mount'); writeFileSync(df, 'evil-brain\n'); chmodSync(df, 0o666); + expect(resolveBrainId(null, dir, noMounts)).toBe(HOST_BRAIN_ID); + }); +}); + +// ── repo-root skills-dir confinement (#419) ──────────────── + +describe('autoDetectSkillsDir — skills/ symlink confinement', () => { + function seedSkills(dir: string): void { + mkdirSync(join(dir, 'skills'), { recursive: true }); + writeFileSync(join(dir, 'skills', 'RESOLVER.md'), '# RESOLVER\n'); + } + + test('OPENCLAW_WORKSPACE: escaping skills symlink is refused', () => { + if (typeof process.getuid !== 'function') return; + const ws = scratch('ws-'); const outside = scratch('outside-'); + seedSkills(outside); // real skills with RESOLVER.md, OUTSIDE the workspace + symlinkSync(join(outside, 'skills'), join(ws, 'skills')); // ws/skills → outside/skills + const found = autoDetectSkillsDir(scratch('cwd-'), { OPENCLAW_WORKSPACE: ws }); + expect(found.source).not.toBe('openclaw_workspace_env'); + expect(found.dir).not.toBe(join(ws, 'skills')); + }); + + test('OPENCLAW_WORKSPACE: in-workspace skills symlink is allowed', () => { + const ws = scratch('ws-'); + mkdirSync(join(ws, '_real'), { recursive: true }); + writeFileSync(join(ws, '_real', 'RESOLVER.md'), '# RESOLVER\n'); + symlinkSync(join(ws, '_real'), join(ws, 'skills')); // contained symlink + const found = autoDetectSkillsDir(scratch('cwd-'), { OPENCLAW_WORKSPACE: ws }); + expect(found.source).toBe('openclaw_workspace_env'); + expect(found.dir).toBe(join(ws, 'skills')); + }); + + test('cwd_walk_up: escaping skills symlink is refused', () => { + if (typeof process.getuid !== 'function') return; + const ws = scratch('ws-'); const outside = scratch('outside-'); + mkdirSync(join(outside, 'skills'), { recursive: true }); + symlinkSync(join(outside, 'skills'), join(ws, 'skills')); + const found = autoDetectSkillsDir(ws, {}); + expect(found.dir).toBeNull(); + }); + + test('cwd_walk_up: in-workspace skills symlink is allowed', () => { + const ws = scratch('ws-'); + mkdirSync(join(ws, '_real'), { recursive: true }); + symlinkSync(join(ws, '_real'), join(ws, 'skills')); + const found = autoDetectSkillsDir(ws, {}); + expect(found.source).toBe('cwd_walk_up'); + expect(found.dir).toBe(join(ws, 'skills')); + }); +}); diff --git a/test/transcription-injection.test.ts b/test/transcription-injection.test.ts new file mode 100644 index 000000000..ad0505819 --- /dev/null +++ b/test/transcription-injection.test.ts @@ -0,0 +1,65 @@ +/** + * Security test for #245 — the large-file transcription path must not pass + * caller-controlled audio paths through a shell. transcribeLargeFile now uses + * execFileSync with argument arrays (+ fs.rmSync for cleanup), so a path + * containing shell metacharacters is a literal argv element, never parsed. + * + * transcription.ts is a dead-internally-but-PUBLISHED export (gbrain/transcription), + * so an external programmatic consumer can still call transcribe() with an + * attacker-influenced path — hence the harden over delete. + */ + +import { describe, test, expect } from 'bun:test'; +import { + mkdtempSync, writeFileSync, rmSync, existsSync, readFileSync, +} from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { execFileSync } from 'child_process'; +import { transcribe } from '../src/core/transcription.ts'; + +function ffmpegAvailable(): boolean { + try { execFileSync('ffmpeg', ['-version'], { stdio: 'pipe' }); + execFileSync('ffprobe', ['-version'], { stdio: 'pipe' }); + return true; } catch { return false; } +} + +describe('transcription — command injection (#245)', () => { + // Source-level guard (deterministic, no ffmpeg dependency): the shell-string + // exec forms must be gone. Mirrors the repo's check-*.sh guard philosophy. + test('source uses execFileSync arg-arrays, never shell execSync', () => { + const src = readFileSync(new URL('../src/core/transcription.ts', import.meta.url), 'utf8'); + // `\bexecSync(` matches the dangerous shell-string call but NOT execFileSync. + expect(src).not.toMatch(/\bexecSync\s*\(/); // no shell command strings + expect(src).toMatch(/execFileSync\(/); // arg-array exec + expect(src).toMatch(/rmSync\(/); // fs cleanup, not shell remove + }); + + // Behavioral: a >25MB file whose NAME contains a $(...) payload. Under the old + // shell-string code the double-quoted interpolation command-substitutes the + // payload (creating the sentinel) before ffprobe runs; under execFileSync the + // payload is a literal filename, so no sentinel is ever created. + test('a $(...) payload in the audio path does not spawn a shell', async () => { + if (!ffmpegAvailable()) return; // path requires ffmpeg/ffprobe; skip otherwise + // Sentinel must be a slash-free name so it's a legal single filename segment; + // a shell `touch ` would create it in process.cwd(), so we check there. + const sentinelName = `gbrain-inj-sentinel-${process.pid}-${Date.now()}`; + const sentinelPath = join(process.cwd(), sentinelName); + if (existsSync(sentinelPath)) rmSync(sentinelPath, { force: true }); + const dir = mkdtempSync(join(tmpdir(), 'transcribe-inj-')); + // Payload touches the sentinel IF a shell ever parses the path. + const evil = join(dir, `clip$(touch ${sentinelName}).mp3`); + writeFileSync(evil, Buffer.alloc(26 * 1024 * 1024)); // >25MB → large-file path + try { + await transcribe(evil, { apiKey: 'dummy', provider: 'groq' }); + } catch { + // Expected: ffprobe/ffmpeg reject the garbage file, or transcribeFile has + // no real key. We only care that no shell ran. + } finally { + rmSync(dir, { recursive: true, force: true }); + } + const pwned = existsSync(sentinelPath); + if (pwned) rmSync(sentinelPath, { force: true }); + expect(pwned).toBe(false); + }, 30_000); +});