fix(desktop): frame each tmux format record so a newline in a field cannot forge a pane

apps/desktop/src/main/terminal/tmux.test.ts

@@ -7,6 +7,7 @@ import {
   awaitRun,
   closeRunPane,
   isDescendantOf,
+  listPanes,
   parseFormatLines,
   pollRun,
   resolveAttachment,
@@ -25,23 +26,85 @@ 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',
+}
+
+/**
+ * The same pane, captured verbatim from each binary with newlines neutralised in the fields others
+ * can set (`#{s/<newline>/<NL>/:…}`): 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/a<NL>user: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/a<NL>user: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('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([
+      ['c', 'd'],
+    ])
   })
 })
 
@@ -128,6 +191,10 @@ interface FakeTmuxState {
   fail?: Record<string, string>
   /** 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<string, string>
   /** Commands the fake is holding right now. */
@@ -193,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 ?? []) {
@@ -251,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: 'build<NL>logs',
+        command: 'make',
+        cwd: '/tmp/a<NL>user: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[] = []
 

apps/desktop/src/main/terminal/tmux.ts

@@ -25,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')
 
@@ -41,12 +42,34 @@ 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 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 `<NL>` before printing.
+ */
+function untrusted(field: string): string {
+  return `#{s/\n/<NL>/:${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)}~>`
+  return { format: `${frame}${fields.join(FIELD)}${frame}`, frame }
+}
+
 export interface TmuxCommandResult {
   ok: boolean
   stdout: string
@@ -112,11 +135,14 @@ export function runTmux(args: string[], env: NodeJS.ProcessEnv): Promise<TmuxCom
       finish({ ok: false, stdout, stderr: 'tmux did not respond' })
     }, TMUX_TIMEOUT_MS)
 
-    child.stdout?.on('data', (chunk: Buffer) => {
-      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
@@ -129,18 +155,20 @@ export function runTmux(args: string[], env: NodeJS.ProcessEnv): Promise<TmuxCom
 }
 
 /**
- * Parses `list-clients`/`list-panes` output into records.
+ * Parses `list-clients`/`list-panes` output into records: only lines framed whole by this call's
+ * marker, each with exactly the fields asked for.
  *
  * Split on a dedicated separator rather than whitespace: window names and
  * working directories contain spaces, and a path with a space would otherwise
  * shift every later field by one.
  */
-export function parseFormatLines(stdout: string, fields: number): string[][] {
+export function parseFormatLines(stdout: string, fields: number, frame: string): string[][] {
   return stdout
     .split('\n')
-    .map((line) => 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 +234,11 @@ export async function resolveAttachment(
   shellPid: number,
   env: NodeJS.ProcessEnv
 ): Promise<TmuxAttachment | null> {
-  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 +254,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<string | null> {
-  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<TerminalPaneState[]> {
-  const format = [
+  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}',
-  ].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,