Skip to content

security: confine FILE_READ and FILE_LIST to user home directory#60

Closed
yoziv wants to merge 1 commit into
InbarR:mainfrom
yoziv:security/yoziv/confine-file-ipc
Closed

security: confine FILE_READ and FILE_LIST to user home directory#60
yoziv wants to merge 1 commit into
InbarR:mainfrom
yoziv:security/yoziv/confine-file-ipc

Conversation

@yoziv
Copy link
Copy Markdown
Contributor

@yoziv yoziv commented Apr 19, 2026

Summary

FILE_READ and FILE_LIST IPC handlers previously accepted any path from the renderer, allowing a compromised renderer to read any file or enumerate any directory on the system. This change restricts filesystem access to the user's home directory (and WSL UNC paths for WSL terminals).

Changes

  • IPC.FILE_LIST: Added path.resolve() + os.homedir() prefix check before fs.readdirSync. WSL paths (\\\\wsl.localhost\\) are allowed.
  • IPC.FILE_READ: Added path.resolve() + os.homedir() prefix check before fs.statSync. WSL paths (//wsl.localhost/) are allowed.

Both handlers return early (empty array / null) for paths outside the allowed prefixes.

Part of #54

FILE_READ and FILE_LIST IPC handlers previously accepted any path from the renderer, allowing a compromised renderer to read any file or enumerate any directory on the system. Now restricts to the user home directory (and WSL paths for WSL terminals).

Part of InbarR#54

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@InbarR
Copy link
Copy Markdown
Owner

InbarR commented Apr 21, 2026

Thanks for this! One concern before merging:

Functional regression: home-dir confinement breaks the directory picker.
tmax's Dir panel and file explorer legitimately browse paths like C:\projects\*, C:\dev\*, D:\*, etc. The tmax config even has a favoriteDirs list that users populate with project roots like C:\projects\tmax. After this patch, FILE_LIST / FILE_READ silently return [] / null for all of those paths - the dir panel looks empty or broken for many users.

Suggested alternative:

  • Allow paths that match any of: os.homedir(), the user's favoriteDirs + recentDirs from config, or \wsl.localhost\.
  • Or reconsider whether the threat model (compromised renderer) warrants filesystem confinement here at all - a compromised renderer could already exfiltrate any file via many other means.

Also edge cases in the current logic:

  • startsWith(os.homedir()) has the same sibling-dir bypass as security: add path traversal guard to git-diff-service #57 had: root /home/user vs /home/user-evil. Use path.sep appended.
  • On Windows, startsWith is case-sensitive but paths are case-insensitive.
  • startsWith('//wsl.localhost/') - on Windows Node's path.resolve normalizes that to \wsl.localhost\ (backslashes).

Leaving open until scope is settled.

@InbarR
Copy link
Copy Markdown
Owner

InbarR commented Apr 24, 2026

Closing without merging. Reasoning:

Regression risk - the allowlist relies on favoriteDirs + recentDirs, but tmax's dir panel supports typing a path to navigate (D:\fresh-drive\, \server\share\, etc.). Those paths aren't in favorites yet, so the first browse silently returns [] / null, which is hard to tell apart from "broken".

Low marginal value - the threat model is "compromised renderer reads arbitrary files". A compromised renderer already has PTY access, so it can cat /etc/passwd or type C:\Windows\... in any shell directly. Confining FILE_READ / FILE_LIST adds defense-in-depth but doesn't meaningfully shrink the attack surface.

Noting item 2 of #54 as "accepted risk, covered by existing PTY confinement requirement" rather than leaving it open indefinitely. Thanks for the patch anyway - the edge cases you handled (path.sep sibling bypass, Windows case) were all on point and they informed the #58 variant that did land.

@InbarR InbarR closed this Apr 24, 2026
yoziv added a commit to yoziv/tmax that referenced this pull request Apr 29, 2026
Pure-function tests (run without Electron packaging):
- PR InbarR#53/InbarR#56: extractLinkFromHtml + unwrapSafelinks (16 tests)
  Extracted to src/renderer/utils/link-extract.ts, shared by
  TerminalPanel.tsx and DetachedApp.tsx (deduplication)
- PR InbarR#57: path traversal guard (10 tests)
  Extracted to src/main/utils/security-guards.ts
- PR InbarR#58: OPEN_PATH extension blocklist (28 tests)
- PR InbarR#60: WSL distro name validation (9 tests)

E2E tests (require npm run package):
- PR InbarR#75: session rename propagates to pane title
- PR InbarR#13: minimum pane size enforcement
- PR InbarR#14: DEC focus sequence injection on pane switch
- PR InbarR#9: ShortcutsHelp escape in capture phase
- PR InbarR#10: pane title doesn't overlap terminal content
- PR InbarR#8: shifted key character in Ctrl+Shift+/ binding

Also extracts duplicated link-extract functions from
TerminalPanel.tsx and DetachedApp.tsx into a shared utility.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
InbarR pushed a commit that referenced this pull request May 2, 2026
Pure-function tests (run without Electron packaging):
- PR #53/#56: extractLinkFromHtml + unwrapSafelinks (16 tests)
  Extracted to src/renderer/utils/link-extract.ts, shared by
  TerminalPanel.tsx and DetachedApp.tsx (deduplication)
- PR #57: path traversal guard (10 tests)
  Extracted to src/main/utils/security-guards.ts
- PR #58: OPEN_PATH extension blocklist (28 tests)
- PR #60: WSL distro name validation (9 tests)

E2E tests (require npm run package):
- PR #75: session rename propagates to pane title
- PR #13: minimum pane size enforcement
- PR #14: DEC focus sequence injection on pane switch
- PR #9: ShortcutsHelp escape in capture phase
- PR #10: pane title doesn't overlap terminal content
- PR #8: shifted key character in Ctrl+Shift+/ binding

Also extracts duplicated link-extract functions from
TerminalPanel.tsx and DetachedApp.tsx into a shared utility.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants