fix(models-config): security & correctness fixes from PR review

zeroaltitude · zeroaltitude · commit d505fa064c18 · 2026-04-27T19:35:53.000-07:00
Five issues raised by Aisle / Codex / Greptile review on PR #72869, addressed inline rather than deferred: 1. CWE-59 symlink-following chmod (Aisle high #1) ensureModelsFileModeForModelsJson called fs.chmod on a path that may be replaced by a symlink. If an attacker can write to the agent dir (or OPENCLAW_AGENT_DIR points there), the chmod followed the link and changed perms on an arbitrary owned file. Now lstat first and refuse to chmod symlinks or non-regular files. 2. Prototype pollution via JSON keys (Aisle medium #3 / CWE-1321) stripAuthProfilesVolatileFields() copied untrusted keys into a plain {} object. Special keys '__proto__', 'constructor', 'prototype' could mutate the result's prototype chain. Now uses Object.create(null) for the result and explicitly filters those three keys (belt-and-suspenders). 3. DoS via unbounded auth-profiles fingerprinting (Aisle medium #4) readAuthProfilesStableHash had no size or depth limits. - Added MAX_AUTH_PROFILES_BYTES = 8 MiB. Above the cap we hash raw bytes instead of running JSON.parse + recursive transform + stable-stringify. - Added MAX_AUTH_PROFILES_DEPTH = 64 with a depth-cap marker so the recursive walk can't stack-overflow on pathologically nested input. 4. 'token' incorrectly stripped from fingerprint (Codex P2 / Greptile P2) AUTH_PROFILE_VOLATILE_FIELDS included 'token' to keep OAuth session token rotation from invalidating the cache. But profiles with type: 'token' use the literal 'token' key as a long-lived static credential — stripping it would mask real auth-state changes when a user rotates a static API token. Removed 'token' from the volatile set and documented the boundary inline. OAuth session fields ('access', 'refresh') and timing fields stay stripped. Skipped from this commit (will reply on threads): - Cache short-circuit on stale on-disk credentials (Codex/Greptile P1): separate concern from the security fixes; needs design discussion on whether to validate disk-vs-config or remove the short-circuit. - models.json drift in cache key (Codex P1): same — touches the fingerprint shape and overlaps with the targetProvider short-circuit. - targetProvider short-circuit untested (Greptile P2): test follow-up once the short-circuit semantics are settled. - Aisle medium #5 (raw secrets in fingerprint cache): structurally larger refactor; needs to land separately to keep this commit\'s blast radius clear. Lint: 0 errors. TS: clean.
diff --git a/src/agents/models-config.ts b/src/agents/models-config.ts
@@ -28,7 +28,14 @@ export { resetModelsJsonReadyCacheForTest } from "./models-config-state.js";
 const AUTH_PROFILE_VOLATILE_FIELDS: ReadonlySet<string> = new Set([
   "access",
   "refresh",
-  "token",
+  // NOTE: "token" was previously stripped as a volatile field, but profiles
+  // with `type: "token"` use the literal `token` key as a long-lived
+  // credential identifier (Greptile P2 / Codex P2 on PR #72869). Stripping
+  // it would mask real auth-state changes — e.g. user rotates a static
+  // API token but the cached fingerprint stays identical, so the
+  // implicit-provider-discovery pipeline never re-runs. Keep the OAuth
+  // session fields above ("access"/"refresh") and the timing fields below,
+  // but treat "token" as significant content.
   "expires",
   "expiresAt",
   "expiresIn",
@@ -39,6 +46,35 @@ const AUTH_PROFILE_VOLATILE_FIELDS: ReadonlySet<string> = new Set([
   "lastValidatedAt",
 ]);
 
+/**
+ * Hard cap on the bytes we will read + parse from auth-profiles.json when
+ * computing the stable fingerprint hash (Aisle medium #4 on PR #72869).
+ * Without a cap, a crafted/large profile file becomes a CPU + memory
+ * exhaustion vector via fs.readFile + JSON.parse + recursive walk +
+ * stableStringify. 8 MiB is far above any plausible legitimate auth-
+ * profiles size and still bounds the worst-case allocation.
+ */
+const MAX_AUTH_PROFILES_BYTES = 8 * 1024 * 1024;
+
+/**
+ * Maximum recursion depth when stripping volatile fields. Bounds the
+ * recursive walk so deeply-nested JSON cannot stack-overflow the gateway
+ * during fingerprinting (Aisle medium #4).
+ */
+const MAX_AUTH_PROFILES_DEPTH = 64;
+
+/**
+ * Keys that mutate Object prototype when assigned with bracket syntax,
+ * triggering prototype pollution (CWE-1321). We always skip these when
+ * building the stripped fingerprint object even though the result is
+ * immediately stable-stringified — defence in depth (Aisle medium #3).
+ */
+const DANGEROUS_PROTO_KEYS: ReadonlySet<string> = new Set([
+  "__proto__",
+  "prototype",
+  "constructor",
+]);
+
 /**
  * Compute a content-based fingerprint for a JSON file whose mtime may
  * change without meaningful content change (e.g. auth-profiles.json rewritten
@@ -49,6 +85,23 @@ const AUTH_PROFILE_VOLATILE_FIELDS: ReadonlySet<string> = new Set([
  * exists.
  */
 async function readAuthProfilesStableHash(pathname: string): Promise<string | null> {
+  // Aisle medium #4: bound the read by file size before pulling it into
+  // memory + JSON.parse + recursive walk. Above the cap we hash the raw
+  // bytes (already-streamed by readFile, but at least we avoid the
+  // recursive transform / stringify cost) instead of the parsed shape.
+  const stat = await fs.stat(pathname).catch(() => null);
+  if (!stat) {
+    return null;
+  }
+  if (stat.size > MAX_AUTH_PROFILES_BYTES) {
+    let raw: Buffer;
+    try {
+      raw = await fs.readFile(pathname);
+    } catch {
+      return null;
+    }
+    return createHash("sha256").update(raw).digest("hex");
+  }
   let raw: string;
   try {
     raw = await fs.readFile(pathname, "utf8");
@@ -63,23 +116,37 @@ async function readAuthProfilesStableHash(pathname: string): Promise<string | nu
     // changes, but avoid using mtime.
     return createHash("sha256").update(raw).digest("hex");
   }
-  const stable = stripAuthProfilesVolatileFields(parsed);
+  const stable = stripAuthProfilesVolatileFields(parsed, 0);
   return createHash("sha256").update(stableStringify(stable)).digest("hex");
 }
 
-function stripAuthProfilesVolatileFields(value: unknown): unknown {
+function stripAuthProfilesVolatileFields(value: unknown, depth: number): unknown {
+  // Aisle medium #4: bound recursion to prevent stack overflow on
+  // pathologically nested JSON. At the cap we serialize the subtree as a
+  // shallow marker; this still produces a stable hash (any change at or
+  // below the cap rolls into the parent's stringification).
+  if (depth >= MAX_AUTH_PROFILES_DEPTH) {
+    return "[depth-capped]";
+  }
   if (value === null || typeof value !== "object") {
     return value;
   }
   if (Array.isArray(value)) {
-    return value.map((entry) => stripAuthProfilesVolatileFields(entry));
+    return value.map((entry) => stripAuthProfilesVolatileFields(entry, depth + 1));
   }
-  const result: Record<string, unknown> = {};
+  // Aisle medium #3: build with Object.create(null) so prototype-mutating
+  // keys ("__proto__", "constructor", "prototype") in untrusted input
+  // can't pollute the resulting object's prototype chain. Filter them
+  // explicitly too — belt and suspenders.
+  const result: Record<string, unknown> = Object.create(null);
   for (const [key, entry] of Object.entries(value as Record<string, unknown>)) {
     if (AUTH_PROFILE_VOLATILE_FIELDS.has(key)) {
       continue;
     }
-    result[key] = stripAuthProfilesVolatileFields(entry);
+    if (DANGEROUS_PROTO_KEYS.has(key)) {
+      continue;
+    }
+    result[key] = stripAuthProfilesVolatileFields(entry, depth + 1);
   }
   return result;
 }
@@ -152,6 +219,24 @@ async function readExistingModelsFile(pathname: string): Promise<{
 }
 
 export async function ensureModelsFileModeForModelsJson(pathname: string): Promise<void> {
+  // Aisle high #1 on PR #72869 (CWE-59 symlink-following chmod): refuse to
+  // chmod a symlink. fs.chmod follows links, so if an attacker can replace
+  // ${agentDir}/models.json with a symlink pointing at a sensitive file
+  // owned by the gateway user, this best-effort chmod would change
+  // permissions on the link target instead. lstat first; if the path is a
+  // symlink (or anything other than a regular file), bail.
+  let stat: Awaited<ReturnType<typeof fs.lstat>>;
+  try {
+    stat = await fs.lstat(pathname);
+  } catch {
+    return; // best-effort — file may not exist yet
+  }
+  if (stat.isSymbolicLink()) {
+    return;
+  }
+  if (!stat.isFile()) {
+    return;
+  }
   await fs.chmod(pathname, 0o600).catch(() => {
     // best-effort
   });
