Skip to content

Commit da5007a

Browse files
committed
fix(desktop): neutralise newlines at the source in tmux fields others can set
The per-call frame is not a secret: on macOS `ps` shows any process's arguments, and on Linux `/proc/<pid>/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 `<NL>` itself (`#{s/\n/<NL>/:…}`) 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.
1 parent b1fed2e commit da5007a

2 files changed

Lines changed: 126 additions & 12 deletions

File tree

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

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
awaitRun,
88
closeRunPane,
99
isDescendantOf,
10+
listPanes,
1011
parseFormatLines,
1112
pollRun,
1213
resolveAttachment,
@@ -42,6 +43,17 @@ const FORGED_ROW = {
4243
'<~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',
4344
}
4445

46+
/**
47+
* The same pane, captured verbatim from each binary with newlines neutralised in the fields others
48+
* can set (`#{s/<newline>/<NL>/:…}`): one line, holding the would-be forged row as plain text.
49+
*/
50+
const NEUTRALISED_ROW = {
51+
'tmux 2.9a':
52+
'<~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',
53+
'tmux 3.4':
54+
'<~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',
55+
}
56+
4557
describe('parseFormatLines', () => {
4658
it('drops lines with the wrong field count rather than mis-assigning them', () => {
4759
expect(parseFormatLines(`${framed(`a${F}b`)}\n${framed('only-one')}\n`, 2, FRAME)).toEqual([
@@ -71,6 +83,23 @@ describe('parseFormatLines', () => {
7183
}
7284
)
7385

86+
it('drops a line framed at one end only, whatever its field count', () => {
87+
const fields = `user:0.0${F}x${F}y${F}z${F}1`
88+
// As long as a frame, so a check of one end alone would cut it off and find five fields.
89+
const pad = 'J'.repeat(FRAME.length)
90+
expect(parseFormatLines(`${pad}${fields}${FRAME}\n`, 5, FRAME)).toEqual([])
91+
expect(parseFormatLines(`${FRAME}${fields}${pad}\n`, 5, FRAME)).toEqual([])
92+
})
93+
94+
it.each(Object.entries(NEUTRALISED_ROW))(
95+
'reads the forging pane as one line once newlines are neutralised (%s)',
96+
(_version, stdout) => {
97+
// One line: no forged row. The separator text in its path then drops it whole.
98+
expect(stdout.trimEnd().split('\n')).toHaveLength(1)
99+
expect(parseFormatLines(stdout, 5, FRAME)).toEqual([])
100+
}
101+
)
102+
74103
it('reads only records framed by this call', () => {
75104
const other = '<~fedcba9876543210~>'
76105
expect(parseFormatLines(`${other}a${F}b${other}\n${framed(`c${F}d`)}\n`, 2, FRAME)).toEqual([
@@ -162,6 +191,10 @@ interface FakeTmuxState {
162191
fail?: Record<string, string>
163192
/** Attached clients, as `list-clients` reports them. */
164193
clients?: Array<{ pid: string; tty: string; session: string }>
194+
/** A session's panes as `list-panes` reports them, with fields others can set. */
195+
listed?: Array<{ windowName: string; command: string; cwd: string }>
196+
/** Every `-F` format the fake was asked for, in order. */
197+
formats?: string[]
165198
/** Commands the fake holds until the file named here exists, like a busy tmux server. */
166199
hold?: Record<string, string>
167200
/** Commands the fake is holding right now. */
@@ -227,6 +260,31 @@ switch (args[0]) {
227260
process.stdout.write(value + '\\n')
228261
break
229262
}
263+
case 'list-panes': {
264+
// Formats as tmux evaluates them: \`#{s/pattern/replacement/:field}\` substitutes, and every
265+
// other field prints as is, a newline included.
266+
const format = args[args.indexOf('-F') + 1]
267+
state.formats = [...(state.formats ?? []), format]
268+
save()
269+
;(state.listed ?? []).forEach((pane, index) => {
270+
const fields = {
271+
session_name: 'work',
272+
window_index: '0',
273+
pane_index: String(index),
274+
window_name: pane.windowName,
275+
pane_current_command: pane.command,
276+
pane_current_path: pane.cwd,
277+
pane_active: index === 0 ? '1' : '0',
278+
}
279+
const line = format
280+
.replace(/#\\{s\\/([^/]*)\\/([^/]*)\\/:([a-z_]+)\\}/g, (_, from, to, name) =>
281+
fields[name].split(from).join(to)
282+
)
283+
.replace(/#\\{([a-z_]+)\\}/g, (_, name) => fields[name])
284+
process.stdout.write(line + '\\n')
285+
})
286+
break
287+
}
230288
case 'list-clients': {
231289
const format = args[args.indexOf('-F') + 1]
232290
for (const client of state.clients ?? []) {
@@ -285,6 +343,49 @@ function fakeTmux(options: { exec?: boolean } = {}) {
285343
}
286344
}
287345

346+
describe("listing a session's panes", () => {
347+
const dirs: string[] = []
348+
349+
afterEach(() => {
350+
for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true })
351+
})
352+
353+
it('lists a pane whose directory, command or title holds a newline, as one pane', async () => {
354+
const tmux = fakeTmux()
355+
dirs.push(tmux.dir)
356+
tmux.write({
357+
...tmux.read(),
358+
listed: [
359+
{ windowName: 'build\nlogs', command: 'make', cwd: '/tmp/a\nuser:0.0' },
360+
{ windowName: 'zsh', command: 'zsh', cwd: '/tmp' },
361+
],
362+
})
363+
364+
expect(await listPanes('work', tmux.env)).toEqual([
365+
{
366+
target: 'work:0.0',
367+
windowName: 'build<NL>logs',
368+
command: 'make',
369+
cwd: '/tmp/a<NL>user:0.0',
370+
active: true,
371+
},
372+
{ target: 'work:0.1', windowName: 'zsh', command: 'zsh', cwd: '/tmp', active: false },
373+
])
374+
})
375+
376+
it('frames each call with a marker of its own', async () => {
377+
const tmux = fakeTmux()
378+
dirs.push(tmux.dir)
379+
await listPanes('work', tmux.env)
380+
await listPanes('work', tmux.env)
381+
382+
const frames = (tmux.read().formats ?? []).map((format) => /^<~([0-9a-f]+)~>/.exec(format)?.[1])
383+
expect(frames).toHaveLength(2)
384+
expect(frames[0]).toMatch(/^[0-9a-f]{16}$/)
385+
expect(frames[1]).not.toBe(frames[0])
386+
})
387+
})
388+
288389
describe('finding the tmux session a shell runs', () => {
289390
const dirs: string[] = []
290391

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

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,21 @@ const RUN_POLL_INTERVAL_MS = 250
4949
const FIELD = '<~sim~>'
5050

5151
/**
52-
* A `-F` format for these fields whose every record starts and ends with a marker made fresh for
53-
* this one call. tmux prints a newline inside a field as is, so a directory named
54-
* `a\nuser:0.0<~sim~>…` would otherwise end one record early and forge another. Nobody outside
55-
* this call knows the marker, so no field can forge a framed record, and the halves of a record a
56-
* newline split are each unframed and dropped.
52+
* A field someone other than the user can set: a directory name (`pane_current_path`), a program's
53+
* name (`pane_current_command`), or a window title a program sets (`window_name`). tmux prints a
54+
* newline inside a field as is, and a newline would end the record early and let the rest read as
55+
* a record of its own, so tmux replaces each newline with `<NL>` before printing.
56+
*/
57+
function untrusted(field: string): string {
58+
return `#{s/\n/<NL>/:${field}}`
59+
}
60+
61+
/**
62+
* A `-F` format whose every record starts and ends with a marker made fresh for this call; only
63+
* lines framed whole by it are read, so a line that is not a whole record is dropped rather than
64+
* misread. The marker is a second line of defence, not a secret (another user can read a process's
65+
* arguments on many systems), which is why newlines are neutralised at the source too
66+
* ({@link untrusted}).
5767
*/
5868
function framedFormat(fields: string[]): { format: string; frame: string } {
5969
const frame = `<~${generateRandomHex(16)}~>`
@@ -125,11 +135,14 @@ export function runTmux(args: string[], env: NodeJS.ProcessEnv): Promise<TmuxCom
125135
finish({ ok: false, stdout, stderr: 'tmux did not respond' })
126136
}, TMUX_TIMEOUT_MS)
127137

128-
child.stdout?.on('data', (chunk: Buffer) => {
129-
stdout += chunk.toString()
138+
// Decoded as streams, so a character split across two chunks arrives whole.
139+
child.stdout?.setEncoding('utf8')
140+
child.stderr?.setEncoding('utf8')
141+
child.stdout?.on('data', (chunk: string) => {
142+
stdout += chunk
130143
})
131-
child.stderr?.on('data', (chunk: Buffer) => {
132-
stderr += chunk.toString()
144+
child.stderr?.on('data', (chunk: string) => {
145+
stderr += chunk
133146
})
134147
child.on('error', (error) => {
135148
if ((error as NodeJS.ErrnoException).code === 'ENOENT') tmuxBinaryMissing = true
@@ -254,9 +267,9 @@ export async function listPanes(
254267
): Promise<TerminalPaneState[]> {
255268
const { format, frame } = framedFormat([
256269
'#{session_name}:#{window_index}.#{pane_index}',
257-
'#{window_name}',
258-
'#{pane_current_command}',
259-
'#{pane_current_path}',
270+
untrusted('window_name'),
271+
untrusted('pane_current_command'),
272+
untrusted('pane_current_path'),
260273
'#{pane_active}',
261274
])
262275
const result = await runTmux(['list-panes', '-s', '-t', session, '-F', format], env)

0 commit comments

Comments
 (0)