diff --git a/CHANGELOG.md b/CHANGELOG.md index 36950bbdf..ee4cdc59c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ Docs: https://docs.openclaw.ai - Security/ACP: harden ACP bridge session management with duplicate-session refresh, idle-session reaping, oldest-idle soft-cap eviction, and burst rate limiting on session creation to reduce local DoS risk without disrupting normal IDE usage. - Security/Plugins/Hooks: add optional `--pin` for npm plugin/hook installs, persist resolved npm metadata (`name`, `version`, `spec`, integrity, shasum, timestamp), warn/confirm on integrity drift during updates, and extend `openclaw security audit` to flag unpinned specs, missing integrity metadata, and install-record version drift. - Security/Plugins: harden plugin discovery by blocking unsafe candidates (root escapes, world-writable paths, suspicious ownership), add startup warnings when `plugins.allow` is empty with discoverable non-bundled plugins, and warn on loaded plugins without install/load-path provenance. +- Refactor/Plugins: extract shared plugin path-safety utilities, split discovery safety checks into typed reasoned guards, precompute provenance matchers during plugin load, and switch ownership tests to injected uid inputs. - Security/Gateway: rate-limit control-plane write RPCs (`config.apply`, `config.patch`, `update.run`) to 3 requests per minute per `deviceId+clientIp`, add restart single-flight coalescing plus a 30-second restart cooldown, and log actor/device/ip with changed-path audit details for config/update-triggered restarts. - Commands/Doctor: skip embedding-provider warnings when `memory.backend` is `qmd`, because QMD manages embeddings internally and does not require `memorySearch` providers. (#17263) Thanks @miloudbelarebia. - Security/Webhooks: harden Feishu and Zalo webhook ingress with webhook-mode token preconditions, loopback-default Feishu bind host, JSON content-type enforcement, per-path rate limiting, replay dedupe for Zalo events, constant-time Zalo secret comparison, and anomaly status counters. diff --git a/src/plugins/discovery.test.ts b/src/plugins/discovery.test.ts index 1a81a4602..11a571646 100644 --- a/src/plugins/discovery.test.ts +++ b/src/plugins/discovery.test.ts @@ -206,21 +206,14 @@ describe("discoverOpenClawPlugins", () => { "utf-8", ); - const proc = process as NodeJS.Process & { getuid: () => number }; - const originalGetUid = proc.getuid; - const actualUid = originalGetUid(); - try { - proc.getuid = () => actualUid + 1; - const result = await withStateDir(stateDir, async () => { - return discoverOpenClawPlugins({}); - }); - expect(result.candidates).toHaveLength(0); - expect( - result.diagnostics.some((diag) => diag.message.includes("suspicious ownership")), - ).toBe(true); - } finally { - proc.getuid = originalGetUid; - } + const actualUid = (process as NodeJS.Process & { getuid: () => number }).getuid(); + const result = await withStateDir(stateDir, async () => { + return discoverOpenClawPlugins({ ownershipUid: actualUid + 1 }); + }); + expect(result.candidates).toHaveLength(0); + expect(result.diagnostics.some((diag) => diag.message.includes("suspicious ownership"))).toBe( + true, + ); }, ); }); diff --git a/src/plugins/discovery.ts b/src/plugins/discovery.ts index 3625ab9a1..747508142 100644 --- a/src/plugins/discovery.ts +++ b/src/plugins/discovery.ts @@ -7,6 +7,7 @@ import { type OpenClawPackageManifest, type PackageManifest, } from "./manifest.js"; +import { formatPosixMode, isPathInside, safeRealpathSync, safeStatSync } from "./path-safety.js"; import type { PluginDiagnostic, PluginOrigin } from "./types.js"; const EXTENSION_EXTS = new Set([".ts", ".js", ".mts", ".cts", ".mjs", ".cjs"]); @@ -29,35 +30,10 @@ export type PluginDiscoveryResult = { diagnostics: PluginDiagnostic[]; }; -function isPathInside(baseDir: string, targetPath: string): boolean { - const rel = path.relative(baseDir, targetPath); - if (!rel) { - return true; +function currentUid(overrideUid?: number | null): number | null { + if (overrideUid !== undefined) { + return overrideUid; } - return !rel.startsWith("..") && !path.isAbsolute(rel); -} - -function safeRealpathSync(targetPath: string): string | null { - try { - return fs.realpathSync(targetPath); - } catch { - return null; - } -} - -function safeStatSync(targetPath: string): fs.Stats | null { - try { - return fs.statSync(targetPath); - } catch { - return null; - } -} - -function formatMode(mode: number): string { - return (mode & 0o777).toString(8).padStart(3, "0"); -} - -function currentUid(): number | null { if (process.platform === "win32") { return null; } @@ -67,28 +43,55 @@ function currentUid(): number | null { return process.getuid(); } -function isUnsafePluginCandidate(params: { +export type CandidateBlockReason = + | "source_escapes_root" + | "path_stat_failed" + | "path_world_writable" + | "path_suspicious_ownership"; + +type CandidateBlockIssue = { + reason: CandidateBlockReason; + sourcePath: string; + rootPath: string; + targetPath: string; + sourceRealPath?: string; + rootRealPath?: string; + modeBits?: number; + foundUid?: number; + expectedUid?: number; +}; + +function checkSourceEscapesRoot(params: { + source: string; + rootDir: string; +}): CandidateBlockIssue | null { + const sourceRealPath = safeRealpathSync(params.source); + const rootRealPath = safeRealpathSync(params.rootDir); + if (!sourceRealPath || !rootRealPath) { + return null; + } + if (isPathInside(rootRealPath, sourceRealPath)) { + return null; + } + return { + reason: "source_escapes_root", + sourcePath: params.source, + rootPath: params.rootDir, + targetPath: params.source, + sourceRealPath, + rootRealPath, + }; +} + +function checkPathStatAndPermissions(params: { source: string; rootDir: string; origin: PluginOrigin; - diagnostics: PluginDiagnostic[]; -}): boolean { - const sourceReal = safeRealpathSync(params.source); - const rootReal = safeRealpathSync(params.rootDir); - if (sourceReal && rootReal && !isPathInside(rootReal, sourceReal)) { - params.diagnostics.push({ - level: "warn", - source: params.source, - message: `blocked plugin candidate: source escapes plugin root (${params.source} -> ${sourceReal}; root=${rootReal})`, - }); - return true; - } - + uid: number | null; +}): CandidateBlockIssue | null { if (process.platform === "win32") { - return false; + return null; } - - const uid = currentUid(); const pathsToCheck = [params.rootDir, params.source]; const seen = new Set(); for (const targetPath of pathsToCheck) { @@ -99,39 +102,99 @@ function isUnsafePluginCandidate(params: { seen.add(normalized); const stat = safeStatSync(targetPath); if (!stat) { - params.diagnostics.push({ - level: "warn", - source: targetPath, - message: `blocked plugin candidate: cannot stat path (${targetPath})`, - }); - return true; + return { + reason: "path_stat_failed", + sourcePath: params.source, + rootPath: params.rootDir, + targetPath, + }; } const modeBits = stat.mode & 0o777; if ((modeBits & 0o002) !== 0) { - params.diagnostics.push({ - level: "warn", - source: targetPath, - message: `blocked plugin candidate: world-writable path (${targetPath}, mode=${formatMode(modeBits)})`, - }); - return true; + return { + reason: "path_world_writable", + sourcePath: params.source, + rootPath: params.rootDir, + targetPath, + modeBits, + }; } if ( params.origin !== "bundled" && - uid !== null && + params.uid !== null && typeof stat.uid === "number" && - stat.uid !== uid && + stat.uid !== params.uid && stat.uid !== 0 ) { - params.diagnostics.push({ - level: "warn", - source: targetPath, - message: `blocked plugin candidate: suspicious ownership (${targetPath}, uid=${stat.uid}, expected uid=${uid} or root)`, - }); - return true; + return { + reason: "path_suspicious_ownership", + sourcePath: params.source, + rootPath: params.rootDir, + targetPath, + foundUid: stat.uid, + expectedUid: params.uid, + }; } } + return null; +} - return false; +function findCandidateBlockIssue(params: { + source: string; + rootDir: string; + origin: PluginOrigin; + ownershipUid?: number | null; +}): CandidateBlockIssue | null { + const escaped = checkSourceEscapesRoot({ + source: params.source, + rootDir: params.rootDir, + }); + if (escaped) { + return escaped; + } + return checkPathStatAndPermissions({ + source: params.source, + rootDir: params.rootDir, + origin: params.origin, + uid: currentUid(params.ownershipUid), + }); +} + +function formatCandidateBlockMessage(issue: CandidateBlockIssue): string { + if (issue.reason === "source_escapes_root") { + return `blocked plugin candidate: source escapes plugin root (${issue.sourcePath} -> ${issue.sourceRealPath}; root=${issue.rootRealPath})`; + } + if (issue.reason === "path_stat_failed") { + return `blocked plugin candidate: cannot stat path (${issue.targetPath})`; + } + if (issue.reason === "path_world_writable") { + return `blocked plugin candidate: world-writable path (${issue.targetPath}, mode=${formatPosixMode(issue.modeBits ?? 0)})`; + } + return `blocked plugin candidate: suspicious ownership (${issue.targetPath}, uid=${issue.foundUid}, expected uid=${issue.expectedUid} or root)`; +} + +function isUnsafePluginCandidate(params: { + source: string; + rootDir: string; + origin: PluginOrigin; + diagnostics: PluginDiagnostic[]; + ownershipUid?: number | null; +}): boolean { + const issue = findCandidateBlockIssue({ + source: params.source, + rootDir: params.rootDir, + origin: params.origin, + ownershipUid: params.ownershipUid, + }); + if (!issue) { + return false; + } + params.diagnostics.push({ + level: "warn", + source: issue.targetPath, + message: formatCandidateBlockMessage(issue), + }); + return true; } function isExtensionFile(filePath: string): boolean { @@ -194,6 +257,7 @@ function addCandidate(params: { source: string; rootDir: string; origin: PluginOrigin; + ownershipUid?: number | null; workspaceDir?: string; manifest?: PackageManifest | null; packageDir?: string; @@ -209,6 +273,7 @@ function addCandidate(params: { rootDir: resolvedRoot, origin: params.origin, diagnostics: params.diagnostics, + ownershipUid: params.ownershipUid, }) ) { return; @@ -232,6 +297,7 @@ function addCandidate(params: { function discoverInDirectory(params: { dir: string; origin: PluginOrigin; + ownershipUid?: number | null; workspaceDir?: string; candidates: PluginCandidate[]; diagnostics: PluginDiagnostic[]; @@ -266,6 +332,7 @@ function discoverInDirectory(params: { source: fullPath, rootDir: path.dirname(fullPath), origin: params.origin, + ownershipUid: params.ownershipUid, workspaceDir: params.workspaceDir, }); } @@ -291,6 +358,7 @@ function discoverInDirectory(params: { source: resolved, rootDir: fullPath, origin: params.origin, + ownershipUid: params.ownershipUid, workspaceDir: params.workspaceDir, manifest, packageDir: fullPath, @@ -312,6 +380,7 @@ function discoverInDirectory(params: { source: indexFile, rootDir: fullPath, origin: params.origin, + ownershipUid: params.ownershipUid, workspaceDir: params.workspaceDir, manifest, packageDir: fullPath, @@ -323,6 +392,7 @@ function discoverInDirectory(params: { function discoverFromPath(params: { rawPath: string; origin: PluginOrigin; + ownershipUid?: number | null; workspaceDir?: string; candidates: PluginCandidate[]; diagnostics: PluginDiagnostic[]; @@ -356,6 +426,7 @@ function discoverFromPath(params: { source: resolved, rootDir: path.dirname(resolved), origin: params.origin, + ownershipUid: params.ownershipUid, workspaceDir: params.workspaceDir, }); return; @@ -380,6 +451,7 @@ function discoverFromPath(params: { source, rootDir: resolved, origin: params.origin, + ownershipUid: params.ownershipUid, workspaceDir: params.workspaceDir, manifest, packageDir: resolved, @@ -402,6 +474,7 @@ function discoverFromPath(params: { source: indexFile, rootDir: resolved, origin: params.origin, + ownershipUid: params.ownershipUid, workspaceDir: params.workspaceDir, manifest, packageDir: resolved, @@ -412,6 +485,7 @@ function discoverFromPath(params: { discoverInDirectory({ dir: resolved, origin: params.origin, + ownershipUid: params.ownershipUid, workspaceDir: params.workspaceDir, candidates: params.candidates, diagnostics: params.diagnostics, @@ -424,6 +498,7 @@ function discoverFromPath(params: { export function discoverOpenClawPlugins(params: { workspaceDir?: string; extraPaths?: string[]; + ownershipUid?: number | null; }): PluginDiscoveryResult { const candidates: PluginCandidate[] = []; const diagnostics: PluginDiagnostic[] = []; @@ -442,6 +517,7 @@ export function discoverOpenClawPlugins(params: { discoverFromPath({ rawPath: trimmed, origin: "config", + ownershipUid: params.ownershipUid, workspaceDir: workspaceDir?.trim() || undefined, candidates, diagnostics, @@ -455,6 +531,7 @@ export function discoverOpenClawPlugins(params: { discoverInDirectory({ dir, origin: "workspace", + ownershipUid: params.ownershipUid, workspaceDir: workspaceRoot, candidates, diagnostics, @@ -467,6 +544,7 @@ export function discoverOpenClawPlugins(params: { discoverInDirectory({ dir: globalDir, origin: "global", + ownershipUid: params.ownershipUid, candidates, diagnostics, seen, @@ -477,6 +555,7 @@ export function discoverOpenClawPlugins(params: { discoverInDirectory({ dir: bundledDir, origin: "bundled", + ownershipUid: params.ownershipUid, candidates, diagnostics, seen, diff --git a/src/plugins/loader.ts b/src/plugins/loader.ts index ab688865f..c88483e86 100644 --- a/src/plugins/loader.ts +++ b/src/plugins/loader.ts @@ -17,6 +17,7 @@ import { import { discoverOpenClawPlugins } from "./discovery.js"; import { initializeGlobalHookRunner } from "./hook-runner-global.js"; import { loadPluginManifestRegistry } from "./manifest-registry.js"; +import { isPathInside, safeStatSync } from "./path-safety.js"; import { createPluginRegistry, type PluginRecord, type PluginRegistry } from "./registry.js"; import { setActivePluginRegistry } from "./runtime.js"; import { createPluginRuntime } from "./runtime/index.js"; @@ -177,61 +178,100 @@ function pushDiagnostics(diagnostics: PluginDiagnostic[], append: PluginDiagnost diagnostics.push(...append); } -function isPathInside(baseDir: string, targetPath: string): boolean { - const rel = path.relative(baseDir, targetPath); - if (!rel) { - return true; - } - return !rel.startsWith("..") && !path.isAbsolute(rel); +type PathMatcher = { + exact: Set; + dirs: string[]; +}; + +type InstallTrackingRule = { + trackedWithoutPaths: boolean; + matcher: PathMatcher; +}; + +type PluginProvenanceIndex = { + loadPathMatcher: PathMatcher; + installRules: Map; +}; + +function createPathMatcher(): PathMatcher { + return { exact: new Set(), dirs: [] }; } -function pathMatchesBaseOrFile(params: { baseOrFile: string; targetFile: string }): boolean { - const baseResolved = resolveUserPath(params.baseOrFile); - const targetResolved = resolveUserPath(params.targetFile); - if (baseResolved === targetResolved) { +function addPathToMatcher(matcher: PathMatcher, rawPath: string): void { + const trimmed = rawPath.trim(); + if (!trimmed) { + return; + } + const resolved = resolveUserPath(trimmed); + if (!resolved) { + return; + } + if (matcher.exact.has(resolved) || matcher.dirs.includes(resolved)) { + return; + } + const stat = safeStatSync(resolved); + if (stat?.isDirectory()) { + matcher.dirs.push(resolved); + return; + } + matcher.exact.add(resolved); +} + +function matchesPathMatcher(matcher: PathMatcher, sourcePath: string): boolean { + if (matcher.exact.has(sourcePath)) { return true; } - try { - const stat = fs.statSync(baseResolved); - if (!stat.isDirectory()) { - return false; + return matcher.dirs.some((dirPath) => isPathInside(dirPath, sourcePath)); +} + +function buildProvenanceIndex(params: { + config: OpenClawConfig; + normalizedLoadPaths: string[]; +}): PluginProvenanceIndex { + const loadPathMatcher = createPathMatcher(); + for (const loadPath of params.normalizedLoadPaths) { + addPathToMatcher(loadPathMatcher, loadPath); + } + + const installRules = new Map(); + const installs = params.config.plugins?.installs ?? {}; + for (const [pluginId, install] of Object.entries(installs)) { + const rule: InstallTrackingRule = { + trackedWithoutPaths: false, + matcher: createPathMatcher(), + }; + const trackedPaths = [install.installPath, install.sourcePath] + .map((entry) => (typeof entry === "string" ? entry.trim() : "")) + .filter(Boolean); + if (trackedPaths.length === 0) { + rule.trackedWithoutPaths = true; + } else { + for (const trackedPath of trackedPaths) { + addPathToMatcher(rule.matcher, trackedPath); + } } - } catch { - return false; + installRules.set(pluginId, rule); } - return isPathInside(baseResolved, targetResolved); + + return { loadPathMatcher, installRules }; } -function isTrackedByInstallRecord(params: { +function isTrackedByProvenance(params: { pluginId: string; source: string; - config: OpenClawConfig; + index: PluginProvenanceIndex; }): boolean { - const install = params.config.plugins?.installs?.[params.pluginId]; - if (!install) { - return false; + const sourcePath = resolveUserPath(params.source); + const installRule = params.index.installRules.get(params.pluginId); + if (installRule) { + if (installRule.trackedWithoutPaths) { + return true; + } + if (matchesPathMatcher(installRule.matcher, sourcePath)) { + return true; + } } - const trackedPaths = [install.installPath, install.sourcePath] - .map((entry) => (typeof entry === "string" ? entry.trim() : "")) - .filter(Boolean); - if (trackedPaths.length === 0) { - return true; - } - return trackedPaths.some((trackedPath) => - pathMatchesBaseOrFile({ - baseOrFile: trackedPath, - targetFile: params.source, - }), - ); -} - -function isTrackedByLoadPath(params: { source: string; loadPaths: string[] }): boolean { - return params.loadPaths.some((loadPath) => - pathMatchesBaseOrFile({ - baseOrFile: loadPath, - targetFile: params.source, - }), - ); + return matchesPathMatcher(params.index.loadPathMatcher, sourcePath); } function warnWhenAllowlistIsOpen(params: { @@ -262,8 +302,7 @@ function warnWhenAllowlistIsOpen(params: { function warnAboutUntrackedLoadedPlugins(params: { registry: PluginRegistry; - config: OpenClawConfig; - normalizedLoadPaths: string[]; + provenance: PluginProvenanceIndex; logger: PluginLogger; }) { for (const plugin of params.registry.plugins) { @@ -271,18 +310,10 @@ function warnAboutUntrackedLoadedPlugins(params: { continue; } if ( - isTrackedByInstallRecord({ + isTrackedByProvenance({ pluginId: plugin.id, source: plugin.source, - config: params.config, - }) - ) { - continue; - } - if ( - isTrackedByLoadPath({ - source: plugin.source, - loadPaths: params.normalizedLoadPaths, + index: params.provenance, }) ) { continue; @@ -351,6 +382,10 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi origin: plugin.origin, })), }); + const provenance = buildProvenanceIndex({ + config: cfg, + normalizedLoadPaths: normalized.loadPaths, + }); // Lazy: avoid creating the Jiti loader when all plugins are disabled (common in unit tests). let jitiLoader: ReturnType | null = null; @@ -605,8 +640,7 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi warnAboutUntrackedLoadedPlugins({ registry, - config: cfg, - normalizedLoadPaths: normalized.loadPaths, + provenance, logger, }); diff --git a/src/plugins/manifest-registry.ts b/src/plugins/manifest-registry.ts index 8929a664f..80313e99f 100644 --- a/src/plugins/manifest-registry.ts +++ b/src/plugins/manifest-registry.ts @@ -4,6 +4,7 @@ import { resolveUserPath } from "../utils.js"; import { normalizePluginsConfig, type NormalizedPluginsConfig } from "./config-state.js"; import { discoverOpenClawPlugins, type PluginCandidate } from "./discovery.js"; import { loadPluginManifest, type PluginManifest } from "./manifest.js"; +import { safeRealpathSync } from "./path-safety.js"; import type { PluginConfigUiHint, PluginDiagnostic, PluginKind, PluginOrigin } from "./types.js"; type SeenIdEntry = { @@ -19,20 +20,6 @@ const PLUGIN_ORIGIN_RANK: Readonly> = { bundled: 3, }; -function safeRealpathSync(rootDir: string, cache: Map): string | null { - const cached = cache.get(rootDir); - if (cached) { - return cached; - } - try { - const resolved = fs.realpathSync(rootDir); - cache.set(rootDir, resolved); - return resolved; - } catch { - return null; - } -} - export type PluginManifestRecord = { id: string; name?: string; diff --git a/src/plugins/path-safety.ts b/src/plugins/path-safety.ts new file mode 100644 index 000000000..48c2da8e6 --- /dev/null +++ b/src/plugins/path-safety.ts @@ -0,0 +1,36 @@ +import fs from "node:fs"; +import path from "node:path"; + +export function isPathInside(baseDir: string, targetPath: string): boolean { + const rel = path.relative(baseDir, targetPath); + if (!rel) { + return true; + } + return !rel.startsWith("..") && !path.isAbsolute(rel); +} + +export function safeRealpathSync(targetPath: string, cache?: Map): string | null { + const cached = cache?.get(targetPath); + if (cached) { + return cached; + } + try { + const resolved = fs.realpathSync(targetPath); + cache?.set(targetPath, resolved); + return resolved; + } catch { + return null; + } +} + +export function safeStatSync(targetPath: string): fs.Stats | null { + try { + return fs.statSync(targetPath); + } catch { + return null; + } +} + +export function formatPosixMode(mode: number): string { + return (mode & 0o777).toString(8).padStart(3, "0"); +}