fix(security): refuse to follow symlinks when writing /tmp session marker (colbymchenry#280)

aaronjmars · aaronjmars · web-flow · commit cda42c82223b · 2026-05-21T16:58:41.000-05:00
`markSessionConsulted` writes `${tmpdir()}/codegraph-consulted-${hash}` on
every `codegraph_context` call so external tooling can detect that an MCP
session has consulted CodeGraph. The old `writeFileSync` followed symlinks
unconditionally, so on a multi-user system any other local user could
pre-create that marker path as a symlink pointing at a victim-writable
file — the next codegraph context call would then overwrite the target's
contents with the ISO timestamp string (CWE-59).

The session-id hash gates predictability and makes opportunistic exploit
infeasible on its own, but tmpdir() is world-writable (mode 1777 on Linux)
and the proper pattern is to never follow links into a shared-prefix
tmpfile. Switch to `openSync` with O_NOFOLLOW + mode 0o600. ELOOP from a
planted symlink lands in the existing silent-fail catch — refuse to write
rather than touch an attacker-chosen target.

Detected by Aeon + manual review.
Severity: medium
CWE-59 (link following), CWE-732 (incorrect permission for critical resource)

Co-authored-by: aaronjmars &lt;aaron@aeon.local&gt;
diff --git a/__tests__/security.test.ts b/__tests__/security.test.ts
@@ -533,3 +533,94 @@ describe('Symlink Cycle Detection', () => {
     expect(files).toContain('src/valid.ts');
   });
 });
+
+describe('Session marker symlink resistance', () => {
+  // The marker write lives in src/mcp/tools.ts behind handleContext. We exercise
+  // it end-to-end via ToolHandler.execute so the test exercises the same code
+  // path Claude Code drives. The session id is per-test so other parallel test
+  // runs can't collide with the marker file we plant a symlink at.
+  const SESSION_ID = `cg-test-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2)}`;
+  const crypto = require('crypto') as typeof import('crypto');
+  const hash = crypto.createHash('md5').update(SESSION_ID).digest('hex').slice(0, 16);
+  const markerPath = path.join(os.tmpdir(), `codegraph-consulted-${hash}`);
+
+  let projectDir: string;
+  let victimDir: string;
+  let victimFile: string;
+
+  beforeEach(async () => {
+    projectDir = createTempDir();
+    victimDir = createTempDir();
+    victimFile = path.join(victimDir, 'private.txt');
+    fs.writeFileSync(victimFile, 'SECRET-DO-NOT-OVERWRITE\n');
+    if (fs.existsSync(markerPath)) fs.unlinkSync(markerPath);
+
+    // A real .codegraph/ has to exist for handleContext to get past the
+    // "not initialized" guard — index a tiny fixture so the call reaches the
+    // marker write step rather than short-circuiting on missing project state.
+    fs.writeFileSync(path.join(projectDir, 'a.ts'), 'export const x = 1;\n');
+    const cg = await CodeGraph.init(projectDir);
+    await cg.indexAll();
+    cg.close();
+  });
+
+  afterEach(() => {
+    if (fs.existsSync(markerPath)) fs.unlinkSync(markerPath);
+    cleanupTempDir(projectDir);
+    cleanupTempDir(victimDir);
+  });
+
+  it('does not follow a pre-planted symlink at the marker path', async () => {
+    // Skip on platforms where the user can't create symlinks (Windows without
+    // dev mode + admin). The CWE-59 risk we're guarding against doesn't apply
+    // when symlinks aren't creatable, so the skip is correct, not a gap.
+    try {
+      fs.symlinkSync(victimFile, markerPath);
+    } catch {
+      return;
+    }
+
+    const cg = await CodeGraph.open(projectDir);
+    const handler = new ToolHandler(cg);
+    process.env.CLAUDE_SESSION_ID = SESSION_ID;
+    try {
+      await handler.execute('codegraph_context', { task: 'find x' });
+    } finally {
+      delete process.env.CLAUDE_SESSION_ID;
+      cg.close();
+    }
+
+    // The victim file's contents must be untouched — the old writeFileSync
+    // path would have followed the symlink and written an ISO timestamp here.
+    expect(fs.readFileSync(victimFile, 'utf8')).toBe('SECRET-DO-NOT-OVERWRITE\n');
+
+    // And the marker path itself must still be the symlink we planted —
+    // no fallback path that quietly unlinked + recreated it (which would
+    // also work, but is a behavior we don't want to silently rely on).
+    expect(fs.lstatSync(markerPath).isSymbolicLink()).toBe(true);
+  });
+
+  it('writes the marker file with 0o600 perms on a clean path', async () => {
+    // No symlink planted — happy path. Verifies the new openSync(mode: 0o600)
+    // call is what actually lands on disk (regression guard for the perm
+    // tightening that came with the O_NOFOLLOW fix).
+    const cg = await CodeGraph.open(projectDir);
+    const handler = new ToolHandler(cg);
+    process.env.CLAUDE_SESSION_ID = SESSION_ID;
+    try {
+      await handler.execute('codegraph_context', { task: 'find x' });
+    } finally {
+      delete process.env.CLAUDE_SESSION_ID;
+      cg.close();
+    }
+
+    expect(fs.existsSync(markerPath)).toBe(true);
+    // chmod's low 9 bits — strip the file-type bits for a clean compare.
+    // Windows can't enforce 0o600 in the POSIX sense; skip the assertion
+    // there since the underlying OS will normalize the mode anyway.
+    if (process.platform !== 'win32') {
+      const mode = fs.statSync(markerPath).mode & 0o777;
+      expect(mode).toBe(0o600);
+    }
+  });
+});
diff --git a/src/mcp/tools.ts b/src/mcp/tools.ts
@@ -7,7 +7,14 @@
 import CodeGraph, { findNearestCodeGraphRoot } from '../index';
 import type { Node, Edge, SearchResult, Subgraph, TaskContext, NodeKind } from '../types';
 import { createHash } from 'crypto';
-import { writeFileSync, readFileSync, existsSync } from 'fs';
+import {
+  constants as fsConstants,
+  closeSync,
+  existsSync,
+  openSync,
+  readFileSync,
+  writeSync,
+} from 'fs';
 import { clamp, validatePathWithinRoot } from '../utils';
 import { tmpdir } from 'os';
 import { join } from 'path';
@@ -186,14 +193,36 @@ function numberSourceLines(slice: string, firstLineNumber: number): string {
 /**
  * Mark a Claude session as having consulted MCP tools.
  * This enables Grep/Glob/Bash commands that would otherwise be blocked.
+ *
+ * Why the explicit openSync + O_NOFOLLOW dance instead of plain writeFileSync:
+ * tmpdir() is world-writable on Linux (mode 1777), so on a shared multi-user
+ * machine any other local user can pre-create `codegraph-consulted-<hash>` as
+ * a symlink pointing at a file the victim owns. The old `writeFileSync` would
+ * happily follow that link and overwrite the target's contents with the ISO
+ * timestamp string (CWE-59). The session-id hash provides the predictability
+ * gate, but it's defense-in-depth: if a session id ever surfaces in logs,
+ * argv, or telemetry the attack becomes trivial, and the right fix is to not
+ * follow links from /tmp paths in the first place.
  */
 function markSessionConsulted(sessionId: string): void {
   try {
     const hash = createHash('md5').update(sessionId).digest('hex').slice(0, 16);
     const markerPath = join(tmpdir(), `codegraph-consulted-${hash}`);
-    writeFileSync(markerPath, new Date().toISOString(), 'utf8');
+    // O_NOFOLLOW makes openSync throw ELOOP if markerPath is already a symlink.
+    // O_CREAT + O_TRUNC keep the original "create-or-overwrite" semantics, and
+    // mode 0o600 prevents readback by other local users (the marker payload is
+    // benign, but narrowing the exposure costs nothing).
+    const flags = fsConstants.O_WRONLY | fsConstants.O_CREAT | fsConstants.O_TRUNC | fsConstants.O_NOFOLLOW;
+    const fd = openSync(markerPath, flags, 0o600);
+    try {
+      writeSync(fd, new Date().toISOString());
+    } finally {
+      closeSync(fd);
+    }
   } catch {
-    // Silently fail - don't break MCP on marker write failure
+    // Silently fail - don't break MCP on marker write failure. ELOOP from a
+    // planted symlink lands here too, which is the intended behavior: refuse
+    // to write rather than overwrite an attacker-chosen target.
   }
 }
 
