mirror of
https://github.com/moltbot/moltbot.git
synced 2026-03-07 22:44:16 +00:00
fix(plugins): harden discovery trust checks
This commit is contained in:
Peter Steinberger
@@ -19,6 +19,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.
|
||||
- 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.
|
||||
|
||||
@@ -116,6 +116,15 @@ Bundled plugins must be enabled explicitly via `plugins.entries.<id>.enabled`
|
||||
or `openclaw plugins enable <id>`. Installed plugins are enabled by default,
|
||||
but can be disabled the same way.
|
||||
|
||||
Hardening notes:
|
||||
|
||||
- If `plugins.allow` is empty and non-bundled plugins are discoverable, OpenClaw logs a startup warning with plugin ids and sources.
|
||||
- Candidate paths are safety-checked before discovery admission. OpenClaw blocks candidates when:
|
||||
- extension entry resolves outside plugin root (including symlink/path traversal escapes),
|
||||
- plugin root/source path is world-writable,
|
||||
- path ownership is suspicious for non-bundled plugins (POSIX owner is neither current uid nor root).
|
||||
- Loaded non-bundled plugins without install/load-path provenance emit a warning so you can pin trust (`plugins.allow`) or install tracking (`plugins.installs`).
|
||||
|
||||
Each plugin must include a `openclaw.plugin.json` file in its root. If a path
|
||||
points at a file, the plugin root is the file's directory and must contain the
|
||||
manifest.
|
||||
|
||||
@@ -149,4 +149,78 @@ describe("discoverOpenClawPlugins", () => {
|
||||
const ids = candidates.map((c) => c.idHint);
|
||||
expect(ids).toContain("demo-plugin-dir");
|
||||
});
|
||||
|
||||
it("blocks extension entries that escape plugin root", async () => {
|
||||
const stateDir = makeTempDir();
|
||||
const globalExt = path.join(stateDir, "extensions", "escape-pack");
|
||||
const outside = path.join(stateDir, "outside.js");
|
||||
fs.mkdirSync(globalExt, { recursive: true });
|
||||
|
||||
fs.writeFileSync(
|
||||
path.join(globalExt, "package.json"),
|
||||
JSON.stringify({
|
||||
name: "@openclaw/escape-pack",
|
||||
openclaw: { extensions: ["../../outside.js"] },
|
||||
}),
|
||||
"utf-8",
|
||||
);
|
||||
fs.writeFileSync(outside, "export default function () {}", "utf-8");
|
||||
|
||||
const result = await withStateDir(stateDir, async () => {
|
||||
return discoverOpenClawPlugins({});
|
||||
});
|
||||
|
||||
expect(result.candidates).toHaveLength(0);
|
||||
expect(
|
||||
result.diagnostics.some((diag) => diag.message.includes("source escapes plugin root")),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it.runIf(process.platform !== "win32")("blocks world-writable plugin paths", async () => {
|
||||
const stateDir = makeTempDir();
|
||||
const globalExt = path.join(stateDir, "extensions");
|
||||
fs.mkdirSync(globalExt, { recursive: true });
|
||||
const pluginPath = path.join(globalExt, "world-open.ts");
|
||||
fs.writeFileSync(pluginPath, "export default function () {}", "utf-8");
|
||||
fs.chmodSync(pluginPath, 0o777);
|
||||
|
||||
const result = await withStateDir(stateDir, async () => {
|
||||
return discoverOpenClawPlugins({});
|
||||
});
|
||||
|
||||
expect(result.candidates).toHaveLength(0);
|
||||
expect(result.diagnostics.some((diag) => diag.message.includes("world-writable path"))).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
it.runIf(process.platform !== "win32" && typeof process.getuid === "function")(
|
||||
"blocks suspicious ownership when uid mismatch is detected",
|
||||
async () => {
|
||||
const stateDir = makeTempDir();
|
||||
const globalExt = path.join(stateDir, "extensions");
|
||||
fs.mkdirSync(globalExt, { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(globalExt, "owner-mismatch.ts"),
|
||||
"export default function () {}",
|
||||
"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;
|
||||
}
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
@@ -29,6 +29,111 @@ export type PluginDiscoveryResult = {
|
||||
diagnostics: PluginDiagnostic[];
|
||||
};
|
||||
|
||||
function isPathInside(baseDir: string, targetPath: string): boolean {
|
||||
const rel = path.relative(baseDir, targetPath);
|
||||
if (!rel) {
|
||||
return true;
|
||||
}
|
||||
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;
|
||||
}
|
||||
if (typeof process.getuid !== "function") {
|
||||
return null;
|
||||
}
|
||||
return process.getuid();
|
||||
}
|
||||
|
||||
function isUnsafePluginCandidate(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;
|
||||
}
|
||||
|
||||
if (process.platform === "win32") {
|
||||
return false;
|
||||
}
|
||||
|
||||
const uid = currentUid();
|
||||
const pathsToCheck = [params.rootDir, params.source];
|
||||
const seen = new Set<string>();
|
||||
for (const targetPath of pathsToCheck) {
|
||||
const normalized = path.resolve(targetPath);
|
||||
if (seen.has(normalized)) {
|
||||
continue;
|
||||
}
|
||||
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;
|
||||
}
|
||||
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;
|
||||
}
|
||||
if (
|
||||
params.origin !== "bundled" &&
|
||||
uid !== null &&
|
||||
typeof stat.uid === "number" &&
|
||||
stat.uid !== 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 false;
|
||||
}
|
||||
|
||||
function isExtensionFile(filePath: string): boolean {
|
||||
const ext = path.extname(filePath);
|
||||
if (!EXTENSION_EXTS.has(ext)) {
|
||||
@@ -83,6 +188,7 @@ function deriveIdHint(params: {
|
||||
|
||||
function addCandidate(params: {
|
||||
candidates: PluginCandidate[];
|
||||
diagnostics: PluginDiagnostic[];
|
||||
seen: Set<string>;
|
||||
idHint: string;
|
||||
source: string;
|
||||
@@ -96,12 +202,23 @@ function addCandidate(params: {
|
||||
if (params.seen.has(resolved)) {
|
||||
return;
|
||||
}
|
||||
const resolvedRoot = path.resolve(params.rootDir);
|
||||
if (
|
||||
isUnsafePluginCandidate({
|
||||
source: resolved,
|
||||
rootDir: resolvedRoot,
|
||||
origin: params.origin,
|
||||
diagnostics: params.diagnostics,
|
||||
})
|
||||
) {
|
||||
return;
|
||||
}
|
||||
params.seen.add(resolved);
|
||||
const manifest = params.manifest ?? null;
|
||||
params.candidates.push({
|
||||
idHint: params.idHint,
|
||||
source: resolved,
|
||||
rootDir: path.resolve(params.rootDir),
|
||||
rootDir: resolvedRoot,
|
||||
origin: params.origin,
|
||||
workspaceDir: params.workspaceDir,
|
||||
packageName: manifest?.name?.trim() || undefined,
|
||||
@@ -143,6 +260,7 @@ function discoverInDirectory(params: {
|
||||
}
|
||||
addCandidate({
|
||||
candidates: params.candidates,
|
||||
diagnostics: params.diagnostics,
|
||||
seen: params.seen,
|
||||
idHint: path.basename(entry.name, path.extname(entry.name)),
|
||||
source: fullPath,
|
||||
@@ -163,6 +281,7 @@ function discoverInDirectory(params: {
|
||||
const resolved = path.resolve(fullPath, extPath);
|
||||
addCandidate({
|
||||
candidates: params.candidates,
|
||||
diagnostics: params.diagnostics,
|
||||
seen: params.seen,
|
||||
idHint: deriveIdHint({
|
||||
filePath: resolved,
|
||||
@@ -187,6 +306,7 @@ function discoverInDirectory(params: {
|
||||
if (indexFile && isExtensionFile(indexFile)) {
|
||||
addCandidate({
|
||||
candidates: params.candidates,
|
||||
diagnostics: params.diagnostics,
|
||||
seen: params.seen,
|
||||
idHint: entry.name,
|
||||
source: indexFile,
|
||||
@@ -230,6 +350,7 @@ function discoverFromPath(params: {
|
||||
}
|
||||
addCandidate({
|
||||
candidates: params.candidates,
|
||||
diagnostics: params.diagnostics,
|
||||
seen: params.seen,
|
||||
idHint: path.basename(resolved, path.extname(resolved)),
|
||||
source: resolved,
|
||||
@@ -249,6 +370,7 @@ function discoverFromPath(params: {
|
||||
const source = path.resolve(resolved, extPath);
|
||||
addCandidate({
|
||||
candidates: params.candidates,
|
||||
diagnostics: params.diagnostics,
|
||||
seen: params.seen,
|
||||
idHint: deriveIdHint({
|
||||
filePath: source,
|
||||
@@ -274,6 +396,7 @@ function discoverFromPath(params: {
|
||||
if (indexFile && isExtensionFile(indexFile)) {
|
||||
addCandidate({
|
||||
candidates: params.candidates,
|
||||
diagnostics: params.diagnostics,
|
||||
seen: params.seen,
|
||||
idHint: path.basename(resolved),
|
||||
source: indexFile,
|
||||
|
||||
@@ -489,4 +489,76 @@ describe("loadOpenClawPlugins", () => {
|
||||
expect(loaded?.origin).toBe("config");
|
||||
expect(overridden?.origin).toBe("bundled");
|
||||
});
|
||||
|
||||
it("warns when plugins.allow is empty and non-bundled plugins are discoverable", () => {
|
||||
process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins";
|
||||
const plugin = writePlugin({
|
||||
id: "warn-open-allow",
|
||||
body: `export default { id: "warn-open-allow", register() {} };`,
|
||||
});
|
||||
const warnings: string[] = [];
|
||||
loadOpenClawPlugins({
|
||||
cache: false,
|
||||
logger: {
|
||||
info: () => {},
|
||||
warn: (msg) => warnings.push(msg),
|
||||
error: () => {},
|
||||
},
|
||||
config: {
|
||||
plugins: {
|
||||
load: { paths: [plugin.file] },
|
||||
},
|
||||
},
|
||||
});
|
||||
expect(
|
||||
warnings.some((msg) => msg.includes("plugins.allow is empty") && msg.includes(plugin.id)),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("warns when loaded non-bundled plugin has no install/load-path provenance", () => {
|
||||
process.env.OPENCLAW_BUNDLED_PLUGINS_DIR = "/nonexistent/bundled/plugins";
|
||||
const prevStateDir = process.env.OPENCLAW_STATE_DIR;
|
||||
const stateDir = makeTempDir();
|
||||
process.env.OPENCLAW_STATE_DIR = stateDir;
|
||||
try {
|
||||
const globalDir = path.join(stateDir, "extensions", "rogue");
|
||||
fs.mkdirSync(globalDir, { recursive: true });
|
||||
writePlugin({
|
||||
id: "rogue",
|
||||
body: `export default { id: "rogue", register() {} };`,
|
||||
dir: globalDir,
|
||||
filename: "index.js",
|
||||
});
|
||||
|
||||
const warnings: string[] = [];
|
||||
const registry = loadOpenClawPlugins({
|
||||
cache: false,
|
||||
logger: {
|
||||
info: () => {},
|
||||
warn: (msg) => warnings.push(msg),
|
||||
error: () => {},
|
||||
},
|
||||
config: {
|
||||
plugins: {
|
||||
allow: ["rogue"],
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const rogue = registry.plugins.find((entry) => entry.id === "rogue");
|
||||
expect(rogue?.status).toBe("loaded");
|
||||
expect(
|
||||
warnings.some(
|
||||
(msg) =>
|
||||
msg.includes("rogue") && msg.includes("loaded without install/load-path provenance"),
|
||||
),
|
||||
).toBe(true);
|
||||
} finally {
|
||||
if (prevStateDir === undefined) {
|
||||
delete process.env.OPENCLAW_STATE_DIR;
|
||||
} else {
|
||||
process.env.OPENCLAW_STATE_DIR = prevStateDir;
|
||||
}
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -177,6 +177,128 @@ 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);
|
||||
}
|
||||
|
||||
function pathMatchesBaseOrFile(params: { baseOrFile: string; targetFile: string }): boolean {
|
||||
const baseResolved = resolveUserPath(params.baseOrFile);
|
||||
const targetResolved = resolveUserPath(params.targetFile);
|
||||
if (baseResolved === targetResolved) {
|
||||
return true;
|
||||
}
|
||||
try {
|
||||
const stat = fs.statSync(baseResolved);
|
||||
if (!stat.isDirectory()) {
|
||||
return false;
|
||||
}
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
return isPathInside(baseResolved, targetResolved);
|
||||
}
|
||||
|
||||
function isTrackedByInstallRecord(params: {
|
||||
pluginId: string;
|
||||
source: string;
|
||||
config: OpenClawConfig;
|
||||
}): boolean {
|
||||
const install = params.config.plugins?.installs?.[params.pluginId];
|
||||
if (!install) {
|
||||
return false;
|
||||
}
|
||||
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,
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
function warnWhenAllowlistIsOpen(params: {
|
||||
logger: PluginLogger;
|
||||
pluginsEnabled: boolean;
|
||||
allow: string[];
|
||||
discoverablePlugins: Array<{ id: string; source: string; origin: PluginRecord["origin"] }>;
|
||||
}) {
|
||||
if (!params.pluginsEnabled) {
|
||||
return;
|
||||
}
|
||||
if (params.allow.length > 0) {
|
||||
return;
|
||||
}
|
||||
const nonBundled = params.discoverablePlugins.filter((entry) => entry.origin !== "bundled");
|
||||
if (nonBundled.length === 0) {
|
||||
return;
|
||||
}
|
||||
const preview = nonBundled
|
||||
.slice(0, 6)
|
||||
.map((entry) => `${entry.id} (${entry.source})`)
|
||||
.join(", ");
|
||||
const extra = nonBundled.length > 6 ? ` (+${nonBundled.length - 6} more)` : "";
|
||||
params.logger.warn(
|
||||
`[plugins] plugins.allow is empty; discovered non-bundled plugins may auto-load: ${preview}${extra}. Set plugins.allow to explicit trusted ids.`,
|
||||
);
|
||||
}
|
||||
|
||||
function warnAboutUntrackedLoadedPlugins(params: {
|
||||
registry: PluginRegistry;
|
||||
config: OpenClawConfig;
|
||||
normalizedLoadPaths: string[];
|
||||
logger: PluginLogger;
|
||||
}) {
|
||||
for (const plugin of params.registry.plugins) {
|
||||
if (plugin.status !== "loaded" || plugin.origin === "bundled") {
|
||||
continue;
|
||||
}
|
||||
if (
|
||||
isTrackedByInstallRecord({
|
||||
pluginId: plugin.id,
|
||||
source: plugin.source,
|
||||
config: params.config,
|
||||
})
|
||||
) {
|
||||
continue;
|
||||
}
|
||||
if (
|
||||
isTrackedByLoadPath({
|
||||
source: plugin.source,
|
||||
loadPaths: params.normalizedLoadPaths,
|
||||
})
|
||||
) {
|
||||
continue;
|
||||
}
|
||||
const message =
|
||||
"loaded without install/load-path provenance; treat as untracked local code and pin trust via plugins.allow or install records";
|
||||
params.registry.diagnostics.push({
|
||||
level: "warn",
|
||||
pluginId: plugin.id,
|
||||
source: plugin.source,
|
||||
message,
|
||||
});
|
||||
params.logger.warn(`[plugins] ${plugin.id}: ${message} (${plugin.source})`);
|
||||
}
|
||||
}
|
||||
|
||||
export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegistry {
|
||||
// Test env: default-disable plugins unless explicitly configured.
|
||||
// This keeps unit/gateway suites fast and avoids loading heavyweight plugin deps by accident.
|
||||
@@ -219,6 +341,16 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi
|
||||
diagnostics: discovery.diagnostics,
|
||||
});
|
||||
pushDiagnostics(registry.diagnostics, manifestRegistry.diagnostics);
|
||||
warnWhenAllowlistIsOpen({
|
||||
logger,
|
||||
pluginsEnabled: normalized.enabled,
|
||||
allow: normalized.allow,
|
||||
discoverablePlugins: manifestRegistry.plugins.map((plugin) => ({
|
||||
id: plugin.id,
|
||||
source: plugin.source,
|
||||
origin: plugin.origin,
|
||||
})),
|
||||
});
|
||||
|
||||
// Lazy: avoid creating the Jiti loader when all plugins are disabled (common in unit tests).
|
||||
let jitiLoader: ReturnType<typeof createJiti> | null = null;
|
||||
@@ -471,6 +603,13 @@ export function loadOpenClawPlugins(options: PluginLoadOptions = {}): PluginRegi
|
||||
});
|
||||
}
|
||||
|
||||
warnAboutUntrackedLoadedPlugins({
|
||||
registry,
|
||||
config: cfg,
|
||||
normalizedLoadPaths: normalized.loadPaths,
|
||||
logger,
|
||||
});
|
||||
|
||||
if (cacheEnabled) {
|
||||
registryCache.set(cacheKey, registry);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user