fix(config): harden include file loading path checks
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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 });
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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),
|
||||
};
|
||||
|
||||
|
||||
@@ -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<string, string>();
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user