diff --git a/CHANGELOG.md b/CHANGELOG.md index e473f2267..13056320a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ Docs: https://docs.openclaw.ai - Agents/Models config: preserve agent-level provider `apiKey` and `baseUrl` during merge-mode `models.json` updates when agent values are present. (#27293) thanks @Sid-Qin. - Docker/GCP onboarding: reduce first-build OOM risk by capping Node heap during `pnpm install`, reuse existing gateway token during `docker-setup.sh` reruns so `.env` stays aligned with config, auto-bootstrap Control UI allowed origins for non-loopback Docker binds, and add GCP docs guidance for tokenized dashboard links + pairing recovery commands. (#26253) Thanks @pandego. - Pairing/Multi-account isolation: keep non-default account pairing allowlists and pending requests strictly account-scoped, while default account continues to use channel-scoped pairing allowlist storage. Thanks @gumadeiras. +- Security/Config includes: harden `$include` file loading with verified-open reads, reject hardlinked include aliases, and enforce include file-size guardrails so config include resolution remains bounded to trusted in-root files. This ships in the next npm release (`2026.2.26`). Thanks @zpbrent for reporting. ## 2026.2.25 diff --git a/src/config/includes.test.ts b/src/config/includes.test.ts index 38360642e..188039637 100644 --- a/src/config/includes.test.ts +++ b/src/config/includes.test.ts @@ -5,6 +5,7 @@ import { describe, expect, it } from "vitest"; import { CircularIncludeError, ConfigIncludeError, + MAX_INCLUDE_FILE_BYTES, deepMerge, type IncludeResolver, resolveConfigIncludes, @@ -640,5 +641,55 @@ describe("security: path traversal protection (CWE-22)", () => { await fs.rm(tempRoot, { recursive: true, force: true }); } }); + + it("rejects include files that are hardlinked aliases", async () => { + if (process.platform === "win32") { + return; + } + const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-includes-hardlink-")); + try { + const configDir = path.join(tempRoot, "config"); + const outsideDir = path.join(tempRoot, "outside"); + await fs.mkdir(configDir, { recursive: true }); + await fs.mkdir(outsideDir, { recursive: true }); + const includePath = path.join(configDir, "extra.json5"); + const outsidePath = path.join(outsideDir, "secret.json5"); + await fs.writeFile(outsidePath, '{"logging":{"redactSensitive":"tools"}}\n', "utf-8"); + try { + await fs.link(outsidePath, includePath); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "EXDEV") { + return; + } + throw err; + } + + expect(() => + resolveConfigIncludes( + { $include: "./extra.json5" }, + path.join(configDir, "openclaw.json"), + ), + ).toThrow(/security checks|hardlink/i); + } finally { + await fs.rm(tempRoot, { recursive: true, force: true }); + } + }); + + it("rejects oversized include files", async () => { + const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-includes-big-")); + try { + const configDir = path.join(tempRoot, "config"); + await fs.mkdir(configDir, { recursive: true }); + const includePath = path.join(configDir, "big.json5"); + const payload = "a".repeat(MAX_INCLUDE_FILE_BYTES + 1); + await fs.writeFile(includePath, `{"blob":"${payload}"}`, "utf-8"); + + expect(() => + resolveConfigIncludes({ $include: "./big.json5" }, path.join(configDir, "openclaw.json")), + ).toThrow(/security checks|max/i); + } finally { + await fs.rm(tempRoot, { recursive: true, force: true }); + } + }); }); }); diff --git a/src/config/includes.ts b/src/config/includes.ts index c9a14a363..0d87a2ae3 100644 --- a/src/config/includes.ts +++ b/src/config/includes.ts @@ -13,12 +13,14 @@ import fs from "node:fs"; import path from "node:path"; import JSON5 from "json5"; +import { openVerifiedFileSync } from "../infra/safe-open-sync.js"; import { isPathInside } from "../security/scan-paths.js"; import { isPlainObject } from "../utils.js"; import { isBlockedObjectKey } from "./prototype-keys.js"; export const INCLUDE_KEY = "$include"; export const MAX_INCLUDE_DEPTH = 10; +export const MAX_INCLUDE_FILE_BYTES = 2 * 1024 * 1024; // ============================================================================ // Types @@ -26,9 +28,18 @@ export const MAX_INCLUDE_DEPTH = 10; export type IncludeResolver = { readFile: (path: string) => string; + readFileWithGuards?: (params: IncludeFileReadParams) => string; parseJson: (raw: string) => unknown; }; +export type IncludeFileReadParams = { + includePath: string; + resolvedPath: string; + rootRealDir: string; + ioFs?: typeof fs; + maxBytes?: number; +}; + // ============================================================================ // Errors // ============================================================================ @@ -227,8 +238,18 @@ class IncludeProcessor { private readFile(includePath: string, resolvedPath: string): string { try { + if (this.resolver.readFileWithGuards) { + return this.resolver.readFileWithGuards({ + includePath, + resolvedPath, + rootRealDir: this.rootRealDir, + }); + } return this.resolver.readFile(resolvedPath); } catch (err) { + if (err instanceof ConfigIncludeError) { + throw err; + } throw new ConfigIncludeError( `Failed to read include file: ${includePath} (resolved: ${resolvedPath})`, includePath, @@ -265,12 +286,78 @@ function safeRealpath(target: string): string { } } +function canUseVerifiedFileOpen(ioFs: typeof fs): boolean { + return ( + typeof ioFs.openSync === "function" && + typeof ioFs.closeSync === "function" && + typeof ioFs.fstatSync === "function" && + typeof ioFs.lstatSync === "function" && + typeof ioFs.realpathSync === "function" && + typeof ioFs.readFileSync === "function" && + typeof ioFs.constants === "object" && + ioFs.constants !== null + ); +} + +export function readConfigIncludeFileWithGuards(params: IncludeFileReadParams): string { + const ioFs = params.ioFs ?? fs; + const maxBytes = params.maxBytes ?? MAX_INCLUDE_FILE_BYTES; + let realPath = params.resolvedPath; + try { + realPath = ioFs.realpathSync(params.resolvedPath); + if (!isPathInside(params.rootRealDir, realPath)) { + throw new ConfigIncludeError( + `Include path resolves outside config directory (symlink): ${params.includePath}`, + params.includePath, + ); + } + } catch (err) { + if (err instanceof ConfigIncludeError) { + throw err; + } + // File may not exist yet; read path will report a precise error below. + } + + if (!canUseVerifiedFileOpen(ioFs)) { + return ioFs.readFileSync(params.resolvedPath, "utf-8"); + } + + const opened = openVerifiedFileSync({ + filePath: params.resolvedPath, + resolvedPath: realPath, + rejectHardlinks: true, + maxBytes, + ioFs, + }); + if (!opened.ok) { + if (opened.reason === "validation") { + throw new ConfigIncludeError( + `Include file failed security checks (regular file, max ${maxBytes} bytes, no hardlinks): ${params.includePath}`, + params.includePath, + ); + } + throw new ConfigIncludeError( + `Failed to read include file: ${params.includePath} (resolved: ${params.resolvedPath})`, + params.includePath, + opened.error instanceof Error ? opened.error : undefined, + ); + } + + try { + return ioFs.readFileSync(opened.fd, "utf-8"); + } finally { + ioFs.closeSync(opened.fd); + } +} + // ============================================================================ // Public API // ============================================================================ const defaultResolver: IncludeResolver = { readFile: (p) => fs.readFileSync(p, "utf-8"), + readFileWithGuards: ({ includePath, resolvedPath, rootRealDir }) => + readConfigIncludeFileWithGuards({ includePath, resolvedPath, rootRealDir }), parseJson: (raw) => JSON5.parse(raw), }; diff --git a/src/config/io.ts b/src/config/io.ts index c74992c49..89d6b3326 100644 --- a/src/config/io.ts +++ b/src/config/io.ts @@ -34,7 +34,11 @@ import { resolveConfigEnvVars, } from "./env-substitution.js"; import { applyConfigEnvVars } from "./env-vars.js"; -import { ConfigIncludeError, resolveConfigIncludes } from "./includes.js"; +import { + ConfigIncludeError, + readConfigIncludeFileWithGuards, + resolveConfigIncludes, +} from "./includes.js"; import { findLegacyConfigIssues } from "./legacy.js"; import { applyMergePatch } from "./merge-patch.js"; import { normalizeExecSafeBinProfilesInConfig } from "./normalize-exec-safe-bin.js"; @@ -634,6 +638,13 @@ function resolveConfigIncludesForRead( ): unknown { return resolveConfigIncludes(parsed, configPath, { readFile: (candidate) => deps.fs.readFileSync(candidate, "utf-8"), + readFileWithGuards: ({ includePath, resolvedPath, rootRealDir }) => + readConfigIncludeFileWithGuards({ + includePath, + resolvedPath, + rootRealDir, + ioFs: deps.fs, + }), parseJson: (raw) => deps.json5.parse(raw), }); } @@ -1032,6 +1043,13 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { try { const resolvedIncludes = resolveConfigIncludes(snapshot.parsed, configPath, { readFile: (candidate) => deps.fs.readFileSync(candidate, "utf-8"), + readFileWithGuards: ({ includePath, resolvedPath, rootRealDir }) => + readConfigIncludeFileWithGuards({ + includePath, + resolvedPath, + rootRealDir, + ioFs: deps.fs, + }), parseJson: (raw) => deps.json5.parse(raw), }); const collected = new Map(); diff --git a/src/infra/safe-open-sync.ts b/src/infra/safe-open-sync.ts index 311849ba9..b502b04e9 100644 --- a/src/infra/safe-open-sync.ts +++ b/src/infra/safe-open-sync.ts @@ -7,9 +7,10 @@ export type SafeOpenSyncResult = | { ok: true; path: string; fd: number; stat: fs.Stats } | { ok: false; reason: SafeOpenSyncFailureReason; error?: unknown }; -const OPEN_READ_FLAGS = - fs.constants.O_RDONLY | - (typeof fs.constants.O_NOFOLLOW === "number" ? fs.constants.O_NOFOLLOW : 0); +type SafeOpenSyncFs = Pick< + typeof fs, + "constants" | "lstatSync" | "realpathSync" | "openSync" | "fstatSync" | "closeSync" +>; function isExpectedPathError(error: unknown): boolean { const code = @@ -25,31 +26,43 @@ export function openVerifiedFileSync(params: { filePath: string; resolvedPath?: string; rejectPathSymlink?: boolean; + rejectHardlinks?: boolean; maxBytes?: number; + ioFs?: SafeOpenSyncFs; }): SafeOpenSyncResult { + const ioFs = params.ioFs ?? fs; + const openReadFlags = + ioFs.constants.O_RDONLY | + (typeof ioFs.constants.O_NOFOLLOW === "number" ? ioFs.constants.O_NOFOLLOW : 0); let fd: number | null = null; try { if (params.rejectPathSymlink) { - const candidateStat = fs.lstatSync(params.filePath); + const candidateStat = ioFs.lstatSync(params.filePath); if (candidateStat.isSymbolicLink()) { return { ok: false, reason: "validation" }; } } - const realPath = params.resolvedPath ?? fs.realpathSync(params.filePath); - const preOpenStat = fs.lstatSync(realPath); + const realPath = params.resolvedPath ?? ioFs.realpathSync(params.filePath); + const preOpenStat = ioFs.lstatSync(realPath); if (!preOpenStat.isFile()) { return { ok: false, reason: "validation" }; } + if (params.rejectHardlinks && preOpenStat.nlink > 1) { + return { ok: false, reason: "validation" }; + } if (params.maxBytes !== undefined && preOpenStat.size > params.maxBytes) { return { ok: false, reason: "validation" }; } - fd = fs.openSync(realPath, OPEN_READ_FLAGS); - const openedStat = fs.fstatSync(fd); + fd = ioFs.openSync(realPath, openReadFlags); + const openedStat = ioFs.fstatSync(fd); if (!openedStat.isFile()) { return { ok: false, reason: "validation" }; } + if (params.rejectHardlinks && openedStat.nlink > 1) { + return { ok: false, reason: "validation" }; + } if (params.maxBytes !== undefined && openedStat.size > params.maxBytes) { return { ok: false, reason: "validation" }; } @@ -67,7 +80,7 @@ export function openVerifiedFileSync(params: { return { ok: false, reason: "io", error }; } finally { if (fd !== null) { - fs.closeSync(fd); + ioFs.closeSync(fd); } } }