diff --git a/src/host-core.test.ts b/src/host-core.test.ts index 2bb72d4..043b6b1 100644 --- a/src/host-core.test.ts +++ b/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/ 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//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); diff --git a/src/session-manager.ts b/src/session-manager.ts index 96bca96..6b00655 100644 --- a/src/session-manager.ts +++ b/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//` 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> | 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') { - // The name field is attacker-controlled: chat platforms with E2E - // attachment encryption (WhatsApp, Matrix) cannot sanitize filename - // server-side, and other adapters pass att.name through raw. Without - // this guard, `path.join(inboxDir, '../../...')` writes anywhere the - // host process has fs permission — see Signal Desktop's Nov 2025 - // attachment-fileName advisory for the same archetype. - const rawName = deriveAttachmentName(att); - const filename = isSafeAttachmentName(rawName) ? rawName : `attachment-${Date.now()}`; - 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 }); + if (typeof att.data !== 'string') continue; + + const rawName = deriveAttachmentName(att); + const filename = isSafeAttachmentName(rawName) ? rawName : `attachment-${Date.now()}`; + 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); + + // 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)) { - files.push({ filename, data: fs.readFileSync(filePath) }); - } else { + try { + const stat = fs.lstatSync(filePath); + 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 }); }