diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index d575db4e3..7378564f0 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -3351,13 +3351,19 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load return out; } - // Why these three checks rather than security.cjs's `validatePath`: that seam - // resolves symlinks with realpathSync and then tests containment, so a link - // whose target sits inside the config dir passes. For a restore that is still - // wrong — writing through any link overwrites whatever it points at instead - // of materializing a regular file at the backed-up path. These checks reject - // links outright, which is strictly stricter than validatePath, not a - // reimplementation of it. Do not "simplify" this to validatePath. + // Why these three checks rather than security.cjs's `assertWithinRoot` / + // `tryWithinRoot`: that seam resolves symlinks with realpathSync and then + // tests containment, so a link whose target sits inside the config dir + // passes. For a restore that is still wrong — writing through any link + // overwrites whatever it points at instead of materializing a regular file + // at the backed-up path. These checks reject links outright, which is + // strictly stricter than assertWithinRoot/tryWithinRoot, not a + // reimplementation of them. Do not "simplify" this to assertWithinRoot or + // tryWithinRoot. Reviewed under epic #4636 Phase 3 and deliberately NOT + // collapsed. Note also: isInsideDir below treats target === root as NOT + // contained (it requires a non-empty relative path), unlike every other + // containment implementation in this repo, which treats target === root as + // contained. /** True when `target` resolves strictly inside `root`. */ function isInsideDir(root, target) { diff --git a/scripts/check-glossary-refs.cjs b/scripts/check-glossary-refs.cjs index d0bcd4d95..4a87a671b 100644 --- a/scripts/check-glossary-refs.cjs +++ b/scripts/check-glossary-refs.cjs @@ -37,6 +37,7 @@ const fs = require('node:fs'); const path = require('node:path'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +const { tryWithinRoot, PathAcceptance } = require('../gsd-core/bin/lib/security.cjs'); const ROOT = path.resolve(__dirname, '..'); const CONTEXT_PATH = path.join(ROOT, 'CONTEXT.md'); @@ -131,20 +132,6 @@ function isTracked(token) { return TRACKED_EXACT.has(token) || TRACKED_PREFIXES.some((prefix) => token.startsWith(prefix)); } -/** - * True if joining `token` to ROOT stays inside ROOT. `PATH_TOKEN_RE` admits `.` - * inside a segment, so a token like `src/../../../etc/passwd` matches and (via - * the `src/` prefix) reads as "tracked" — `path.join(ROOT, token)` would then - * normalize to an out-of-tree absolute path and `fs.existsSync` would probe it, - * turning a doc lint into a filesystem-existence oracle on the CI host. A - * CONTEXT.md reference is always a plain in-repo path, so a `..` escape is never - * legitimate: confine to ROOT and drop anything that climbs out. - */ -function isWithinRoot(token) { - const resolved = path.resolve(ROOT, token); - return resolved === ROOT || resolved.startsWith(ROOT + path.sep); -} - /** * Every distinct, trackable file-path token referenced in `text`, with any * trailing `:` suffix stripped. @@ -177,7 +164,8 @@ function extractTrackedRefs(text) { if (!/[A-Za-z0-9_]$/.test(token)) return; if (token.includes('NNNN')) return; if (!isTracked(token)) return; - if (!isWithinRoot(token)) return; + // Containment decision is the canonical predicate's, per ADR-4650. + if (tryWithinRoot(token, ROOT, PathAcceptance.AbsoluteInsideRoot) === null) return; tokens.add(token); }; const subTokenRe = /[\w.-]+(?:\/[\w.-]+)*/g; diff --git a/src/external-descriptor-trust.cts b/src/external-descriptor-trust.cts index 4bd346361..ce3ee854f 100644 --- a/src/external-descriptor-trust.cts +++ b/src/external-descriptor-trust.cts @@ -32,11 +32,17 @@ import path from 'node:path'; * `root`) that would redirect the LEXICALLY-confined path to a physically * different, unconfined location on disk. A caller relying on this for a * write-confinement guarantee against a symlink-planting attacker must pair - * it with a symlink check (or refuse to follow symlinks at write time) — see - * capability-source.cts's install adapters (:491,577,675), which is what - * currently keeps every caller of this function's callers symlink-safe: they - * reject symlinks upstream, before a target ever reaches a lexical-only check - * like this one. + * it with a symlink check (or refuse to follow symlinks at write time). Only + * the capability-loader.cts route into assertDescriptorConfined gets this for + * free today: capability-source.cts's staging path rejects symlinks upstream, + * before a target ever reaches a lexical-only check like this one — see + * copyDirRecursive's `entry.isSymbolicLink()` throw (capability-source.cts:585-586) + * and the post-copy budget-walk re-check (capability-source.cts:671-674). This + * does NOT extend to isPathConfined's other callers: install-engine.cts:1608 + * and install-profiles.cts:755,880 do not go through capability-source.cts's + * adapters at all and have no symlink guard of their own here. Of the + * remaining callers, only retired-artifact-cleanup.cts:69 carries its own + * defense, via a local `lstatSync(destDir).isSymbolicLink()` check at line 77. * * `opts.pathImpl` (default: the ambient `path` module) lets a caller inject * `path.win32` or `path.posix`. This is security-relevant: the win32 branch diff --git a/src/installer-migrations.cts b/src/installer-migrations.cts index 4a2eb0a9e..775877acd 100644 --- a/src/installer-migrations.cts +++ b/src/installer-migrations.cts @@ -19,6 +19,7 @@ import { import { platformWriteSync, retryRenameSync, posixNormalize } from './shell-command-projection.cjs'; import { realClock, type Clock } from './clock.cjs'; import { isInstallScopeId, type InstallScope } from './install-scope.cjs'; +import { tryWithinRoot, PathAcceptance } from './security.cjs'; // #2874 (ADR-58 cleanup phase): this file is the ~1200-line migration // plan/apply/rollback/lock/journal engine — almost none of it is on the // installRuntimeArtifacts call tree. Only `readInstallManifest` and @@ -666,9 +667,16 @@ interface EnsureInsideConfigResult { function ensureInsideConfig(configDir: string, relPath: string): EnsureInsideConfigResult { const normalized = normalizeRelPath(relPath); + // fullPath stays the LEXICAL path.resolve result (not the canonical + // predicate's realpath-resolved value): both callers (readJson's + // ensureInsideConfig call and the migration-apply loop) use fullPath for + // fs.existsSync checks and journal entries, and those must not shift if + // configDir happens to be a symlink. Per ADR-4650 decision 6, the + // containment DECISION (whether fullPath is inside configDir) is owned by + // the canonical predicate — this wrapper only decides how to degrade + // (throw with this file's existing message), never whether contained. const fullPath = path.resolve(configDir, normalized); - const root = path.resolve(configDir); - if (fullPath !== root && !fullPath.startsWith(root + path.sep)) { + if (tryWithinRoot(fullPath, configDir, PathAcceptance.AbsoluteInsideRoot) === null) { throw new Error(`migration path escapes configDir: ${relPath}`); } return { normalized, fullPath }; diff --git a/src/planning-inspect.cts b/src/planning-inspect.cts index fe60b61e8..a6d175e50 100644 --- a/src/planning-inspect.cts +++ b/src/planning-inspect.cts @@ -92,6 +92,9 @@ const { parseMarkdownTable, matchTableSchema } = markdownTable; // eslint-disable-next-line @typescript-eslint/no-require-imports import coreUtilsMod = require('./core-utils.cjs'); const { normalizeLineEndings } = coreUtilsMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import securityMod = require('./security.cjs'); +const { tryWithinRoot, PathAcceptance } = securityMod; /** * The wire schema version. A consumer MUST reject any value other than this @@ -212,32 +215,45 @@ function toPosix(value: string): string { * directory like `.planning-evil/` that merely shares a string prefix. Pure * string comparison, no I/O — callers own their own `fs.realpathSync` call * (and its own not-found/broken-symlink handling). + * + * NOT an independent containment implementation — it is the comparison step + * of one. `readDocument` below realpaths target and root itself (to keep its + * own exists-vs-escaped tri-state) and calls this directly; `isPathContained` + * gets its containment DECISION from the canonical `tryWithinRoot` predicate + * instead (ADR-4650 decision 6) and no longer uses this function. Every + * caller owns its own resolution. */ function isWithinRoot(resolvedTarget: string, resolvedRoot: string): boolean { return resolvedTarget === resolvedRoot || resolvedTarget.startsWith(resolvedRoot + path.sep); } /** - * Containment check for a path (file OR directory) that resolves its own - * `fs.realpathSync`, then delegates the actual boundary comparison to - * `isWithinRoot`. Used where the caller does not need to distinguish "target - * vanished / broken symlink" from "target resolved but escapes root" — both - * degrade the same way at every call site that uses this (an escaped or - * unresolvable phase directory is treated identically to an unreadable one). - * `readDocument` below needs that distinction for its own exists/readable - * tri-state, so it keeps its own inline `realpathSync` calls and calls - * `isWithinRoot` directly instead of this wrapper. + * Containment check for a path (file OR directory), used where the caller + * does not need to distinguish "target vanished / broken symlink" from + * "target resolved but escapes root" — both degrade the same way at every + * call site that uses this (an escaped or unresolvable phase directory is + * treated identically to an unreadable one). `readDocument` below needs + * that distinction for its own exists/readable tri-state, so it keeps its + * own inline `realpathSync` calls and calls `isWithinRoot` directly instead + * of this wrapper. + * + * The containment DECISION comes from the canonical `tryWithinRoot` + * predicate (ADR-4650 decision 6: a wrapper may decide HOW to degrade, + * never WHETHER a path is contained). Must-exist stays this module's OWN + * degradation condition, applied after: `tryWithinRoot` deliberately accepts + * a not-yet-created path under the root (ancestor-walk realpath), but every + * caller of `isPathContained` guards an `fs` read that is about to happen + * against an already-existing directory, so a vanished/unresolvable path + * must still degrade the same as an escaped one. */ function isPathContained(target: string, root: string): boolean { - let realTarget: string; - let realRoot: string; + if (tryWithinRoot(target, root, PathAcceptance.AbsoluteInsideRoot) === null) return false; try { - realTarget = fs.realpathSync(target); - realRoot = fs.realpathSync(root); + fs.realpathSync(target); } catch { return false; } - return isWithinRoot(realTarget, realRoot); + return true; } function readDocument(filePath: string, root: string): { text: string | null; exists: boolean; readable: boolean } {