mirror of https://github.com/openclaw/openclaw.git
fix(access-policy): warn on file-specific bwrap deny entries, expand ~ in scripts keys
This commit is contained in:
parent
a9a1e7d912
commit
1c03f79f1f
|
|
@ -130,6 +130,8 @@ For cross-agent MCP tool delegation (an orchestrator invoking a tool on behalf o
|
|||
|
||||
**Interpreter bypass of exec bit.** The `x` bit gates `execve()` on the file itself. Running `bash script.sh` executes bash (permitted), which reads the script as text (read permitted if `r` is set). The exec bit on the script is irrelevant for interpreter-based invocations. To prevent execution of a script entirely, place it in the deny list (no read access).
|
||||
|
||||
**File-specific `deny[]` entries on Linux (bwrap).** On Linux, `deny[]` entries are enforced at the OS layer using `bwrap --tmpfs` overlays, which only work on directories. When a `deny[]` entry resolves to an existing file (e.g. `deny: ["~/.netrc"]`), the OS-level mount is skipped — bwrap cannot overlay a file with a tmpfs. Tool-layer enforcement still blocks read/write/edit calls for that file. However, exec commands running inside the sandbox can still access the file directly (e.g. `cat ~/.netrc`). A warning is emitted to stderr when this gap is active. To enforce at the OS layer on Linux, deny the parent directory instead (e.g. `deny: ["~/.aws/"]` rather than `deny: ["~/.aws/credentials"]`). On macOS, seatbelt handles file-level denials correctly with `(deny file-read* (literal ...))`.
|
||||
|
||||
**No approval flow.** Access policy is fail-closed: a denied operation returns an error immediately. There is no `ask`/`on-miss` mode equivalent to exec approvals. If an agent hits a denied path, it receives a permission error and must handle it. Interactive approval for filesystem access is planned as a follow-up feature.
|
||||
|
||||
## Related
|
||||
|
|
|
|||
|
|
@ -865,6 +865,19 @@ describe("applyScriptPolicyOverride", () => {
|
|||
}
|
||||
});
|
||||
|
||||
it("matches scripts key written with ~ even though resolvedArgv0 is absolute", () => {
|
||||
// Regression: "~/bin/deploy.sh" in scripts{} must match resolvedArgv0 "/home/user/bin/deploy.sh".
|
||||
// A direct object lookup misses tilde keys; ~ must be expanded before comparing.
|
||||
const absPath = path.join(os.homedir(), "bin", "deploy.sh");
|
||||
const base: AccessPolicyConfig = {
|
||||
default: "rwx",
|
||||
scripts: { "~/bin/deploy.sh": { rules: { "/secret/**": "---" } } },
|
||||
};
|
||||
const { overrideRules, hashMismatch } = applyScriptPolicyOverride(base, absPath);
|
||||
expect(hashMismatch).toBeUndefined();
|
||||
expect(overrideRules?.["/secret/**"]).toBe("---");
|
||||
});
|
||||
|
||||
it("applies override when sha256 matches — rules in overrideRules, not policy", () => {
|
||||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "ap-test-"));
|
||||
const scriptPath = path.join(tmpDir, "script.sh");
|
||||
|
|
|
|||
|
|
@ -456,7 +456,19 @@ export function applyScriptPolicyOverride(
|
|||
policy: AccessPolicyConfig,
|
||||
resolvedArgv0: string,
|
||||
): { policy: AccessPolicyConfig; overrideRules?: Record<string, PermStr>; hashMismatch?: true } {
|
||||
const override = policy.scripts?.[resolvedArgv0];
|
||||
// Normalise ~ in scripts keys so "~/bin/deploy.sh" matches the resolved absolute
|
||||
// path "/home/user/bin/deploy.sh" that resolveArgv0 returns. A direct lookup
|
||||
// would always miss tilde-keyed entries, silently skipping sha256 verification.
|
||||
const scripts = policy.scripts;
|
||||
const override = scripts
|
||||
? (scripts[resolvedArgv0] ??
|
||||
Object.entries(scripts).find(([k]) => {
|
||||
if (!k.startsWith("~")) {
|
||||
return false;
|
||||
}
|
||||
return k.replace(/^~(?=$|[/\\])/, os.homedir()) === resolvedArgv0;
|
||||
})?.[1])
|
||||
: undefined;
|
||||
if (!override) {
|
||||
return { policy };
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,7 +1,11 @@
|
|||
import os from "node:os";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import type { AccessPolicyConfig } from "../config/types.tools.js";
|
||||
import { generateBwrapArgs, wrapCommandWithBwrap } from "./exec-sandbox-bwrap.js";
|
||||
import {
|
||||
_warnBwrapFileDenyOnce,
|
||||
generateBwrapArgs,
|
||||
wrapCommandWithBwrap,
|
||||
} from "./exec-sandbox-bwrap.js";
|
||||
|
||||
const HOME = os.homedir();
|
||||
|
||||
|
|
@ -302,6 +306,19 @@ describe("generateBwrapArgs", () => {
|
|||
expect(tmpfsMounts).not.toContain("/etc/hosts");
|
||||
});
|
||||
|
||||
it("emits a console.error warning when a file-specific deny[] entry is skipped", () => {
|
||||
// Use /etc/passwd (always a file) rather than /etc/hosts which is already in
|
||||
// _bwrapFileDenyWarnedPaths from the generateBwrapArgs test above.
|
||||
const errSpy = vi.spyOn(console, "error").mockImplementation(() => {});
|
||||
try {
|
||||
_warnBwrapFileDenyOnce("/etc/passwd");
|
||||
expect(errSpy).toHaveBeenCalledWith(expect.stringContaining("/etc/passwd"));
|
||||
expect(errSpy).toHaveBeenCalledWith(expect.stringContaining("parent directory"));
|
||||
} finally {
|
||||
errSpy.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it("still emits --tmpfs for deny[] entry that resolves to a directory", () => {
|
||||
// Non-existent paths are treated as directories (forward-protection).
|
||||
const config: AccessPolicyConfig = { default: "r--", deny: [`${HOME}/.nonexistent-dir/**`] };
|
||||
|
|
|
|||
|
|
@ -26,6 +26,25 @@ const SYSTEM_RO_BIND_PATHS = ["/usr", "/bin", "/lib", "/lib64", "/sbin", "/etc",
|
|||
|
||||
let bwrapAvailableCache: boolean | undefined;
|
||||
|
||||
// Warn once per process when a file-specific deny[] entry cannot be enforced at
|
||||
// the OS layer (bwrap --tmpfs only accepts directories). Tool-layer enforcement
|
||||
// still applies for read/write/edit tool calls, but exec commands that access
|
||||
// the file directly inside the sandbox are not blocked at the syscall level.
|
||||
// See docs/tools/access-policy.md — "File-specific deny[] entries on Linux".
|
||||
const _bwrapFileDenyWarnedPaths = new Set<string>();
|
||||
export function _warnBwrapFileDenyOnce(filePath: string): void {
|
||||
if (_bwrapFileDenyWarnedPaths.has(filePath)) {
|
||||
return;
|
||||
}
|
||||
_bwrapFileDenyWarnedPaths.add(filePath);
|
||||
console.error(
|
||||
`[access-policy] bwrap: deny[] entry "${filePath}" resolves to a file — ` +
|
||||
`OS-level (bwrap) enforcement is not applied. ` +
|
||||
`Tool-layer enforcement still blocks read/write/edit tool calls. ` +
|
||||
`To protect this file at the OS layer on Linux, deny its parent directory instead.`,
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns true if bwrap is installed and executable on this system.
|
||||
* Result is cached after the first call.
|
||||
|
|
@ -195,9 +214,12 @@ export function generateBwrapArgs(
|
|||
}
|
||||
if (isDir) {
|
||||
args.push("--tmpfs", p);
|
||||
} else {
|
||||
// File-specific entry: tool-layer checkAccessPolicy still denies read/write/edit
|
||||
// tool calls, but exec commands inside the sandbox can still access the file
|
||||
// directly. Warn operators so they know to deny the parent directory instead.
|
||||
_warnBwrapFileDenyOnce(p);
|
||||
}
|
||||
// File-specific entry: tool-layer checkAccessPolicy already denies reads/writes;
|
||||
// bwrap cannot mount tmpfs over a file so skip the OS-layer mount silently.
|
||||
}
|
||||
|
||||
// Script-specific override mounts — emitted after deny[] so they can reopen
|
||||
|
|
|
|||
Loading…
Reference in New Issue