Merge pull request #2001 from Hinotoi-agent/fix/outbox-path-confinement

[security] fix(container): prevent host file read/delete via container-controlled outbox paths

src/host-core.test.ts

@@ -23,6 +23,8 @@ import {
   sessionDir,
   inboundDbPath,
   outboundDbPath,
+  readOutboxFiles,
+  clearOutbox,
 } from './session-manager.js';
 import { getSession, findSession } from './db/sessions.js';
 import type { InboundEvent } from './channels/adapter.js';
@@ -108,6 +110,147 @@ describe('session manager', () => {
     outDb.close();
   });
 
+  it('should reject outbound attachment filenames that escape the message outbox', () => {
+    initSessionFolder('ag-1', 'sess-test');
+    const dir = sessionDir('ag-1', 'sess-test');
+    const msgOutbox = path.join(dir, 'outbox', 'msg-1');
+    fs.mkdirSync(msgOutbox, { recursive: true });
+
+    const outside = path.join(TEST_DIR, 'outside.txt');
+    fs.writeFileSync(outside, 'outside secret');
+
+    expect(readOutboxFiles('ag-1', 'sess-test', 'msg-1', ['../../../../../outside.txt'])).toBeUndefined();
+  });
+
+  it('should reject outbound attachment symlinks that escape the message outbox', () => {
+    initSessionFolder('ag-1', 'sess-test');
+    const dir = sessionDir('ag-1', 'sess-test');
+    const msgOutbox = path.join(dir, 'outbox', 'msg-1');
+    fs.mkdirSync(msgOutbox, { recursive: true });
+
+    const outside = path.join(TEST_DIR, 'outside.txt');
+    fs.writeFileSync(outside, 'outside secret');
+    fs.symlinkSync('../../../../../outside.txt', path.join(msgOutbox, 'safe-name.txt'));
+
+    expect(readOutboxFiles('ag-1', 'sess-test', 'msg-1', ['safe-name.txt'])).toBeUndefined();
+  });
+
+  it('should not recursively delete outside the outbox for unsafe message ids', () => {
+    initSessionFolder('ag-1', 'sess-test');
+    const victimDir = path.join(TEST_DIR, 'victim-dir');
+    fs.mkdirSync(victimDir, { recursive: true });
+    fs.writeFileSync(path.join(victimDir, 'keep.txt'), 'do not delete');
+
+    clearOutbox('ag-1', 'sess-test', '../../../../victim-dir');
+
+    expect(fs.existsSync(path.join(victimDir, 'keep.txt'))).toBe(true);
+  });
+
+  it('should still read and clear normal basename outbox files', () => {
+    initSessionFolder('ag-1', 'sess-test');
+    const dir = sessionDir('ag-1', 'sess-test');
+    const msgOutbox = path.join(dir, 'outbox', 'msg-1');
+    fs.mkdirSync(msgOutbox, { recursive: true });
+    fs.writeFileSync(path.join(msgOutbox, 'result.txt'), 'ok');
+
+    const files = readOutboxFiles('ag-1', 'sess-test', 'msg-1', ['result.txt']);
+    expect(files).toHaveLength(1);
+    expect(files?.[0]?.filename).toBe('result.txt');
+    expect(files?.[0]?.data.toString()).toBe('ok');
+
+    clearOutbox('ag-1', 'sess-test', 'msg-1');
+    expect(fs.existsSync(msgOutbox)).toBe(false);
+  });
+
+  it('should reject inbound attachment writes through a pre-placed symlinked inbox dir', () => {
+    initSessionFolder('ag-1', 'sess-test');
+    const { session } = resolveSession('ag-1', 'mg-1', null, 'shared');
+
+    // The container has /workspace write access, so it can pre create
+    // inbox/<msgId> as a symlink to escape.
+    const inboxRoot = path.join(sessionDir('ag-1', session.id), 'inbox');
+    fs.mkdirSync(inboxRoot, { recursive: true });
+    const evilTarget = path.join(TEST_DIR, 'evil-target');
+    fs.mkdirSync(evilTarget, { recursive: true });
+    fs.symlinkSync(evilTarget, path.join(inboxRoot, 'msg-evil'));
+
+    writeSessionMessage('ag-1', session.id, {
+      id: 'msg-evil',
+      kind: 'chat',
+      timestamp: now(),
+      content: JSON.stringify({
+        text: 'evil',
+        attachments: [{ name: 'photo.png', data: Buffer.from('PNGBYTES').toString('base64'), size: 8 }],
+      }),
+    });
+
+    expect(fs.existsSync(path.join(evilTarget, 'photo.png'))).toBe(false);
+  });
+
+  it('should refuse to follow a pre-existing symlink at the inbound attachment path', () => {
+    initSessionFolder('ag-1', 'sess-test');
+    const { session } = resolveSession('ag-1', 'mg-1', null, 'shared');
+
+    // The container pre creates inbox/<msgId>/photo.png as a symlink to a
+    // host file. Without the wx flag, writeFileSync would follow it.
+    const inboxDir = path.join(sessionDir('ag-1', session.id), 'inbox', 'msg-sym');
+    fs.mkdirSync(inboxDir, { recursive: true });
+    const outside = path.join(TEST_DIR, 'outside.txt');
+    fs.writeFileSync(outside, 'ORIGINAL');
+    fs.symlinkSync(outside, path.join(inboxDir, 'photo.png'));
+
+    writeSessionMessage('ag-1', session.id, {
+      id: 'msg-sym',
+      kind: 'chat',
+      timestamp: now(),
+      content: JSON.stringify({
+        text: 'sym',
+        attachments: [{ name: 'photo.png', data: Buffer.from('PNGBYTES').toString('base64'), size: 8 }],
+      }),
+    });
+
+    expect(fs.readFileSync(outside, 'utf-8')).toBe('ORIGINAL');
+  });
+
+  it('should reject inbound attachments when messageId is unsafe', () => {
+    initSessionFolder('ag-1', 'sess-test');
+    const { session } = resolveSession('ag-1', 'mg-1', null, 'shared');
+
+    writeSessionMessage('ag-1', session.id, {
+      id: '../../escape',
+      kind: 'chat',
+      timestamp: now(),
+      content: JSON.stringify({
+        text: 'msgid',
+        attachments: [{ name: 'photo.png', data: Buffer.from('PNGBYTES').toString('base64'), size: 8 }],
+      }),
+    });
+
+    const inboxRoot = path.join(sessionDir('ag-1', session.id), 'inbox');
+    if (fs.existsSync(inboxRoot)) {
+      expect(fs.readdirSync(inboxRoot)).toEqual([]);
+    }
+  });
+
+  it('should still save inbound attachments with safe basenames', () => {
+    initSessionFolder('ag-1', 'sess-test');
+    const { session } = resolveSession('ag-1', 'mg-1', null, 'shared');
+
+    writeSessionMessage('ag-1', session.id, {
+      id: 'msg-ok',
+      kind: 'chat',
+      timestamp: now(),
+      content: JSON.stringify({
+        text: 'ok',
+        attachments: [{ name: 'photo.png', data: Buffer.from('PNGBYTES').toString('base64'), size: 8 }],
+      }),
+    });
+
+    const expected = path.join(sessionDir('ag-1', session.id), 'inbox', 'msg-ok', 'photo.png');
+    expect(fs.existsSync(expected)).toBe(true);
+    expect(fs.readFileSync(expected, 'utf-8')).toBe('PNGBYTES');
+  });
+
   it('should resolve to existing session (shared mode)', () => {
     const { session: s1, created: c1 } = resolveSession('ag-1', 'mg-1', null, 'shared');
     expect(c1).toBe(true);

src/session-manager.ts

@@ -21,7 +21,6 @@ import { DATA_DIR } from './config.js';
 import { getMessagingGroup } from './db/messaging-groups.js';
 import {
   createSession,
-  findSession,
   findSessionByAgentGroup,
   findSessionForAgent,
   getSession,
@@ -38,6 +37,11 @@ import {
 import { log } from './log.js';
 import type { Session } from './types.js';
 
+function isPathInside(parent: string, child: string): boolean {
+  const relative = path.relative(parent, child);
+  return relative === '' || (!relative.startsWith('..') && !path.isAbsolute(relative));
+}
+
 /** Root directory for all session data. */
 export function sessionsBaseDir(): string {
   return path.join(DATA_DIR, 'v2-sessions');
@@ -234,6 +238,20 @@ export function writeSessionMessage(
 /**
  * If message content has attachments with base64 `data`, save them to
  * the session's inbox directory and replace with `localPath`.
+ *
+ * Both `messageId` and `att.name` originate in untrusted input. WhatsApp
+ * passes `msg.key.id` through raw (and that field is client generated, so a
+ * peer can craft it), and other adapters may follow. The session dir is
+ * mounted writable into the container, so a compromised agent can also
+ * pre-place a symlink at `inbox/<future msgId>/` and wait for a chat message
+ * with a matching id to redirect the host's write.
+ *
+ * Defenses, mirrored from the outbound side:
+ *   1. basename check on `messageId` and `filename`.
+ *   2. lstat of the inbox dir to refuse pre-placed symlinks.
+ *   3. realpath-based containment under the session inbox root.
+ *   4. `wx` flag on writeFileSync to refuse following a pre-existing symlink
+ *      at the target file path or overwriting any existing file.
  */
 function extractAttachmentFiles(
   agentGroupId: string,
@@ -251,34 +269,75 @@ function extractAttachmentFiles(
   const attachments = parsed.attachments as Array<Record<string, unknown>> | undefined;
   if (!Array.isArray(attachments)) return contentStr;
 
+  if (!isSafeAttachmentName(messageId)) {
+    log.warn('Rejecting unsafe inbound message id', { messageId });
+    return contentStr;
+  }
+
   let changed = false;
   for (const att of attachments) {
-    if (typeof att.data === 'string') {
+    if (typeof att.data !== 'string') continue;
-      // The name field is attacker-controlled: chat platforms with E2E
+
-      // attachment encryption (WhatsApp, Matrix) cannot sanitize filename
+    const rawName = deriveAttachmentName(att);
-      // server-side, and other adapters pass att.name through raw. Without
+    const filename = isSafeAttachmentName(rawName) ? rawName : `attachment-${Date.now()}`;
-      // this guard, `path.join(inboxDir, '../../...')` writes anywhere the
+    if (filename !== rawName) {
-      // host process has fs permission — see Signal Desktop's Nov 2025
+      log.warn('Refused unsafe attachment filename, would escape inbox', {
-      // attachment-fileName advisory for the same archetype.
+        messageId,
-      const rawName = deriveAttachmentName(att);
+        rawName,
-      const filename = isSafeAttachmentName(rawName) ? rawName : `attachment-${Date.now()}`;
+        replacement: filename,
-      if (filename !== rawName) {
+      });
-        log.warn('Refused unsafe attachment filename — would escape inbox', {
-          messageId,
-          rawName,
-          replacement: filename,
-        });
-      }
-      const inboxDir = path.join(sessionDir(agentGroupId, sessionId), 'inbox', messageId);
-      fs.mkdirSync(inboxDir, { recursive: true });
-      const filePath = path.join(inboxDir, filename);
-      fs.writeFileSync(filePath, Buffer.from(att.data as string, 'base64'));
-      att.name = filename;
-      att.localPath = `inbox/${messageId}/${filename}`;
-      delete att.data;
-      changed = true;
-      log.debug('Saved attachment to inbox', { messageId, filename, size: att.size });
     }
+
+    const inboxDir = path.join(sessionDir(agentGroupId, sessionId), 'inbox', messageId);
+
+    // Refuse to mkdir through a symlink that the container may have pre placed
+    // at inboxDir. With recursive:true, mkdirSync would silently no op on a
+    // pre existing symlink and the subsequent writeFileSync would follow it.
+    if (fs.existsSync(inboxDir)) {
+      const stat = fs.lstatSync(inboxDir);
+      if (stat.isSymbolicLink() || !stat.isDirectory()) {
+        log.warn('Rejecting unsafe inbox directory', { messageId, inboxDir });
+        continue;
+      }
+    }
+    fs.mkdirSync(inboxDir, { recursive: true });
+
+    let realInboxDir: string;
+    try {
+      realInboxDir = fs.realpathSync(inboxDir);
+    } catch (err) {
+      log.warn('Failed to resolve inbox directory', { messageId, err });
+      continue;
+    }
+    const inboxRoot = path.join(sessionDir(agentGroupId, sessionId), 'inbox');
+    if (!isPathInside(fs.realpathSync(inboxRoot), realInboxDir)) {
+      log.warn('Inbox directory escaped session inbox root', { messageId, inboxDir });
+      continue;
+    }
+
+    const filePath = path.join(inboxDir, filename);
+    try {
+      // wx = exclusive create. Refuses to follow a pre existing symlink or
+      // overwrite any existing file. The host expects to be the sole writer
+      // of these attachments.
+      fs.writeFileSync(filePath, Buffer.from(att.data as string, 'base64'), { flag: 'wx' });
+    } catch (err: unknown) {
+      const e = err as NodeJS.ErrnoException;
+      if (e.code === 'EEXIST') {
+        log.warn('Inbox attachment target already exists, refusing to overwrite', {
+          messageId,
+          filename,
+        });
+        continue;
+      }
+      throw err;
+    }
+
+    att.name = filename;
+    att.localPath = `inbox/${messageId}/${filename}`;
+    delete att.data;
+    changed = true;
+    log.debug('Saved attachment to inbox', { messageId, filename, size: att.size });
   }
 
   return changed ? JSON.stringify(parsed) : contentStr;
@@ -369,19 +428,48 @@ export function readOutboxFiles(
   messageId: string,
   filenames: string[],
 ): OutboundFile[] | undefined {
+  if (!isSafeAttachmentName(messageId)) {
+    log.warn('Rejecting unsafe outbox message id', { messageId });
+    return undefined;
+  }
+
   const outboxDir = path.join(sessionDir(agentGroupId, sessionId), 'outbox', messageId);
   if (!fs.existsSync(outboxDir)) return undefined;
+
+  let realOutboxDir: string;
+  try {
+    const stat = fs.lstatSync(outboxDir);
+    if (!stat.isDirectory() || stat.isSymbolicLink()) {
+      log.warn('Rejecting unsafe outbox directory', { messageId, outboxDir });
+      return undefined;
+    }
+    realOutboxDir = fs.realpathSync(outboxDir);
+  } catch (err) {
+    log.warn('Failed to inspect outbox directory', { messageId, err });
+    return undefined;
+  }
+
   const files: OutboundFile[] = [];
   for (const filename of filenames) {
-    // Reject any name that isn't a bare basename before touching the filesystem.
     if (!isSafeAttachmentName(filename)) {
-      log.warn('Refused unsafe outbox filename — would escape outbox', { messageId, filename });
+      log.warn('Refused unsafe outbox filename, would escape outbox', { messageId, filename });
       continue;
     }
+
     const filePath = path.join(outboxDir, filename);
-    if (fs.existsSync(filePath)) {
+    try {
-      files.push({ filename, data: fs.readFileSync(filePath) });
+      const stat = fs.lstatSync(filePath);
-    } else {
+      if (!stat.isFile() || stat.isSymbolicLink()) {
+        log.warn('Rejecting unsafe outbox file', { messageId, filename });
+        continue;
+      }
+      const realFilePath = fs.realpathSync(filePath);
+      if (!isPathInside(realOutboxDir, realFilePath)) {
+        log.warn('Rejecting outbox file outside message directory', { messageId, filename });
+        continue;
+      }
+      files.push({ filename, data: fs.readFileSync(realFilePath) });
+    } catch {
       log.warn('Outbox file not found', { messageId, filename });
     }
   }
@@ -395,10 +483,26 @@ export function readOutboxFiles(
  * thrown error would trigger the delivery retry path and deliver twice.
  */
 export function clearOutbox(agentGroupId: string, sessionId: string, messageId: string): void {
+  if (!isSafeAttachmentName(messageId)) {
+    log.warn('Rejecting unsafe outbox cleanup message id', { messageId });
+    return;
+  }
+
   const outboxDir = path.join(sessionDir(agentGroupId, sessionId), 'outbox', messageId);
   if (!fs.existsSync(outboxDir)) return;
   try {
-    fs.rmSync(outboxDir, { recursive: true, force: true });
+    const stat = fs.lstatSync(outboxDir);
+    if (!stat.isDirectory() || stat.isSymbolicLink()) {
+      log.warn('Rejecting unsafe outbox cleanup directory', { messageId, outboxDir });
+      return;
+    }
+    const realOutboxBase = fs.realpathSync(path.join(sessionDir(agentGroupId, sessionId), 'outbox'));
+    const realOutboxDir = fs.realpathSync(outboxDir);
+    if (!isPathInside(realOutboxBase, realOutboxDir)) {
+      log.warn('Rejecting outbox cleanup outside session outbox', { messageId, outboxDir });
+      return;
+    }
+    fs.rmSync(realOutboxDir, { recursive: true, force: true });
   } catch (err) {
     log.warn('Outbox cleanup failed (message already delivered)', { messageId, err });
   }