Fix path traversal in attachment handling on channel-inbound path
This commit is contained in:
23
src/attachment-safety.ts
Normal file
23
src/attachment-safety.ts
Normal file
@@ -0,0 +1,23 @@
|
|||||||
|
import path from 'path';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Is `name` safe to use as the last segment of a path inside an
|
||||||
|
* attachment-staging directory? Filenames originate from untrusted sources —
|
||||||
|
* channel messages from any chat participant, agent-to-agent forwards from
|
||||||
|
* a possibly-compromised peer agent — and land in `path.join(dir, name)`
|
||||||
|
* sinks on the host. Without this guard, a `..`-laden name escapes the
|
||||||
|
* inbox and writes anywhere the host process has filesystem permission.
|
||||||
|
*
|
||||||
|
* Rejects:
|
||||||
|
* - non-string / empty
|
||||||
|
* - `.` / `..` (traversal sentinels that path.basename returns as-is)
|
||||||
|
* - anything containing a path separator (`/` or `\`) or NUL
|
||||||
|
* - any value where `path.basename(name) !== name`, catching OS-specific
|
||||||
|
* separators and covering drives/prefixes on Windows runtimes
|
||||||
|
*/
|
||||||
|
export function isSafeAttachmentName(name: string): boolean {
|
||||||
|
if (typeof name !== 'string' || name.length === 0) return false;
|
||||||
|
if (name === '.' || name === '..') return false;
|
||||||
|
if (/[\\/\0]/.test(name)) return false;
|
||||||
|
return path.basename(name) === name;
|
||||||
|
}
|
||||||
@@ -173,6 +173,43 @@ describe('session manager', () => {
|
|||||||
|
|
||||||
expect(getSession(session.id)!.last_active).not.toBeNull();
|
expect(getSession(session.id)!.last_active).not.toBeNull();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should refuse path-traversal in attachment filenames', () => {
|
||||||
|
// Regression: attachment.name comes from untrusted senders (E2EE-protected
|
||||||
|
// chat platforms can't sanitize it server-side). Without the guard, a
|
||||||
|
// `../../../tmp/pwned` filename escapes the inbox dir and writes anywhere
|
||||||
|
// the host process can reach.
|
||||||
|
const { session } = resolveSession('ag-1', 'mg-1', null, 'shared');
|
||||||
|
const inboxBase = path.join(sessionDir('ag-1', session.id), 'inbox');
|
||||||
|
const escapeTarget = path.join('/tmp', 'nanoclaw-traversal-canary');
|
||||||
|
if (fs.existsSync(escapeTarget)) fs.rmSync(escapeTarget);
|
||||||
|
|
||||||
|
writeSessionMessage('ag-1', session.id, {
|
||||||
|
id: 'msg-attack',
|
||||||
|
kind: 'chat',
|
||||||
|
timestamp: now(),
|
||||||
|
content: JSON.stringify({
|
||||||
|
text: 'pwn',
|
||||||
|
attachments: [
|
||||||
|
{
|
||||||
|
type: 'document',
|
||||||
|
name: '../../../../../../../../tmp/nanoclaw-traversal-canary',
|
||||||
|
data: Buffer.from('owned').toString('base64'),
|
||||||
|
},
|
||||||
|
],
|
||||||
|
}),
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(fs.existsSync(escapeTarget)).toBe(false);
|
||||||
|
// The bytes should still land — under a synthesized safe name inside the
|
||||||
|
// inbox — so the agent doesn't lose data on a malicious filename.
|
||||||
|
const inboxDir = path.join(inboxBase, 'msg-attack');
|
||||||
|
expect(fs.existsSync(inboxDir)).toBe(true);
|
||||||
|
const written = fs.readdirSync(inboxDir);
|
||||||
|
expect(written).toHaveLength(1);
|
||||||
|
expect(written[0]).not.toContain('/');
|
||||||
|
expect(written[0]).not.toContain('..');
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('router', () => {
|
describe('router', () => {
|
||||||
|
|||||||
@@ -21,6 +21,7 @@
|
|||||||
import fs from 'fs';
|
import fs from 'fs';
|
||||||
import path from 'path';
|
import path from 'path';
|
||||||
|
|
||||||
|
import { isSafeAttachmentName } from '../../attachment-safety.js';
|
||||||
import { getAgentGroup } from '../../db/agent-groups.js';
|
import { getAgentGroup } from '../../db/agent-groups.js';
|
||||||
import { getSession } from '../../db/sessions.js';
|
import { getSession } from '../../db/sessions.js';
|
||||||
import { wakeContainer } from '../../container-runner.js';
|
import { wakeContainer } from '../../container-runner.js';
|
||||||
@@ -29,6 +30,8 @@ import { resolveSession, sessionDir, writeSessionMessage } from '../../session-m
|
|||||||
import type { Session } from '../../types.js';
|
import type { Session } from '../../types.js';
|
||||||
import { hasDestination } from './db/agent-destinations.js';
|
import { hasDestination } from './db/agent-destinations.js';
|
||||||
|
|
||||||
|
export { isSafeAttachmentName };
|
||||||
|
|
||||||
export interface ForwardedAttachment {
|
export interface ForwardedAttachment {
|
||||||
name: string;
|
name: string;
|
||||||
filename: string;
|
filename: string;
|
||||||
@@ -36,26 +39,6 @@ export interface ForwardedAttachment {
|
|||||||
localPath: string;
|
localPath: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* Is `name` safe to use as the last segment of a path inside the target
|
|
||||||
* agent's inbox directory? Filenames arrive in messages_out content from
|
|
||||||
* the source agent — under a multi-agent setup with heterogenous providers
|
|
||||||
* (or a compromised / hallucinating sub-agent) they can't be trusted.
|
|
||||||
*
|
|
||||||
* Rejects:
|
|
||||||
* - empty string
|
|
||||||
* - `.` / `..` (traversal sentinels that path.basename returns as-is)
|
|
||||||
* - anything containing a path separator (`/` or `\`) or NUL
|
|
||||||
* - any value where `path.basename(name) !== name`, catching OS-specific
|
|
||||||
* separators and covering drives/prefixes on Windows runtimes
|
|
||||||
*/
|
|
||||||
export function isSafeAttachmentName(name: string): boolean {
|
|
||||||
if (typeof name !== 'string' || name.length === 0) return false;
|
|
||||||
if (name === '.' || name === '..') return false;
|
|
||||||
if (/[\\/\0]/.test(name)) return false;
|
|
||||||
return path.basename(name) === name;
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Copy file attachments from the source agent's outbox into the target
|
* Copy file attachments from the source agent's outbox into the target
|
||||||
* agent's inbox. Returns attachments using the formatter's existing
|
* agent's inbox. Returns attachments using the formatter's existing
|
||||||
|
|||||||
@@ -289,7 +289,14 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
|
|||||||
log.warn('adapter.subscribe failed', { channelType: event.channelType, threadId: event.threadId, err });
|
log.warn('adapter.subscribe failed', { channelType: event.channelType, threadId: event.threadId, err });
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
} else if (agent.ignored_message_policy === 'accumulate') {
|
} else if (agent.ignored_message_policy === 'accumulate' && !(engages && (!accessOk || !scopeOk))) {
|
||||||
|
// Accumulate stores the message as silent context. We allow it when
|
||||||
|
// engagement simply didn't fire, but NOT when engagement fired and
|
||||||
|
// the access/scope gate refused — those refusals are security
|
||||||
|
// decisions about an untrusted sender, and silently storing their
|
||||||
|
// message (which also stages their attachments to disk via
|
||||||
|
// writeSessionMessage → extractAttachmentFiles) is exactly what the
|
||||||
|
// gate is meant to prevent.
|
||||||
await deliverToAgent(agent, agentGroup, mg, event, userId, adapter?.supportsThreads === true, false);
|
await deliverToAgent(agent, agentGroup, mg, event, userId, adapter?.supportsThreads === true, false);
|
||||||
accumulatedCount++;
|
accumulatedCount++;
|
||||||
} else {
|
} else {
|
||||||
|
|||||||
@@ -14,6 +14,7 @@ import type Database from 'better-sqlite3';
|
|||||||
import fs from 'fs';
|
import fs from 'fs';
|
||||||
import path from 'path';
|
import path from 'path';
|
||||||
|
|
||||||
|
import { isSafeAttachmentName } from './attachment-safety.js';
|
||||||
import type { OutboundFile } from './channels/adapter.js';
|
import type { OutboundFile } from './channels/adapter.js';
|
||||||
import { DATA_DIR } from './config.js';
|
import { DATA_DIR } from './config.js';
|
||||||
import { getMessagingGroup } from './db/messaging-groups.js';
|
import { getMessagingGroup } from './db/messaging-groups.js';
|
||||||
@@ -252,11 +253,26 @@ function extractAttachmentFiles(
|
|||||||
let changed = false;
|
let changed = false;
|
||||||
for (const att of attachments) {
|
for (const att of attachments) {
|
||||||
if (typeof att.data === 'string') {
|
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 = (att.name as string | undefined) ?? `attachment-${Date.now()}`;
|
||||||
|
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);
|
const inboxDir = path.join(sessionDir(agentGroupId, sessionId), 'inbox', messageId);
|
||||||
fs.mkdirSync(inboxDir, { recursive: true });
|
fs.mkdirSync(inboxDir, { recursive: true });
|
||||||
const filename = (att.name as string) || `attachment-${Date.now()}`;
|
|
||||||
const filePath = path.join(inboxDir, filename);
|
const filePath = path.join(inboxDir, filename);
|
||||||
fs.writeFileSync(filePath, Buffer.from(att.data as string, 'base64'));
|
fs.writeFileSync(filePath, Buffer.from(att.data as string, 'base64'));
|
||||||
|
att.name = filename;
|
||||||
att.localPath = `inbox/${messageId}/${filename}`;
|
att.localPath = `inbox/${messageId}/${filename}`;
|
||||||
delete att.data;
|
delete att.data;
|
||||||
changed = true;
|
changed = true;
|
||||||
|
|||||||
Reference in New Issue
Block a user