From d1c00dbb7c64a39e205464dae7f2a068420e91c1 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 18 Feb 2026 03:26:36 +0100 Subject: [PATCH] fix: harden include confinement edge cases (#18652) (thanks @aether-ai-agent) --- CHANGELOG.md | 1 + docs/gateway/configuration-reference.md | 2 +- ...doctor-config-flow.include-warning.test.ts | 38 +++++++++++++++ src/commands/doctor-config-flow.ts | 25 ++++++++++ src/config/includes.test.ts | 46 +++++++++++++++++-- src/config/includes.ts | 26 ++++++++--- 6 files changed, 126 insertions(+), 12 deletions(-) create mode 100644 src/commands/doctor-config-flow.include-warning.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index fcc3e2aff6..4f2e426393 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,7 @@ Docs: https://docs.openclaw.ai - BlueBubbles: match outbound message-id fallback recovery by chat identifier as well as account context. Thanks @tyler6204. - BlueBubbles: include sender identifier in untrusted conversation metadata for conversation info payloads. Thanks @tyler6204. - Security/Exec: fix the OC-09 credential-theft path via environment-variable injection. (#18048) Thanks @aether-ai-agent. +- Security/Config: confine `$include` resolution to the top-level config directory, harden traversal/symlink checks with cross-platform-safe path containment, and add doctor hints for invalid escaped include paths. (#18652) Thanks @aether-ai-agent. - Providers: improve error messaging for unconfigured local `ollama`/`vllm` providers. (#18183) Thanks @arosstale. - TTS: surface all provider errors instead of only the last error in aggregated failures. (#17964) Thanks @ikari-pl. - CLI/Doctor/Configure: skip gateway auth checks for loopback-only setups. (#18407) Thanks @sggolakiya. diff --git a/docs/gateway/configuration-reference.md b/docs/gateway/configuration-reference.md index 90ec0fb5fb..5f551a2de5 100644 --- a/docs/gateway/configuration-reference.md +++ b/docs/gateway/configuration-reference.md @@ -2435,7 +2435,7 @@ Split config into multiple files: - Array of files: deep-merged in order (later overrides earlier). - Sibling keys: merged after includes (override included values). - Nested includes: up to 10 levels deep. -- Paths: relative (to the including file), absolute, or `../` parent references. +- Paths: resolved relative to the including file, but must stay inside the top-level config directory (`dirname` of the main config file). Absolute/`../` forms are allowed only when they still resolve inside that boundary. - Errors: clear messages for missing files, parse errors, and circular includes. --- diff --git a/src/commands/doctor-config-flow.include-warning.test.ts b/src/commands/doctor-config-flow.include-warning.test.ts new file mode 100644 index 0000000000..504a6d84b8 --- /dev/null +++ b/src/commands/doctor-config-flow.include-warning.test.ts @@ -0,0 +1,38 @@ +import fs from "node:fs/promises"; +import path from "node:path"; +import { describe, expect, it, vi } from "vitest"; +import { withTempHome } from "../../test/helpers/temp-home.js"; + +const { noteSpy } = vi.hoisted(() => ({ + noteSpy: vi.fn(), +})); + +vi.mock("../terminal/note.js", () => ({ + note: noteSpy, +})); + +import { loadAndMaybeMigrateDoctorConfig } from "./doctor-config-flow.js"; + +describe("doctor include warning", () => { + it("surfaces include confinement hint for escaped include paths", async () => { + await withTempHome(async (home) => { + const configDir = path.join(home, ".openclaw"); + await fs.mkdir(configDir, { recursive: true }); + await fs.writeFile( + path.join(configDir, "openclaw.json"), + JSON.stringify({ $include: "/etc/passwd" }, null, 2), + "utf-8", + ); + + await loadAndMaybeMigrateDoctorConfig({ + options: { nonInteractive: true }, + confirm: async () => false, + }); + }); + + expect(noteSpy).toHaveBeenCalledWith( + expect.stringContaining("$include paths must stay under:"), + "Doctor warnings", + ); + }); +}); diff --git a/src/commands/doctor-config-flow.ts b/src/commands/doctor-config-flow.ts index d79cf150a2..323eb07f71 100644 --- a/src/commands/doctor-config-flow.ts +++ b/src/commands/doctor-config-flow.ts @@ -147,6 +147,30 @@ function noteOpencodeProviderOverrides(cfg: OpenClawConfig) { note(lines.join("\n"), "OpenCode Zen"); } +function noteIncludeConfinementWarning(snapshot: { + path?: string | null; + issues?: Array<{ message: string }>; +}): void { + const issues = snapshot.issues ?? []; + const includeIssue = issues.find( + (issue) => + issue.message.includes("Include path escapes config directory") || + issue.message.includes("Include path resolves outside config directory"), + ); + if (!includeIssue) { + return; + } + const configRoot = path.dirname(snapshot.path ?? CONFIG_PATH); + note( + [ + `- $include paths must stay under: ${configRoot}`, + '- Move shared include files under that directory and update to relative paths like "./shared/common.json".', + `- Error: ${includeIssue.message}`, + ].join("\n"), + "Doctor warnings", + ); +} + type TelegramAllowFromUsernameHit = { path: string; entry: string }; type TelegramAllowFromListRef = { @@ -758,6 +782,7 @@ export async function loadAndMaybeMigrateDoctorConfig(params: { const fixHints: string[] = []; if (snapshot.exists && !snapshot.valid && snapshot.legacyIssues.length === 0) { note("Config invalid; doctor will run with best-effort config.", "Config"); + noteIncludeConfinementWarning(snapshot); } const warnings = snapshot.warnings ?? []; if (warnings.length > 0) { diff --git a/src/config/includes.test.ts b/src/config/includes.test.ts index 6c6b506e95..35facb8b3d 100644 --- a/src/config/includes.test.ts +++ b/src/config/includes.test.ts @@ -1,3 +1,5 @@ +import fs from "node:fs/promises"; +import os from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; import { @@ -287,6 +289,17 @@ describe("resolveConfigIncludes", () => { /escapes config directory/, ); }); + + it("allows nested parent traversal when path stays under top-level config directory", () => { + const files = { + [configPath("sub", "child.json")]: { $include: "../shared/common.json" }, + [configPath("shared", "common.json")]: { shared: true }, + }; + const obj = { $include: "./sub/child.json" }; + expect(resolve(obj, files)).toEqual({ + shared: true, + }); + }); }); describe("real-world config patterns", () => { @@ -520,12 +533,35 @@ describe("security: path traversal protection (CWE-22)", () => { expect(() => resolve(obj, {})).toThrow(ConfigIncludeError); }); - it("handles config at filesystem root edge case", () => { + it("allows child include when config is at filesystem root", () => { const rootConfigPath = path.join(path.parse(process.cwd()).root, "test.json"); - const files = { [rootConfigPath]: { root: true } }; - // Even at root, absolute paths to other locations should be rejected - const obj = { $include: "/etc/passwd" }; - expect(() => resolve(obj, files, rootConfigPath)).toThrow(ConfigIncludeError); + const childPath = path.join(path.parse(process.cwd()).root, "child.json"); + const files = { [childPath]: { root: true } }; + const obj = { $include: childPath }; + expect(resolve(obj, files, rootConfigPath)).toEqual({ root: true }); + }); + + it("allows include files when the config root path is a symlink", async () => { + const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-includes-symlink-")); + try { + const realRoot = path.join(tempRoot, "real"); + const linkRoot = path.join(tempRoot, "link"); + await fs.mkdir(path.join(realRoot, "includes"), { recursive: true }); + await fs.writeFile( + path.join(realRoot, "includes", "extra.json5"), + "{ logging: { redactSensitive: 'tools' } }\n", + "utf-8", + ); + await fs.symlink(realRoot, linkRoot); + + const result = resolveConfigIncludes( + { $include: "./includes/extra.json5" }, + path.join(linkRoot, "openclaw.json"), + ); + expect(result).toEqual({ logging: { redactSensitive: "tools" } }); + } finally { + await fs.rm(tempRoot, { recursive: true, force: true }); + } }); }); }); diff --git a/src/config/includes.ts b/src/config/includes.ts index 174e41bd2d..e265186bf7 100644 --- a/src/config/includes.ts +++ b/src/config/includes.ts @@ -13,6 +13,7 @@ import fs from "node:fs"; import path from "node:path"; import JSON5 from "json5"; +import { isPathInside } from "../security/scan-paths.js"; import { isPlainObject } from "../utils.js"; export const INCLUDE_KEY = "$include"; @@ -75,12 +76,17 @@ export function deepMerge(target: unknown, source: unknown): unknown { class IncludeProcessor { private visited = new Set(); private depth = 0; + private readonly rootDir: string; + private readonly rootRealDir: string; constructor( private basePath: string, private resolver: IncludeResolver, + rootDir?: string, ) { this.visited.add(path.normalize(basePath)); + this.rootDir = path.normalize(rootDir ?? path.dirname(basePath)); + this.rootRealDir = path.normalize(safeRealpath(this.rootDir)); } process(obj: unknown): unknown { @@ -173,10 +179,10 @@ class IncludeProcessor { : path.resolve(configDir, includePath); const normalized = path.normalize(resolved); - // SECURITY: Reject paths outside config directory (CWE-22: Path Traversal) - if (!normalized.startsWith(configDir + path.sep) && normalized !== configDir) { + // SECURITY: Reject paths outside top-level config directory (CWE-22: Path Traversal) + if (!isPathInside(this.rootDir, normalized)) { throw new ConfigIncludeError( - `Include path escapes config directory: ${includePath}`, + `Include path escapes config directory: ${includePath} (root: ${this.rootDir})`, includePath, ); } @@ -184,9 +190,9 @@ class IncludeProcessor { // SECURITY: Resolve symlinks and re-validate to prevent symlink bypass try { const real = fs.realpathSync(normalized); - if (!real.startsWith(configDir + path.sep) && real !== configDir) { + if (!isPathInside(this.rootRealDir, real)) { throw new ConfigIncludeError( - `Include path resolves outside config directory (symlink): ${includePath}`, + `Include path resolves outside config directory (symlink): ${includePath} (root: ${this.rootDir})`, includePath, ); } @@ -240,13 +246,21 @@ class IncludeProcessor { } private processNested(resolvedPath: string, parsed: unknown): unknown { - const nested = new IncludeProcessor(resolvedPath, this.resolver); + const nested = new IncludeProcessor(resolvedPath, this.resolver, this.rootDir); nested.visited = new Set([...this.visited, resolvedPath]); nested.depth = this.depth + 1; return nested.process(parsed); } } +function safeRealpath(target: string): string { + try { + return fs.realpathSync(target); + } catch { + return target; + } +} + // ============================================================================ // Public API // ============================================================================