From 434bcd4117f18bdb0d3df01df7b9fcae0bc6cf5b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 22:37:31 -0700 Subject: [PATCH 1/3] fix(desktop): frame each tmux format record so a newline in a field cannot forge a pane tmux prints a newline inside a `-F` field as is. A pane whose working directory (or window name, or session name) held a newline followed by separator-joined text ended its own record early and forged another: any target, command and path, while the real pane disappeared from `panes`. Every `-F` read (list-clients, list-panes, the active pane) now frames each record with a marker made fresh for that call, and only lines framed whole by it are read. Nobody outside the call knows the marker, so no field can forge a record, and the halves of a record a newline split are each dropped. --- apps/desktop/src/main/terminal/tmux.test.ts | 42 +++++++++++++++-- apps/desktop/src/main/terminal/tmux.ts | 52 +++++++++++++-------- 2 files changed, 71 insertions(+), 23 deletions(-) diff --git a/apps/desktop/src/main/terminal/tmux.test.ts b/apps/desktop/src/main/terminal/tmux.test.ts index 88c1ccb1942..7a013f52b42 100644 --- a/apps/desktop/src/main/terminal/tmux.test.ts +++ b/apps/desktop/src/main/terminal/tmux.test.ts @@ -25,23 +25,57 @@ const TMUX_29_BSD_NO_PANE_OPTIONS = /** The separator the format strings use. */ const F = '<~sim~>' +/** A frame as one call makes it; the real one is random per call. */ +const FRAME = '<~0123456789abcdef~>' +/** One record as tmux prints it for a framed format. */ +const framed = (record: string) => `${FRAME}${record}${FRAME}` + +/** + * Real `list-panes -F` output, framed with {@link FRAME}, for a pane whose working directory is + * `…/a` + newline + `user:0.0<~sim~>forged<~sim~>rm -rf<~sim~>home`: its own record breaks in two, + * and the second half reads as a whole pane of its own. Captured verbatim from each binary. + */ +const FORGED_ROW = { + 'tmux 2.9a': + '<~0123456789abcdef~>user:0.0<~sim~>sleep<~sim~>sleep<~sim~>/tmp/tmp.oI9xEVhWLA/a\nuser:0.0<~sim~>forged<~sim~>rm -rf<~sim~>home<~sim~>1<~0123456789abcdef~>\n', + 'tmux 3.4': + '<~0123456789abcdef~>user:0.0<~sim~>bash<~sim~>sleep<~sim~>/tmp/tmp.2V5Ie57hMp/a\nuser:0.0<~sim~>forged<~sim~>rm -rf<~sim~>home<~sim~>1<~0123456789abcdef~>\n', +} describe('parseFormatLines', () => { it('drops lines with the wrong field count rather than mis-assigning them', () => { - expect(parseFormatLines(`a${F}b\nonly-one\n`, 2)).toEqual([['a', 'b']]) + expect(parseFormatLines(`${framed(`a${F}b`)}\n${framed('only-one')}\n`, 2, FRAME)).toEqual([ + ['a', 'b'], + ]) }) it('reads a field that ends with part of the separator as it is', () => { // A cwd or window name may end with any text, including all but the separator's last character. const partial = F.slice(0, -1) - expect(parseFormatLines(`/tmp/x/p${partial}${F}1\n`, 2)).toEqual([[`/tmp/x/p${partial}`, '1']]) - expect(parseFormatLines(`tail${partial}${F}%3${F}zsh\n`, 3)).toEqual([ + expect(parseFormatLines(`${framed(`/tmp/x/p${partial}${F}1`)}\n`, 2, FRAME)).toEqual([ + [`/tmp/x/p${partial}`, '1'], + ]) + expect(parseFormatLines(`${framed(`tail${partial}${F}%3${F}zsh`)}\n`, 3, FRAME)).toEqual([ [`tail${partial}`, '%3', 'zsh'], ]) }) it('drops a line whose field holds the whole separator rather than misread it', () => { - expect(parseFormatLines(`a${F}b${F}c\n`, 2)).toEqual([]) + expect(parseFormatLines(`${framed(`a${F}b${F}c`)}\n`, 2, FRAME)).toEqual([]) + }) + + it.each(Object.entries(FORGED_ROW))( + 'never reads a row a directory name forges with a newline (%s)', + (_version, stdout) => { + expect(parseFormatLines(stdout, 5, FRAME)).toEqual([]) + } + ) + + it('reads only records framed by this call', () => { + const other = '<~fedcba9876543210~>' + expect(parseFormatLines(`${other}a${F}b${other}\n${framed(`c${F}d`)}\n`, 2, FRAME)).toEqual([ + ['c', 'd'], + ]) }) }) diff --git a/apps/desktop/src/main/terminal/tmux.ts b/apps/desktop/src/main/terminal/tmux.ts index 6fb40d82ba3..24106195bb9 100644 --- a/apps/desktop/src/main/terminal/tmux.ts +++ b/apps/desktop/src/main/terminal/tmux.ts @@ -17,6 +17,7 @@ * of the shell that launched it. */ import { spawn } from 'node:child_process' +import { randomBytes } from 'node:crypto' import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' @@ -41,12 +42,24 @@ const RUN_POLL_INTERVAL_MS = 250 /** * Field separator for `-F` output. Printable on purpose: tmux 3.4 and 3.5 print a control * character as its octal escape, so a control-character separator arrived as the text `\037` and - * no line split. No tmux escapes these characters. No proper prefix of the separator is also a - * suffix of it, so it can only be found where it was written or wholly inside a field: a field - * holding it changes the line's field count, and that line is dropped rather than misread. + * no line split. No tmux escapes these characters. Fields are untrusted text (a directory or + * window name can hold the separator, or a newline), so records are also framed per call: see + * {@link framedFormat}. */ const FIELD = '<~sim~>' +/** + * A `-F` format for these fields whose every record starts and ends with a marker made fresh for + * this one call. tmux prints a newline inside a field as is, so a directory named + * `a\nuser:0.0<~sim~>…` would otherwise end one record early and forge another. Nobody outside + * this call knows the marker, so no field can forge a framed record, and the halves of a record a + * newline split are each unframed and dropped. + */ +function framedFormat(fields: string[]): { format: string; frame: string } { + const frame = `<~${randomBytes(8).toString('hex')}~>` + return { format: `${frame}${fields.join(FIELD)}${frame}`, frame } +} + export interface TmuxCommandResult { ok: boolean stdout: string @@ -129,18 +142,20 @@ export function runTmux(args: string[], env: NodeJS.ProcessEnv): Promise line.trimEnd()) - .filter((line) => line.length > 0) - .map((line) => line.split(FIELD)) + .filter( + (line) => line.length >= frame.length * 2 && line.startsWith(frame) && line.endsWith(frame) + ) + .map((line) => line.slice(frame.length, line.length - frame.length).split(FIELD)) .filter((parts) => parts.length === fields) } @@ -206,11 +221,11 @@ export async function resolveAttachment( shellPid: number, env: NodeJS.ProcessEnv ): Promise { - const format = ['#{client_pid}', '#{client_tty}', '#{client_session}'].join(FIELD) + const { format, frame } = framedFormat(['#{client_pid}', '#{client_tty}', '#{client_session}']) const listed = await runTmux(['list-clients', '-F', format], env) if (!listed.ok) return null - const clients = parseFormatLines(listed.stdout, 3) + const clients = parseFormatLines(listed.stdout, 3, frame) if (clients.length === 0) return null const parents = await listProcessParents() @@ -226,28 +241,27 @@ export async function resolveAttachment( /** The active pane of a session, as a target usable by every other call. */ export async function activePane(session: string, env: NodeJS.ProcessEnv): Promise { - const result = await runTmux( - ['display-message', '-p', '-t', session, '#{session_name}:#{window_index}.#{pane_index}'], - env - ) - const target = result.stdout.trim() - return result.ok && target ? target : null + const { format, frame } = framedFormat(['#{session_name}:#{window_index}.#{pane_index}']) + const result = await runTmux(['display-message', '-p', '-t', session, format], env) + if (!result.ok) return null + const [target] = parseFormatLines(result.stdout, 1, frame)[0] ?? [] + return target || null } export async function listPanes( session: string, env: NodeJS.ProcessEnv ): Promise { - const format = [ + const { format, frame } = framedFormat([ '#{session_name}:#{window_index}.#{pane_index}', '#{window_name}', '#{pane_current_command}', '#{pane_current_path}', '#{pane_active}', - ].join(FIELD) + ]) const result = await runTmux(['list-panes', '-s', '-t', session, '-F', format], env) if (!result.ok) return [] - return parseFormatLines(result.stdout, 5).map( + return parseFormatLines(result.stdout, 5, frame).map( ([target, windowName, command, cwd, active]): TerminalPaneState => ({ target, windowName, From b1fed2ede39da7b42a3a1a249eca0af8b89b4eea Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 22:49:50 -0700 Subject: [PATCH 2/3] fix(desktop): make the tmux format frame with the shared random helper --- apps/desktop/src/main/terminal/tmux.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src/main/terminal/tmux.ts b/apps/desktop/src/main/terminal/tmux.ts index 24106195bb9..3da31407fe7 100644 --- a/apps/desktop/src/main/terminal/tmux.ts +++ b/apps/desktop/src/main/terminal/tmux.ts @@ -17,7 +17,6 @@ * of the shell that launched it. */ import { spawn } from 'node:child_process' -import { randomBytes } from 'node:crypto' import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { dirname, join } from 'node:path' @@ -26,6 +25,7 @@ import type { TerminalPaneState } from '@sim/terminal-protocol' import { getErrorMessage } from '@sim/utils/errors' import { sleep } from '@sim/utils/helpers' import { generateId } from '@sim/utils/id' +import { generateRandomHex } from '@sim/utils/random' const logger = createLogger('DesktopTmux') @@ -56,7 +56,7 @@ const FIELD = '<~sim~>' * newline split are each unframed and dropped. */ function framedFormat(fields: string[]): { format: string; frame: string } { - const frame = `<~${randomBytes(8).toString('hex')}~>` + const frame = `<~${generateRandomHex(16)}~>` return { format: `${frame}${fields.join(FIELD)}${frame}`, frame } } From da5007aa1a1987b0eb362a924bf8e710e45c4915 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 23:21:51 -0700 Subject: [PATCH 3/3] fix(desktop): neutralise newlines at the source in tmux fields others can set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-call frame is not a secret: on macOS `ps` shows any process's arguments, and on Linux `/proc//cmdline` is world-readable, so someone who owns the directory a pane sits in can read the frame and rename the directory before tmux reads it. tmux now replaces each newline with `` itself (`#{s/\n//:…}`) in the fields someone other than the user can set: the pane's directory, its command, and the window title a program sets. A record is then one line, and the frame stays as a second line of defence. tmux output is also decoded as a stream, so a character split across two chunks arrives whole. --- apps/desktop/src/main/terminal/tmux.test.ts | 101 ++++++++++++++++++++ apps/desktop/src/main/terminal/tmux.ts | 37 ++++--- 2 files changed, 126 insertions(+), 12 deletions(-) diff --git a/apps/desktop/src/main/terminal/tmux.test.ts b/apps/desktop/src/main/terminal/tmux.test.ts index 7a013f52b42..b989428d007 100644 --- a/apps/desktop/src/main/terminal/tmux.test.ts +++ b/apps/desktop/src/main/terminal/tmux.test.ts @@ -7,6 +7,7 @@ import { awaitRun, closeRunPane, isDescendantOf, + listPanes, parseFormatLines, pollRun, resolveAttachment, @@ -42,6 +43,17 @@ const FORGED_ROW = { '<~0123456789abcdef~>user:0.0<~sim~>bash<~sim~>sleep<~sim~>/tmp/tmp.2V5Ie57hMp/a\nuser:0.0<~sim~>forged<~sim~>rm -rf<~sim~>home<~sim~>1<~0123456789abcdef~>\n', } +/** + * The same pane, captured verbatim from each binary with newlines neutralised in the fields others + * can set (`#{s///:…}`): one line, holding the would-be forged row as plain text. + */ +const NEUTRALISED_ROW = { + 'tmux 2.9a': + '<~0123456789abcdef~>user:0.0<~sim~>sleep<~sim~>sleep<~sim~>/tmp/tmp.CXoBFxxZAf/auser:0.0<~sim~>forged<~sim~>rm -rf<~sim~>home<~sim~>1<~0123456789abcdef~>\n', + 'tmux 3.4': + '<~0123456789abcdef~>user:0.0<~sim~>sleep<~sim~>sleep<~sim~>/tmp/tmp.Wz6L8cjmce/auser:0.0<~sim~>forged<~sim~>rm -rf<~sim~>home<~sim~>1<~0123456789abcdef~>\n', +} + describe('parseFormatLines', () => { it('drops lines with the wrong field count rather than mis-assigning them', () => { expect(parseFormatLines(`${framed(`a${F}b`)}\n${framed('only-one')}\n`, 2, FRAME)).toEqual([ @@ -71,6 +83,23 @@ describe('parseFormatLines', () => { } ) + it('drops a line framed at one end only, whatever its field count', () => { + const fields = `user:0.0${F}x${F}y${F}z${F}1` + // As long as a frame, so a check of one end alone would cut it off and find five fields. + const pad = 'J'.repeat(FRAME.length) + expect(parseFormatLines(`${pad}${fields}${FRAME}\n`, 5, FRAME)).toEqual([]) + expect(parseFormatLines(`${FRAME}${fields}${pad}\n`, 5, FRAME)).toEqual([]) + }) + + it.each(Object.entries(NEUTRALISED_ROW))( + 'reads the forging pane as one line once newlines are neutralised (%s)', + (_version, stdout) => { + // One line: no forged row. The separator text in its path then drops it whole. + expect(stdout.trimEnd().split('\n')).toHaveLength(1) + expect(parseFormatLines(stdout, 5, FRAME)).toEqual([]) + } + ) + it('reads only records framed by this call', () => { const other = '<~fedcba9876543210~>' expect(parseFormatLines(`${other}a${F}b${other}\n${framed(`c${F}d`)}\n`, 2, FRAME)).toEqual([ @@ -162,6 +191,10 @@ interface FakeTmuxState { fail?: Record /** Attached clients, as `list-clients` reports them. */ clients?: Array<{ pid: string; tty: string; session: string }> + /** A session's panes as `list-panes` reports them, with fields others can set. */ + listed?: Array<{ windowName: string; command: string; cwd: string }> + /** Every `-F` format the fake was asked for, in order. */ + formats?: string[] /** Commands the fake holds until the file named here exists, like a busy tmux server. */ hold?: Record /** Commands the fake is holding right now. */ @@ -227,6 +260,31 @@ switch (args[0]) { process.stdout.write(value + '\\n') break } + case 'list-panes': { + // Formats as tmux evaluates them: \`#{s/pattern/replacement/:field}\` substitutes, and every + // other field prints as is, a newline included. + const format = args[args.indexOf('-F') + 1] + state.formats = [...(state.formats ?? []), format] + save() + ;(state.listed ?? []).forEach((pane, index) => { + const fields = { + session_name: 'work', + window_index: '0', + pane_index: String(index), + window_name: pane.windowName, + pane_current_command: pane.command, + pane_current_path: pane.cwd, + pane_active: index === 0 ? '1' : '0', + } + const line = format + .replace(/#\\{s\\/([^/]*)\\/([^/]*)\\/:([a-z_]+)\\}/g, (_, from, to, name) => + fields[name].split(from).join(to) + ) + .replace(/#\\{([a-z_]+)\\}/g, (_, name) => fields[name]) + process.stdout.write(line + '\\n') + }) + break + } case 'list-clients': { const format = args[args.indexOf('-F') + 1] for (const client of state.clients ?? []) { @@ -285,6 +343,49 @@ function fakeTmux(options: { exec?: boolean } = {}) { } } +describe("listing a session's panes", () => { + const dirs: string[] = [] + + afterEach(() => { + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }) + }) + + it('lists a pane whose directory, command or title holds a newline, as one pane', async () => { + const tmux = fakeTmux() + dirs.push(tmux.dir) + tmux.write({ + ...tmux.read(), + listed: [ + { windowName: 'build\nlogs', command: 'make', cwd: '/tmp/a\nuser:0.0' }, + { windowName: 'zsh', command: 'zsh', cwd: '/tmp' }, + ], + }) + + expect(await listPanes('work', tmux.env)).toEqual([ + { + target: 'work:0.0', + windowName: 'buildlogs', + command: 'make', + cwd: '/tmp/auser:0.0', + active: true, + }, + { target: 'work:0.1', windowName: 'zsh', command: 'zsh', cwd: '/tmp', active: false }, + ]) + }) + + it('frames each call with a marker of its own', async () => { + const tmux = fakeTmux() + dirs.push(tmux.dir) + await listPanes('work', tmux.env) + await listPanes('work', tmux.env) + + const frames = (tmux.read().formats ?? []).map((format) => /^<~([0-9a-f]+)~>/.exec(format)?.[1]) + expect(frames).toHaveLength(2) + expect(frames[0]).toMatch(/^[0-9a-f]{16}$/) + expect(frames[1]).not.toBe(frames[0]) + }) +}) + describe('finding the tmux session a shell runs', () => { const dirs: string[] = [] diff --git a/apps/desktop/src/main/terminal/tmux.ts b/apps/desktop/src/main/terminal/tmux.ts index 3da31407fe7..265ac34dcdd 100644 --- a/apps/desktop/src/main/terminal/tmux.ts +++ b/apps/desktop/src/main/terminal/tmux.ts @@ -49,11 +49,21 @@ const RUN_POLL_INTERVAL_MS = 250 const FIELD = '<~sim~>' /** - * A `-F` format for these fields whose every record starts and ends with a marker made fresh for - * this one call. tmux prints a newline inside a field as is, so a directory named - * `a\nuser:0.0<~sim~>…` would otherwise end one record early and forge another. Nobody outside - * this call knows the marker, so no field can forge a framed record, and the halves of a record a - * newline split are each unframed and dropped. + * A field someone other than the user can set: a directory name (`pane_current_path`), a program's + * name (`pane_current_command`), or a window title a program sets (`window_name`). tmux prints a + * newline inside a field as is, and a newline would end the record early and let the rest read as + * a record of its own, so tmux replaces each newline with `` before printing. + */ +function untrusted(field: string): string { + return `#{s/\n//:${field}}` +} + +/** + * A `-F` format whose every record starts and ends with a marker made fresh for this call; only + * lines framed whole by it are read, so a line that is not a whole record is dropped rather than + * misread. The marker is a second line of defence, not a secret (another user can read a process's + * arguments on many systems), which is why newlines are neutralised at the source too + * ({@link untrusted}). */ function framedFormat(fields: string[]): { format: string; frame: string } { const frame = `<~${generateRandomHex(16)}~>` @@ -125,11 +135,14 @@ export function runTmux(args: string[], env: NodeJS.ProcessEnv): Promise { - stdout += chunk.toString() + // Decoded as streams, so a character split across two chunks arrives whole. + child.stdout?.setEncoding('utf8') + child.stderr?.setEncoding('utf8') + child.stdout?.on('data', (chunk: string) => { + stdout += chunk }) - child.stderr?.on('data', (chunk: Buffer) => { - stderr += chunk.toString() + child.stderr?.on('data', (chunk: string) => { + stderr += chunk }) child.on('error', (error) => { if ((error as NodeJS.ErrnoException).code === 'ENOENT') tmuxBinaryMissing = true @@ -254,9 +267,9 @@ export async function listPanes( ): Promise { const { format, frame } = framedFormat([ '#{session_name}:#{window_index}.#{pane_index}', - '#{window_name}', - '#{pane_current_command}', - '#{pane_current_path}', + untrusted('window_name'), + untrusted('pane_current_command'), + untrusted('pane_current_path'), '#{pane_active}', ]) const result = await runTmux(['list-panes', '-s', '-t', session, '-F', format], env)