Skip to content

fix(cli,tui): escape and validate SSH session response fields#876

Open
johntmyers wants to merge 1 commit intomainfrom
fix/ssh-proxy-command-escape
Open

fix(cli,tui): escape and validate SSH session response fields#876
johntmyers wants to merge 1 commit intomainfrom
fix/ssh-proxy-command-escape

Conversation

@johntmyers
Copy link
Copy Markdown
Collaborator

Summary

  • The CLI and TUI build an SSH ProxyCommand string by interpolating fields from CreateSshSessionResponse (sandbox_id, token, and the assembled gateway URL) into a shell command. These three fields were not passed through shell_escape, even though exe_command and gateway_name in the same format strings were.
  • OpenSSH invokes ProxyCommand through /bin/sh -c, so a hostile or compromised gateway could inject shell metacharacters ($(...), backticks, ;, |) and execute arbitrary commands on the developer's workstation under their user privileges.
  • This PR centralizes the fix by extracting a build_proxy_command helper in openshell-core::forward that shell-escapes every interpolated argument, and adds a validate_ssh_session_response boundary validator that rejects malformed responses at the gRPC trust boundary — belt-and-suspenders with the escape.

Related Issue

OS-100

Changes

  • crates/openshell-core/src/forward.rs
    • New build_proxy_command(exe, gateway_url, sandbox_id, token, gateway_name) — one source of truth for the ProxyCommand format string; every argument goes through shell_escape.
    • New validate_ssh_session_response(&CreateSshSessionResponse) + SshSessionResponseError enum. Enforces conservative charsets per field: sandbox_id [A-Za-z0-9._-]{1,128}, token URL-safe ASCII up to 4096 bytes, gateway_host IP/bracketed-IPv6/DNS hostname ≤253 bytes, gateway_scheme exactly http|https, gateway_port in 1..=65535, connect_path RFC 3986 path charset with mandatory leading /, optional host_key_fingerprint [A-Za-z0-9:+/=-] only.
    • 13 new unit tests covering realistic responses and per-field adversarial inputs.
  • crates/openshell-cli/src/ssh.rs
    • ssh_session_config calls validate_ssh_session_response immediately after response.into_inner(), then uses build_proxy_command.
    • render_ssh_config shell-escapes gateway and name (defense-in-depth — validate_gateway_name already gates gateway).
  • crates/openshell-tui/src/lib.rs
    • All three ProxyCommand sites (shell connect, sandbox exec, port-forward reconciliation) validate the response and use build_proxy_command; validation failures surface via app.status_text / tracing::warn!.
  • proto/openshell.proto
    • Documents the charset contract on CreateSshSessionResponse fields. Servers must uphold; clients reject.

Testing

  • cargo test -p openshell-core — 110 tests pass, including 13 new tests for the validator and build_proxy_command.
  • cargo test -p openshell-cli --lib — 78 tests pass.
  • cargo test -p openshell-tui --lib — 12 tests pass.
  • mise run pre-commit — passes (lint, format, license headers, rust tests).
  • Regression check: build_proxy_command_escapes_shell_metacharacters builds the command with attacker-controlled values (x$(touch /tmp/pwn)x, tok`id`) and asserts no $, backtick, |, ;, &, or newline appears outside single-quoted regions.
  • Regression check: validate_ssh_session_response_rejects_* covers sandbox_id="a;b", token="$(id)", gateway_host="evil; cmd", gateway_scheme="javascript", gateway_port=0/70000, connect_path="/x + backtick + id + backtick + y"/"no-leading-slash"/"/a b"/"/a\nb".

Checklist

  • Conventional Commits format
  • No secrets or credentials in diff
  • Scoped to the issue at hand (no unrelated refactors)
  • Documentation updated where applicable (proto comment)

The CLI and TUI build an SSH ProxyCommand string by interpolating fields
from CreateSshSessionResponse into a shell command. Three fields -
sandbox_id, token, and the assembled gateway_url (composed from
gateway_scheme, gateway_host, gateway_port, connect_path) - were not
passed through shell_escape, even though exe_command and gateway_name
were. The resulting string is handed to `ssh -o ProxyCommand=...` which
OpenSSH routes through `/bin/sh -c`, so a hostile or compromised gateway
could inject shell metacharacters and execute arbitrary commands under
the developer's workstation account.

- Add build_proxy_command in openshell-core::forward that wraps every
  interpolated value in shell_escape. Use it at all four ProxyCommand
  sites: openshell-cli/src/ssh.rs (ssh_session_config) and the three
  TUI sites in openshell-tui/src/lib.rs (shell connect, sandbox exec,
  port-forward reconciliation).
- Add validate_ssh_session_response in openshell-core::forward, called
  at each site immediately after response.into_inner(). Enforces a
  conservative charset per field (sandbox_id, token, gateway_host,
  gateway_scheme, gateway_port, connect_path, host_key_fingerprint) so
  malformed responses fail loudly at the gRPC trust boundary before
  shell escaping is attempted. Belt-and-suspenders with the escape.
- Harden render_ssh_config so `gateway` and `name` are shell-escaped
  even though validate_gateway_name currently gates `gateway`.
- Document the charset contract on CreateSshSessionResponse in
  proto/openshell.proto: servers must uphold these rules and clients
  reject responses that violate them.
- Add regression unit tests covering adversarial values in every field
  and a build_proxy_command test asserting no shell metacharacter
  remains outside single-quoted regions.

Tracks OS-100.
@johntmyers johntmyers requested a review from a team as a code owner April 17, 2026 20:03
@johntmyers johntmyers added the topic:security Security issues label Apr 17, 2026
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot bot commented Apr 17, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@mjamiv
Copy link
Copy Markdown
Contributor

mjamiv commented Apr 19, 2026

+1 on security merit. openshell sandbox ssh-config output is regularly piped into ssh -F workflows; an unescaped session response field from a compromised gateway is a real ProxyCommand shell-injection vector. The validation/escape pass is the right defense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic:security Security issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants