Skip to content

Commit d578a68

Browse files
omarshahineclawsweeper-repair
omarshahine
authored and
clawsweeper-repair
committed
fix(file-transfer): address PR review feedback (security + availability)
Reviewer findings addressed (greptile + aisle): - policy: persistAllowAlways no longer escalates per-node approvals to the '*' wildcard entry; allow-always now writes under the specific node's own entry, never the wildcard (greptile P1 SECURITY). - policy: add literal '..' segment short-circuit in evaluateFilePolicy, raised before glob match. Stops "/allowed/../etc/passwd" from passing preflight against "/allowed/**" globs (aisle MEDIUM CWE-22). - file-write: replace no-op base64 try/catch with actual round-trip validation. Buffer.from(s, "base64") never throws — invalid input silently decoded to garbage bytes. Now re-encodes and compares modulo padding/url-variant chars (greptile P1 SECURITY). - file-write: document the parent-symlink residual risk and rely on the existing gateway-side post-flight policy check; full rollback requires a node-side file.unlink which is deferred to a follow-up. Initial segment-walk attempt was reverted because it false-positives on system symlinks like macOS /var → /private/var (aisle HIGH CWE-59). - dir-fetch tool: add preValidateTarball pass that runs `tar -tzvf` and rejects symlinks, hardlinks, absolute paths, '..' traversal, uncompressed sizes >64MB, and entry counts >5000 — before any extraction. Drops --no-overwrite-dir (GNU-only flag rejected by BSD tar on macOS) (aisle HIGH x2 CWE-22 + CWE-409, greptile P2). - dir-fetch tool: stream-hash files via fs.open + read loop instead of fs.readFile to avoid full-buffer reads on large extracted entries. - dir-fetch handler: replace spawnSync in countTarEntries with async spawn + bounded buffer so tar -tzf can't park the node-host event loop for up to 10s on a slow filesystem (greptile P1 AVAIL). - audit: clear auditDirPromise on rejection so a transient mkdir failure doesn't permanently silence the audit log (greptile P2). New tests: wildcard escalation rejection, base64 malformed/url-variant, '..' traversal short-circuit (3 cases). 84/84 passing.
1 parent c2d127f commit d578a68

7 files changed

Lines changed: 402 additions & 54 deletions

File tree

‎extensions/file-transfer/src/node-host/dir-fetch.ts‎

Lines changed: 51 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { spawn, spawnSync } from "node:child_process";
1+
import { spawn } from "node:child_process";
22
import crypto from "node:crypto";
33
import fs from "node:fs/promises";
44
import path from "node:path";
@@ -83,20 +83,56 @@ async function preflightDu(dirPath: string, maxBytes: number): Promise<boolean>
8383
});
8484
}
8585

86-
function countTarEntries(tarBuffer: Buffer): number {
87-
const result = spawnSync("tar", ["-tzf", "-"], {
88-
input: tarBuffer,
89-
maxBuffer: 32 * 1024 * 1024,
90-
timeout: 10000,
86+
async function countTarEntries(tarBuffer: Buffer): Promise<number> {
87+
// Async spawn so a slow `tar -tzf` doesn't park the node-host event
88+
// loop for up to 10s. Other in-flight requests continue to be served.
89+
return new Promise((resolve) => {
90+
const child = spawn("tar", ["-tzf", "-"], { stdio: ["pipe", "pipe", "ignore"] });
91+
let stdoutBuf = "";
92+
let aborted = false;
93+
const watchdog = setTimeout(() => {
94+
aborted = true;
95+
try {
96+
child.kill("SIGKILL");
97+
} catch {
98+
/* gone */
99+
}
100+
resolve(0);
101+
}, 10_000);
102+
child.stdout.on("data", (chunk: Buffer) => {
103+
stdoutBuf += chunk.toString();
104+
// Bound buffer growth — pathological archives shouldn't OOM us.
105+
if (stdoutBuf.length > 32 * 1024 * 1024) {
106+
aborted = true;
107+
try {
108+
child.kill("SIGKILL");
109+
} catch {
110+
/* gone */
111+
}
112+
clearTimeout(watchdog);
113+
resolve(0);
114+
}
115+
});
116+
child.on("close", (code) => {
117+
clearTimeout(watchdog);
118+
if (aborted) {
119+
return;
120+
}
121+
if (code !== 0) {
122+
resolve(0);
123+
return;
124+
}
125+
const lines = stdoutBuf.split("\n").filter((l) => l.trim().length > 0 && l !== "./");
126+
resolve(lines.length);
127+
});
128+
child.on("error", () => {
129+
clearTimeout(watchdog);
130+
if (!aborted) {
131+
resolve(0);
132+
}
133+
});
134+
child.stdin.end(tarBuffer);
91135
});
92-
if (result.status !== 0 || !result.stdout) {
93-
return 0;
94-
}
95-
const lines = (result.stdout as Buffer)
96-
.toString("utf-8")
97-
.split("\n")
98-
.filter((l) => l.trim().length > 0 && l !== "./");
99-
return lines.length;
100136
}
101137

102138
export async function handleDirFetch(params: DirFetchParams): Promise<DirFetchResult> {
@@ -256,7 +292,7 @@ export async function handleDirFetch(params: DirFetchParams): Promise<DirFetchRe
256292
const sha256 = crypto.createHash("sha256").update(tarBuffer).digest("hex");
257293
const tarBase64 = tarBuffer.toString("base64");
258294
const tarBytes = tarBuffer.byteLength;
259-
const fileCount = countTarEntries(tarBuffer);
295+
const fileCount = await countTarEntries(tarBuffer);
260296

261297
return {
262298
ok: true,

‎extensions/file-transfer/src/node-host/file-write.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,38 @@ describe("handleFileWrite — integrity check", () => {
191191
});
192192
});
193193

194+
describe("handleFileWrite — base64 round-trip validation", () => {
195+
it("rejects malformed base64 that silently drops characters", async () => {
196+
const target = path.join(tmpRoot, "bad.bin");
197+
// "@" is not in the base64 alphabet — Buffer.from would silently drop
198+
// it and decode "AAA" instead of failing.
199+
const r = await handleFileWrite({
200+
path: target,
201+
contentBase64: "AAA@@@",
202+
});
203+
expect(r).toMatchObject({ ok: false, code: "INVALID_BASE64" });
204+
await expect(fs.access(target)).rejects.toMatchObject({ code: "ENOENT" });
205+
});
206+
207+
it("accepts standard base64 with and without padding", async () => {
208+
const target = path.join(tmpRoot, "padded.bin");
209+
// Buffer.from("hi") -> "aGk=" with padding, "aGk" without.
210+
const r1 = await handleFileWrite({ path: target, contentBase64: "aGk=" });
211+
expect(r1.ok).toBe(true);
212+
213+
const target2 = path.join(tmpRoot, "unpadded.bin");
214+
const r2 = await handleFileWrite({ path: target2, contentBase64: "aGk" });
215+
expect(r2.ok).toBe(true);
216+
});
217+
218+
it("accepts base64url variant (-_ instead of +/)", async () => {
219+
const target = path.join(tmpRoot, "url.bin");
220+
// Buffer.from([0xfb, 0xff]) -> "+/8=" standard, "-_8=" url
221+
const r = await handleFileWrite({ path: target, contentBase64: "-_8=" });
222+
expect(r.ok).toBe(true);
223+
});
224+
});
225+
194226
describe("handleFileWrite — size cap", () => {
195227
it("rejects content larger than the 16MB cap", async () => {
196228
const target = path.join(tmpRoot, "big.bin");

‎extensions/file-transfer/src/node-host/file-write.ts‎

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -58,18 +58,20 @@ export async function handleFileWrite(
5858
return err("INVALID_PATH", "path must be absolute");
5959
}
6060

61-
// 2. Decode base64 → Buffer
62-
let buf: Buffer;
63-
try {
64-
buf = Buffer.from(contentBase64, "base64");
65-
// Verify round-trip to catch invalid base64
66-
if (
67-
buf.toString("base64") !== contentBase64 &&
68-
Buffer.from(contentBase64, "base64url").toString("base64url") !== contentBase64
69-
) {
70-
// Tolerate standard base64 with or without padding; just use what we decoded.
71-
}
72-
} catch {
61+
// 2. Decode base64 → Buffer.
62+
// Buffer.from(s, "base64") in Node never throws — it silently drops
63+
// non-base64 characters and returns whatever it could decode. That
64+
// means a typo or truncated input would land garbage on disk if we
65+
// accepted whatever decoded. Defense: round-trip the decoded buffer
66+
// back to base64 and compare against the input modulo padding/url
67+
// variants. A mismatch means characters were silently dropped.
68+
const buf = Buffer.from(contentBase64, "base64");
69+
const reEncoded = buf.toString("base64");
70+
// Normalize: drop padding and convert base64url chars to standard so the
71+
// comparison tolerates both "=" / no-"=" inputs and "-_" base64url.
72+
const normalize = (s: string): string =>
73+
s.replace(/=+$/u, "").replace(/-/gu, "+").replace(/_/gu, "/");
74+
if (normalize(reEncoded) !== normalize(contentBase64)) {
7375
return err("INVALID_BASE64", "contentBase64 is not valid base64");
7476
}
7577

@@ -104,11 +106,16 @@ export async function handleFileWrite(
104106
}
105107
}
106108

107-
// 4. Refuse to write through symlinks (lstat sees the link itself, not
108-
// its target). A path that's a symlink could escape the operator's
109-
// intended path policy — e.g., an allowed dir could contain a
110-
// symlink pointing at /etc/hosts.
111-
// Otherwise determine overwritten status and reject directories.
109+
// 4. The lstat-on-final check below catches the case where the target
110+
// itself is a symlink, but it does NOT catch a symlink in a parent
111+
// component (e.g. ~/Downloads/evil → /etc, write to .../evil/passwd).
112+
// The gateway-side post-flight check (in file-write-tool.ts)
113+
// canonicalizes via realpath after the write and re-runs policy
114+
// against that canonical path; an escape through a parent-dir
115+
// symlink surfaces there. The current behavior on a post-flight
116+
// deny is to throw loudly with the canonical path so the operator
117+
// can manually inspect — full rollback requires a node-side
118+
// file.unlink (out of scope; tracked as a follow-up).
112119
let overwritten = false;
113120
try {
114121
const existingLStat = await fs.lstat(targetPath);

‎extensions/file-transfer/src/shared/audit.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,21 @@ async function ensureAuditDir(): Promise<string> {
5151
if (auditDirPromise) {
5252
return auditDirPromise;
5353
}
54-
auditDirPromise = (async () => {
54+
const promise = (async () => {
5555
const dir = path.join(os.homedir(), ".openclaw", "audit");
5656
await fs.mkdir(dir, { recursive: true, mode: 0o700 });
5757
return dir;
5858
})();
59-
return auditDirPromise;
59+
// If the mkdir rejects (transient permission error etc.), clear the
60+
// cached singleton so the NEXT call retries instead of permanently
61+
// silencing the audit log.
62+
promise.catch(() => {
63+
if (auditDirPromise === promise) {
64+
auditDirPromise = null;
65+
}
66+
});
67+
auditDirPromise = promise;
68+
return promise;
6069
}
6170

6271
function auditFilePath(dir: string): string {

‎extensions/file-transfer/src/shared/policy.test.ts‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,41 @@ describe("evaluateFilePolicy — default deny", () => {
5656
});
5757
});
5858

59+
describe("evaluateFilePolicy — '..' traversal short-circuit", () => {
60+
it("rejects /allowed/../etc/passwd even when /allowed/** is allowed", () => {
61+
withConfig({
62+
n1: { allowReadPaths: ["/allowed/**"] },
63+
});
64+
const r = evaluateFilePolicy({
65+
nodeId: "n1",
66+
kind: "read",
67+
path: "/allowed/../etc/passwd",
68+
});
69+
expect(r).toMatchObject({ ok: false, code: "POLICY_DENIED", askable: false });
70+
expect(r.ok ? "" : r.reason).toMatch(/\.\./);
71+
});
72+
73+
it("rejects a path that ENDS in /..", () => {
74+
withConfig({
75+
n1: { allowReadPaths: ["/tmp/**"] },
76+
});
77+
const r = evaluateFilePolicy({
78+
nodeId: "n1",
79+
kind: "read",
80+
path: "/tmp/foo/..",
81+
});
82+
expect(r).toMatchObject({ ok: false, code: "POLICY_DENIED" });
83+
});
84+
85+
it("rejects bare '..'", () => {
86+
withConfig({
87+
n1: { allowReadPaths: ["/**"] },
88+
});
89+
const r = evaluateFilePolicy({ nodeId: "n1", kind: "read", path: ".." });
90+
expect(r).toMatchObject({ ok: false, code: "POLICY_DENIED" });
91+
});
92+
});
93+
5994
describe("evaluateFilePolicy — denyPaths always wins", () => {
6095
it("denies even when allowReadPaths matches", () => {
6196
withConfig({
@@ -258,6 +293,36 @@ describe("persistAllowAlways", () => {
258293
expect(root.gateway.nodes.fileTransfer["Lobster"].allowWritePaths).toContain("/srv/out.txt");
259294
});
260295

296+
it("never persists under the '*' wildcard even when '*' is the matching key", async () => {
297+
let captured: Record<string, unknown> | null = null;
298+
mutateConfigFileMock.mockImplementation(
299+
async ({ mutate }: { mutate: (draft: Record<string, unknown>) => void }) => {
300+
const draft: Record<string, unknown> = {
301+
gateway: { nodes: { fileTransfer: { "*": { allowReadPaths: ["/var/log/**"] } } } },
302+
};
303+
mutate(draft);
304+
captured = draft;
305+
},
306+
);
307+
308+
await persistAllowAlways({
309+
nodeId: "n1",
310+
nodeDisplayName: "Lobster",
311+
kind: "read",
312+
path: "/srv/added.png",
313+
});
314+
315+
const root = captured as unknown as {
316+
gateway: {
317+
nodes: { fileTransfer: Record<string, { allowReadPaths?: string[] }> };
318+
};
319+
};
320+
// The "*" entry must not have been mutated.
321+
expect(root.gateway.nodes.fileTransfer["*"].allowReadPaths).toEqual(["/var/log/**"]);
322+
// A new entry keyed by displayName (not "*") must hold the new path.
323+
expect(root.gateway.nodes.fileTransfer["Lobster"].allowReadPaths).toEqual(["/srv/added.png"]);
324+
});
325+
261326
it("dedupes when path already present", async () => {
262327
let captured: Record<string, unknown> | null = null;
263328
mutateConfigFileMock.mockImplementation(

‎extensions/file-transfer/src/shared/policy.ts‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,12 +141,40 @@ function normalizeAskMode(value: unknown): FilePolicyAskMode {
141141
* 5. ask=on-miss → POLICY_DENIED with askable=true.
142142
* 6. ask=off (or unset) → POLICY_DENIED, not askable.
143143
*/
144+
/**
145+
* Reject any path whose RAW string contains a ".." segment. Checking the
146+
* raw string (not the normalized form) is the point — `posix.normalize`
147+
* collapses "/allowed/../etc/passwd" to "/etc/passwd", which would defeat
148+
* the check. We want to flag the literal traversal sequence the agent
149+
* passed in, before any glob match runs.
150+
*
151+
* Without this, "/allowed/../etc/passwd" matches the glob "/allowed/**"
152+
* pre-realpath, so the node fetches the bytes before the post-flight
153+
* canonical-path check denies — too late, the bytes already crossed the
154+
* node→gateway boundary.
155+
*/
156+
function containsParentRefSegment(p: string): boolean {
157+
const segments = p.split("/");
158+
return segments.includes("..");
159+
}
160+
144161
export function evaluateFilePolicy(input: {
145162
nodeId: string;
146163
nodeDisplayName?: string;
147164
kind: FilePolicyKind;
148165
path: string;
149166
}): FilePolicyDecision {
167+
// Reject literal traversal sequences before consulting any allow/deny
168+
// glob list. minimatch on the raw string can wrongly accept
169+
// "/allowed/../etc/passwd" against "/allowed/**".
170+
if (containsParentRefSegment(input.path)) {
171+
return {
172+
ok: false,
173+
code: "POLICY_DENIED",
174+
reason: "path contains '..' segments; reject before glob match",
175+
askable: false,
176+
};
177+
}
150178
const config = readFilePolicyConfig();
151179
if (!config) {
152180
return {
@@ -250,7 +278,11 @@ export async function persistAllowAlways(input: {
250278
const nodes = (gateway.nodes ??= {}) as Record<string, unknown>;
251279
const fileTransfer = (nodes.fileTransfer ??= {}) as Record<string, NodeFilePolicyConfig>;
252280

253-
const candidates = [input.nodeId, input.nodeDisplayName, "*"].filter(
281+
// SECURITY: never persist allow-always under the "*" wildcard. An
282+
// operator approving a path on node A must not silently grant the
283+
// same path on every other node sharing the wildcard entry. Always
284+
// write under the specific node's own entry, creating it if needed.
285+
const candidates = [input.nodeId, input.nodeDisplayName].filter(
254286
(k): k is string => typeof k === "string" && k.length > 0,
255287
);
256288
let key = candidates.find((c) => fileTransfer[c]);

0 commit comments

Comments
 (0)