fix(openshell): pin sandbox file reads (#69798)

* fix(openshell): pin sandbox file reads against parent symlink swaps

* docs(changelog): note openshell sandbox read pinning (#69798)

* fix(openshell): containment-check against literal root and self-contain file-identity helper

* test(openshell): spy on fsPromises.open for swap races, skip dev=0 test on win32

* fix(openshell): single-syscall fallback identity check + tighten sameFileIdentity types

* fix(openshell): re-fstat pinned handle after identity check for defense-in-depth

* fix(openshell): lstat leaf on platforms without O_NOFOLLOW to close windows symlink gap

* fix(openshell): expose test seam for O_NOFOLLOW availability instead of patching native constants

extensions/openshell/src/fs-bridge.ts

@@ -1,4 +1,6 @@
+import fs from "node:fs";
 import fsPromises from "node:fs/promises";
+import type { FileHandle } from "node:fs/promises";
 import path from "node:path";
 import { writeFileWithinRoot } from "openclaw/plugin-sdk/infra-runtime";
 import type {
@@ -50,13 +52,16 @@ class OpenShellFsBridge implements SandboxFsBridge {
   }): Promise<Buffer> {
     const target = this.resolveTarget(params);
     const hostPath = this.requireHostPath(target);
-    await assertLocalPathSafety({
-      target,
-      root: target.mountHostRoot,
-      allowMissingLeaf: false,
-      allowFinalSymlinkForUnlink: false,
+    const handle = await openPinnedReadableFile({
+      absolutePath: hostPath,
+      rootPath: target.mountHostRoot,
+      containerPath: target.containerPath,
     });
-    return await fsPromises.readFile(hostPath);
+    try {
+      return (await handle.readFile()) as Buffer;
+    } finally {
+      await handle.close();
+    }
   }
 
   async writeFile(params: {
@@ -352,3 +357,200 @@ async function resolveCanonicalCandidate(targetPath: string): Promise<string> {
     cursor = parent;
   }
 }
+
+async function openPinnedReadableFile(params: {
+  absolutePath: string;
+  rootPath: string;
+  containerPath: string;
+}): Promise<FileHandle> {
+  // The literal root is what `resolveTarget` joins caller-provided relative
+  // paths against, so pre-open containment must be checked in literal form.
+  // The canonical root is derived separately and used for the post-open
+  // path checks (fd-path readlink and realpath cross-check), so a workspace
+  // that is itself configured as a symlink still works.
+  const literalRoot = path.resolve(params.rootPath);
+  const canonicalRoot = await fsPromises.realpath(literalRoot).catch(() => literalRoot);
+  const literalPath = path.resolve(params.absolutePath);
+  // Cheap string-prefix check on the caller-provided absolute path; no
+  // filesystem state is read here, so there is no TOCTOU window. Deeper
+  // checks run after the fd is pinned.
+  if (!isPathInside(literalRoot, literalPath)) {
+    throw new Error(`Sandbox path escapes allowed mounts; cannot access: ${params.containerPath}`);
+  }
+  const { flags: openReadFlags, supportsNoFollow } = resolveOpenReadFlags();
+  // Open first so every later check runs against an fd that is already pinned
+  // to one specific inode. `O_NOFOLLOW` prevents the final path component from
+  // being a symlink; the ancestor walk below handles parent-directory symlink
+  // swaps on platforms where fd-path readlink is not available.
+  const handle = await fsPromises.open(literalPath, openReadFlags);
+  try {
+    const openedStat = await handle.stat();
+    if (!openedStat.isFile()) {
+      throw new Error(`Sandbox boundary checks failed; cannot read files: ${params.containerPath}`);
+    }
+    if (openedStat.nlink > 1) {
+      throw new Error(`Sandbox boundary checks failed; cannot read files: ${params.containerPath}`);
+    }
+    const resolvedPath = await resolveOpenedReadablePath(handle.fd);
+    if (resolvedPath !== null) {
+      // Primary guarantee on Linux: the fd's resolved path is derived from the
+      // kernel, so a parent-directory swap cannot make this return a stale path.
+      if (!isPathInside(canonicalRoot, resolvedPath)) {
+        throw new Error(
+          `Sandbox boundary checks failed; cannot read files: ${params.containerPath}`,
+        );
+      }
+      return handle;
+    }
+    // Fallback for platforms where fd-path readlink is unavailable. On macOS,
+    // `/dev/fd/N` is a character device so readlink returns EINVAL; on Windows
+    // there is no `/proc` equivalent. With no kernel-backed path readback we
+    // must prove the pinned fd is in-root without trusting a separate
+    // `realpath` + `lstat` pair that would race between the two awaits. Walk
+    // every ancestor between `literalRoot` and `literalPath` — the actual
+    // on-disk chain — and reject if any ancestor is a symlink, then use a
+    // single `stat` call to confirm that the path still resolves to the
+    // same file the fd has pinned. `fs.promises.stat` resolves the path and
+    // returns the final file's identity in one syscall, so there is no
+    // between-await window for an attacker to race.
+    await assertAncestorChainHasNoSymlinks(literalRoot, literalPath, params.containerPath, {
+      // On platforms where `O_NOFOLLOW` is unavailable (Windows), the open
+      // call would have transparently followed a final-component symlink, so
+      // the ancestor walk has to lstat the leaf as well.
+      includeLeaf: !supportsNoFollow,
+    });
+    const currentResolvedStat = await fsPromises.stat(literalPath);
+    if (!sameFileIdentity(currentResolvedStat, openedStat)) {
+      throw new Error(`Sandbox boundary checks failed; cannot read files: ${params.containerPath}`);
+    }
+    // Belt-and-suspenders: re-fstat the pinned fd after the identity check and
+    // confirm the file type and link count are still trustworthy. A hardlink
+    // that appeared between the initial fstat and here is not exploitable for
+    // the read (the fd is already pinned to the original inode), but failing
+    // closed here keeps the guarantee simple: the bytes we return always come
+    // from a file that was a single-linked regular file at verification time.
+    const postCheckStat = await handle.stat();
+    if (!postCheckStat.isFile() || postCheckStat.nlink > 1) {
+      throw new Error(`Sandbox boundary checks failed; cannot read files: ${params.containerPath}`);
+    }
+    return handle;
+  } catch (error) {
+    await handle.close();
+    throw error;
+  }
+}
+
+// Walks each directory between canonicalRoot (exclusive) and
+// targetAbsolutePath, `lstat`'ing each segment. Rejects if any intermediate
+// segment is a symlink or a non-directory. By default the final component is
+// not walked because `O_NOFOLLOW` already protects it on the open call. Pass
+// `includeLeaf: true` on platforms where `O_NOFOLLOW` is unavailable
+// (Windows) so a symlinked leaf cannot be followed silently by `open`.
+async function assertAncestorChainHasNoSymlinks(
+  canonicalRoot: string,
+  targetAbsolutePath: string,
+  containerPath: string,
+  options: { includeLeaf?: boolean } = {},
+): Promise<void> {
+  const relative = path.relative(canonicalRoot, targetAbsolutePath);
+  if (relative === "" || relative.startsWith("..") || path.isAbsolute(relative)) {
+    return;
+  }
+  const segments = relative.split(path.sep).filter((segment) => segment.length > 0);
+  const lastIndex = options.includeLeaf ? segments.length : segments.length - 1;
+  let cursor = canonicalRoot;
+  for (let i = 0; i < lastIndex; i += 1) {
+    cursor = path.join(cursor, segments[i]);
+    const stat = await fsPromises.lstat(cursor).catch(() => null);
+    if (!stat) {
+      throw new Error(`Sandbox boundary checks failed; cannot read files: ${containerPath}`);
+    }
+    const isLeaf = i === segments.length - 1;
+    if (stat.isSymbolicLink()) {
+      throw new Error(`Sandbox boundary checks failed; cannot read files: ${containerPath}`);
+    }
+    if (!isLeaf && !stat.isDirectory()) {
+      throw new Error(`Sandbox boundary checks failed; cannot read files: ${containerPath}`);
+    }
+  }
+}
+
+type ReadOpenFlagsResolution = { flags: number; supportsNoFollow: boolean };
+
+let readOpenFlagsResolverForTest: (() => ReadOpenFlagsResolution) | undefined;
+
+function resolveOpenReadFlags(): ReadOpenFlagsResolution {
+  if (readOpenFlagsResolverForTest) {
+    return readOpenFlagsResolverForTest();
+  }
+  const closeOnExec = (fs.constants as Record<string, number>).O_CLOEXEC ?? 0;
+  const supportsNoFollow = typeof fs.constants.O_NOFOLLOW === "number";
+  const noFollow = supportsNoFollow ? fs.constants.O_NOFOLLOW : 0;
+  return {
+    flags: fs.constants.O_RDONLY | noFollow | closeOnExec,
+    supportsNoFollow,
+  };
+}
+
+/**
+ * Test-only seam for forcing the open-flag/`O_NOFOLLOW` resolution. Used to
+ * exercise the Windows-style fallback (no `O_NOFOLLOW`, ancestor walk
+ * includes the leaf) on platforms where `fs.constants.O_NOFOLLOW` is a
+ * non-configurable native data property and cannot be patched directly.
+ *
+ * @internal
+ */
+export function setReadOpenFlagsResolverForTest(
+  resolver: (() => ReadOpenFlagsResolution) | undefined,
+): void {
+  readOpenFlagsResolverForTest = resolver;
+}
+
+// Resolves the absolute path associated with an open fd via the kernel-backed
+// `/proc/self/fd/<fd>` (Linux) or `/dev/fd/<fd>` (some BSDs). Returns null
+// when no fd-path endpoint is available. Note: on macOS `/dev/fd/N` is a
+// character device rather than a symlink, so `readlink` fails with EINVAL
+// there and the caller must use the ancestor-walk fallback instead.
+async function resolveOpenedReadablePath(fd: number): Promise<string | null> {
+  for (const fdPath of [`/proc/self/fd/${fd}`, `/dev/fd/${fd}`]) {
+    try {
+      const openedPath = await fsPromises.readlink(fdPath);
+      return normalizeOpenedReadablePath(openedPath);
+    } catch {
+      continue;
+    }
+  }
+  return null;
+}
+
+function normalizeOpenedReadablePath(openedPath: string): string {
+  const deletedSuffix = " (deleted)";
+  const withoutDeletedSuffix = openedPath.endsWith(deletedSuffix)
+    ? openedPath.slice(0, -deletedSuffix.length)
+    : openedPath;
+  return path.resolve(withoutDeletedSuffix);
+}
+
+// File identity comparison with win32-aware `dev=0` handling, matching the
+// shared `src/infra/file-identity.ts` contract. Kept local because extension
+// production code is not allowed to reach into core `src/**` by relative
+// import, and this helper is not yet part of the `openclaw/plugin-sdk/*`
+// public surface. Stats here come from `FileHandle.stat()` / `fs.promises.stat()`
+// with no `{ bigint: true }` option, so all fields are numbers.
+function sameFileIdentity(
+  left: { dev: number; ino: number },
+  right: { dev: number; ino: number },
+  platform: NodeJS.Platform = process.platform,
+): boolean {
+  if (left.ino !== right.ino) {
+    return false;
+  }
+  if (left.dev === right.dev) {
+    return true;
+  }
+  // On Windows, path-based stat can report `dev=0` while fd-based stat reports
+  // a real volume serial. Treat either side `dev=0` as "unknown device"
+  // rather than a mismatch so legitimate Windows fallback reads are not
+  // rejected.
+  return platform === "win32" && (left.dev === 0 || right.dev === 0);
+}

extensions/openshell/src/openshell-core.test.ts

@@ -1,3 +1,4 @@
+import nodeFs from "node:fs";
 import fs from "node:fs/promises";
 import os from "node:os";
 import path from "node:path";
@@ -200,6 +201,22 @@ afterEach(async () => {
   await Promise.all(tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true })));
 });
 
+function cloneStatWithDev<T extends nodeFs.Stats | nodeFs.BigIntStats>(
+  stat: T,
+  dev: number | bigint,
+): T {
+  return Object.defineProperty(
+    Object.create(Object.getPrototypeOf(stat), Object.getOwnPropertyDescriptors(stat)),
+    "dev",
+    {
+      value: dev,
+      configurable: true,
+      enumerable: true,
+      writable: true,
+    },
+  ) as T;
+}
+
 function createMirrorBackendMock(): OpenShellSandboxBackend {
   return {
     id: "openshell",
@@ -307,6 +324,247 @@ describe("openshell fs bridges", () => {
     expect(backend.syncLocalPathToRemote).not.toHaveBeenCalled();
   });
 
+  it("rejects a parent symlink swap that lands outside the sandbox root", async () => {
+    const workspaceDir = await makeTempDir("openclaw-openshell-fs-");
+    const outsideDir = await makeTempDir("openclaw-openshell-outside-");
+    await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true });
+    await fs.writeFile(path.join(workspaceDir, "subdir", "secret.txt"), "inside", "utf8");
+    await fs.writeFile(path.join(outsideDir, "secret.txt"), "outside", "utf8");
+    const backend = createMirrorBackendMock();
+    const sandbox = createSandboxTestContext({
+      overrides: {
+        backendId: "openshell",
+        workspaceDir,
+        agentWorkspaceDir: workspaceDir,
+        containerWorkdir: "/sandbox",
+      },
+    });
+
+    const { createOpenShellFsBridge } = await import("./fs-bridge.js");
+    const bridge = createOpenShellFsBridge({ sandbox, backend });
+    const originalOpen = fs.open.bind(fs);
+    const targetPath = path.join(workspaceDir, "subdir", "secret.txt");
+    let swapped = false;
+    const openSpy = vi.spyOn(fs, "open").mockImplementation((async (...args: unknown[]) => {
+      const filePath = args[0];
+      if (!swapped && filePath === targetPath) {
+        swapped = true;
+        nodeFs.rmSync(path.join(workspaceDir, "subdir"), { recursive: true, force: true });
+        nodeFs.symlinkSync(outsideDir, path.join(workspaceDir, "subdir"));
+      }
+      return await (originalOpen as (...delegated: unknown[]) => Promise<unknown>)(...args);
+    }) as unknown as typeof fs.open);
+
+    try {
+      await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).rejects.toThrow(
+        "Sandbox boundary checks failed",
+      );
+      expect(openSpy).toHaveBeenCalled();
+    } finally {
+      openSpy.mockRestore();
+    }
+  });
+
+  it("falls back to inode checks when fd path resolution is unavailable", async () => {
+    const workspaceDir = await makeTempDir("openclaw-openshell-fs-");
+    await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true });
+    await fs.writeFile(path.join(workspaceDir, "subdir", "secret.txt"), "inside", "utf8");
+
+    const backend = createMirrorBackendMock();
+    const sandbox = createSandboxTestContext({
+      overrides: {
+        backendId: "openshell",
+        workspaceDir,
+        agentWorkspaceDir: workspaceDir,
+        containerWorkdir: "/sandbox",
+      },
+    });
+
+    const { createOpenShellFsBridge } = await import("./fs-bridge.js");
+    const bridge = createOpenShellFsBridge({ sandbox, backend });
+    const readlinkSpy = vi.spyOn(fs, "readlink").mockRejectedValue(new Error("fd path unavailable"));
+
+    try {
+      await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).resolves.toEqual(
+        Buffer.from("inside"),
+      );
+      expect(readlinkSpy).toHaveBeenCalled();
+    } finally {
+      readlinkSpy.mockRestore();
+    }
+  });
+
+  // The shared `sameFileIdentity` contract intentionally treats either-side
+  // `dev=0` as "unknown device" on win32 (path-based stat can legitimately
+  // report `dev=0` there) and only fails closed on other platforms. Skip the
+  // Linux/macOS rejection expectation on Windows runners.
+  it.skipIf(process.platform === "win32")(
+    "rejects fallback reads when path stats report an unknown device id",
+    async () => {
+    const workspaceDir = await makeTempDir("openclaw-openshell-fs-");
+    const targetPath = path.join(workspaceDir, "subdir", "secret.txt");
+    await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true });
+    await fs.writeFile(targetPath, "inside", "utf8");
+
+    const backend = createMirrorBackendMock();
+    const sandbox = createSandboxTestContext({
+      overrides: {
+        backendId: "openshell",
+        workspaceDir,
+        agentWorkspaceDir: workspaceDir,
+        containerWorkdir: "/sandbox",
+      },
+    });
+
+    const { createOpenShellFsBridge } = await import("./fs-bridge.js");
+    const bridge = createOpenShellFsBridge({ sandbox, backend });
+    const readlinkSpy = vi.spyOn(fs, "readlink").mockRejectedValue(new Error("fd path unavailable"));
+    const originalStat = fs.stat.bind(fs);
+    const statSpy = vi.spyOn(fs, "stat").mockImplementation(async (...args) => {
+      const stat = await originalStat(...args);
+      if (args[0] === targetPath) {
+        return cloneStatWithDev(stat, 0);
+      }
+      return stat;
+    });
+
+    try {
+      await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).rejects.toThrow(
+        "Sandbox boundary checks failed",
+      );
+      expect(readlinkSpy).toHaveBeenCalled();
+      expect(statSpy).toHaveBeenCalledWith(targetPath);
+    } finally {
+      statSpy.mockRestore();
+      readlinkSpy.mockRestore();
+    }
+    },
+  );
+
+  it("rejects fallback reads when an ancestor directory is swapped to a symlink", async () => {
+    const workspaceDir = await makeTempDir("openclaw-openshell-fs-");
+    const outsideDir = await makeTempDir("openclaw-openshell-outside-");
+    await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true });
+    await fs.writeFile(path.join(workspaceDir, "subdir", "secret.txt"), "inside", "utf8");
+    await fs.writeFile(path.join(outsideDir, "secret.txt"), "outside", "utf8");
+
+    const backend = createMirrorBackendMock();
+    const sandbox = createSandboxTestContext({
+      overrides: {
+        backendId: "openshell",
+        workspaceDir,
+        agentWorkspaceDir: workspaceDir,
+        containerWorkdir: "/sandbox",
+      },
+    });
+
+    const { createOpenShellFsBridge } = await import("./fs-bridge.js");
+    const bridge = createOpenShellFsBridge({ sandbox, backend });
+    const originalOpen = fs.open.bind(fs);
+    const targetPath = path.join(workspaceDir, "subdir", "secret.txt");
+    let swapped = false;
+    const openSpy = vi.spyOn(fs, "open").mockImplementation((async (...args: unknown[]) => {
+      const filePath = args[0];
+      if (!swapped && filePath === targetPath) {
+        swapped = true;
+        nodeFs.rmSync(path.join(workspaceDir, "subdir"), { recursive: true, force: true });
+        nodeFs.symlinkSync(outsideDir, path.join(workspaceDir, "subdir"));
+      }
+      return await (originalOpen as (...delegated: unknown[]) => Promise<unknown>)(...args);
+    }) as unknown as typeof fs.open);
+    // Force the fallback verification path even on Linux so the ancestor-walk
+    // guard is exercised directly.
+    const readlinkSpy = vi.spyOn(fs, "readlink").mockRejectedValue(new Error("fd path unavailable"));
+
+    try {
+      await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).rejects.toThrow(
+        "Sandbox boundary checks failed",
+      );
+      expect(openSpy).toHaveBeenCalled();
+      expect(readlinkSpy).toHaveBeenCalled();
+    } finally {
+      readlinkSpy.mockRestore();
+      openSpy.mockRestore();
+    }
+  });
+
+  it("rejects fallback reads of a symlinked leaf when O_NOFOLLOW is unavailable", async () => {
+    const workspaceDir = await makeTempDir("openclaw-openshell-fs-");
+    const outsideDir = await makeTempDir("openclaw-openshell-outside-");
+    await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true });
+    await fs.writeFile(path.join(outsideDir, "secret.txt"), "outside", "utf8");
+    // The workspace contains a symlink as the FINAL path component pointing
+    // out-of-root. On Windows `O_NOFOLLOW` is `undefined`, so `open` would
+    // silently traverse the symlink to the outside file; the ancestor walk
+    // must lstat the leaf in that case to fail closed.
+    await fs.symlink(
+      path.join(outsideDir, "secret.txt"),
+      path.join(workspaceDir, "subdir", "secret.txt"),
+    );
+
+    const backend = createMirrorBackendMock();
+    const sandbox = createSandboxTestContext({
+      overrides: {
+        backendId: "openshell",
+        workspaceDir,
+        agentWorkspaceDir: workspaceDir,
+        containerWorkdir: "/sandbox",
+      },
+    });
+
+    const { createOpenShellFsBridge, setReadOpenFlagsResolverForTest } = await import(
+      "./fs-bridge.js"
+    );
+    const bridge = createOpenShellFsBridge({ sandbox, backend });
+    // Force the fallback path so the leaf-lstat guard is exercised.
+    const readlinkSpy = vi.spyOn(fs, "readlink").mockRejectedValue(new Error("fd path unavailable"));
+    // Simulate a host that lacks `O_NOFOLLOW` (e.g. Windows) without touching
+    // the non-configurable native `fs.constants` data property. The bridge
+    // exposes a test-only seam for exactly this case.
+    setReadOpenFlagsResolverForTest(() => ({
+      flags: nodeFs.constants.O_RDONLY,
+      supportsNoFollow: false,
+    }));
+
+    try {
+      await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).rejects.toThrow(
+        "Sandbox boundary checks failed",
+      );
+      expect(readlinkSpy).toHaveBeenCalled();
+    } finally {
+      setReadOpenFlagsResolverForTest(undefined);
+      readlinkSpy.mockRestore();
+    }
+  });
+
+  it("rejects hardlinked files inside the sandbox root", async () => {
+    const workspaceDir = await makeTempDir("openclaw-openshell-fs-");
+    const outsideDir = await makeTempDir("openclaw-openshell-outside-");
+    await fs.mkdir(path.join(workspaceDir, "subdir"), { recursive: true });
+    await fs.writeFile(path.join(outsideDir, "secret.txt"), "outside", "utf8");
+    await fs.link(
+      path.join(outsideDir, "secret.txt"),
+      path.join(workspaceDir, "subdir", "secret.txt"),
+    );
+
+    const backend = createMirrorBackendMock();
+    const sandbox = createSandboxTestContext({
+      overrides: {
+        backendId: "openshell",
+        workspaceDir,
+        agentWorkspaceDir: workspaceDir,
+        containerWorkdir: "/sandbox",
+      },
+    });
+
+    const { createOpenShellFsBridge } = await import("./fs-bridge.js");
+    const bridge = createOpenShellFsBridge({ sandbox, backend });
+
+    await expect(bridge.readFile({ filePath: "subdir/secret.txt" })).rejects.toThrow(
+      "Sandbox boundary checks failed",
+    );
+  });
+
   it("maps agent mount paths when the sandbox workspace is read-only", async () => {
     const workspaceDir = await makeTempDir("openclaw-openshell-fs-");
     const agentWorkspaceDir = await makeTempDir("openclaw-openshell-agent-");