fix(windows): keep the proxy auth token off srt-win exec's command line - #478
Open
ig-ant wants to merge 5 commits into
Open
fix(windows): keep the proxy auth token off srt-win exec's command line#478ig-ant wants to merge 5 commits into
ig-ant wants to merge 5 commits into
Conversation
The per-session proxy auth token rode srt-win exec's argv inside the --env proxy URL values (HTTPS_PROXY=http://user:<token>@localhost:...). Windows command lines are freely readable by same-user sibling processes, so a concurrent local process could lift the token and ride that session's proxy allowlist. Token-bearing overlay entries now travel over srt-win exec's own stdin as a length-prefixed JSON frame, gated by a new --env-stdin flag: - TS: wrapCommandWithSandboxWindows splits the overlay - entries whose value embeds the token become a stdinPayload (u32 LE length + JSON [[KEY,VALUE],...]) the caller writes to the spawned broker's stdin; everything else stays on --env for debuggability. The token-bearing entries are also dropped from the broker's spawn env. The srt CLI and test helpers write the payload after spawning. - Rust: exec reads the frame from its stdin before spawning the runner (runner::decode_env_frame / merge_env_overlay; frame entries win over same-key --env duplicates) and forwards it to the child via the existing runner spec pipe, so the token still reaches only the sandboxed child's environment block. The broker's stdin was previously unused (the runner spec travels over a dedicated anonymous pipe, and the child's stdin is that pipe at EOF), so nothing is displaced. The flag is opt-in: an older host that never passes --env-stdin gets the old behavior unchanged. New smoke-env-stdin.ps1 proves end to end on the Windows CI legs that the token reaches the child's environment while appearing on no command line, host-side or sandbox-side, with a planted-token decoy validating the sweep. Unit tests cover the frame wire format on both sides and the argv/env partition.
Follow-up to the previous commit, which moved token-bearing proxy env entries off srt-win exec's argv onto a stdin frame. This makes the contract structural instead of conventional: - Misuse cannot hang: exec reads the frame BEFORE any per-exec side effect (previously it read after stamping per-exec deny ACEs, so a wedged caller left live ACEs on real paths), rejects a console stdin outright, and fails after 5s naming the stdinPayload contract if the frame never arrives. New spawnWrappedCommandWindows() owns the spawn+write so callers do not hand-roll the stdio contract; the srt CLI uses it. - Secrets are tagged at the source: generateProxyEnvEntries (the one builder that interpolates the token) returns secretKeys, and both the argv/stdin partition and the broker-env handling key off the tags - the two hand-copied value-matching predicates are gone. - Broker env scrubs by KEY, not value: every secret-tagged name is deleted from the ambient env in any case variant, so a stale export or an outer nested-sandbox session's token-bearing HTTPS_PROXY no longer rides the broker's spawn env (test added). - Channel keys must be disjoint: srt-win now errors on a case-insensitive overlap between --env and the frame instead of silently merging; the wire format has one writer definition per side (encodeEnvStdinFrame with the 4 MiB cap surfaced as a typed error, shared encode_frame in Rust, test-only encode_env_frame). - Stdin writes absorb only EPIPE (child exit is the real diagnostic); other stream errors surface via the child's error event, in the CLI, the srt-win spawn helper, and the test helper. The smoke script's planted-token control now diagnoses itself: it queries the decoy directly by PID and names which link broke (process gone, WMI lacks command-line rights + whoami, or a quoting bug with the actual command line), avoids -WindowStyle (can fail outside an interactive session) with a Diagnostics.Process fallback, waits for the decoy to actually die before the clean sweep, and checks exec's own exit state before interpreting sweep results. README documents the spawn contract and the version-skew failure mode (an older pinned srt-win.exe rejects --env-stdin with a clean unknown-argument error; prebuilt binaries ship from source at release time).
…delivery in tests The smoke's S4 check (token present in the sandboxed child's environment) failed through two observation bugs, not delivery bugs: the child's bulk stdout dump can go missing in some contexts while trailing single lines arrive, and Windows PowerShell 5.1 inside the sandbox produced zero entries from `Get-ChildItem env:` with no trace, because the loop sat outside any try/catch. The in-guest lib unit tests pin the actual delivery path (frame -> spec -> env block) as correct. - Probe: the child writes its full dump to cenv.txt in the probe dir (explicit icacls modify grants for BUILTIN\Users and the sandbox SID); the host prefers the file over stdout parsing and reports which oracle ran plus the child's FILEOK/FILEERR breadcrumb. - Env enumeration is double-sourced with distinct prefixes - CENVD= from [Environment]::GetEnvironmentVariables() (pure .NET, no PS provider) and CENV= from Get-ChildItem env: - each wrapped in try/catch that records CENVDERR=/CENVERR= in the dump, so an enumeration failure can never be silent again. S4 accepts either prefix and prints the entire dump verbatim on failure. - Earlier hardening in this change: exec's 'overlay=4 var(s)' stderr line asserted (S2b); minimal literal PATH/PATHEXT and a Quote-Arg that refuses embedded quotes keep the hand-rolled Arguments string sound; the decoy control diagnoses itself by direct PID query and stays alive (ping-first - cmd's rem swallows the & operator). - Tests (library unittest target, cargo test --lib): cli.rs drives a hand-rolled probe-shaped frame through decode -> merge -> RunnerSpec -> encode and asserts the token var in the spec JSON; launch.rs asserts a spec overlay entry lands in the CREATE_UNICODE_ENVIRONMENT block with its lowercase case-twin. Verified live on a Windows 11 arm64 guest: lib tests 2/2 in-guest; smoke-env-stdin ALL OK (S1 control detects a planted argv token; S3 and S5 sweeps find the real token on no command line host- or sandbox-side; S4 finds it in the child environment via the file oracle).
… sites Three integration-test sites missed the --env-stdin contract sweep and failed on Windows CI: - rexecSandboxed (H5/H6) and the H-glob restore re-check spawned the wrapped argv without writing stdinPayload, so srt-win exec read EOF where the secret-env frame belongs and failed with "read length prefix: failed to fill whole buffer". Both now pass the frame as spawnAsync input. - The "argv carrying the --env overlay" row asserted the token-bearing proxy URLs (HTTP_PROXY/ALL_PROXY/DOCKER_HTTP_PROXY) ride --env; with a session auth token they now ride the stdin frame instead. The row asserts the split: those keys absent from argv, present in the decoded frame (ports still range-checked), with tokenless RSYNC_PROXY proving --env still carries the plain set. No-Verification-Needed: test-only diff
…scrub test The scrub test planted a fake HTTPS_PROXY (dead port) in process.env and restored it in a finally, but Bun latches HTTP(S)_PROXY into its HTTP client the moment the variable is SET - even with no request in flight and even after deletion. Every later in-process request then dialed the dead proxy, so whichever TLS-terminating e2e suite ran next 502'd: credential-mask-sigv4 on one CI leg, tls-terminate-proxy on another, and the same file's G/P schannel rows on Windows - the failing set varied with test-file order, which is per-runner. Exercise the same delete-by-key scrub path with CLOUDSDK_PROXY_PASSWORD instead: it is secret-tagged by generateProxyEnvEntries (the property under test) but meaningless to Bun's networking, verified by probe not to latch. A comment marks the hazard so no future test replants the latched names. No-Verification-Needed: test-only diff
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The per-session proxy auth token rode
srt-win exec's argv inside the--envproxy URL values. Windows command lines are freely readable by same-user sibling processes, so a concurrent local process could lift the token and ride that session's proxy allowlist.srt-win exec's own stdin as a<u32 LE length><JSON [[KEY,VALUE],…]>frame behind an opt-in--env-stdinflag; everything tokenless stays on--envfor debuggability.generateProxyEnvEntriestags secret entries at the source (no substring heuristics), and the broker's spawn env deletes every secret-tagged key value-blind (closing the nested-session stale-token case).spawnWrappedCommandWindowsowns spawn+frame-write as the sanctioned path;wrapWithSandboxArgvreturnsstdinPayloadfor callers that spawn themselves (README documents the contract and the old-binary skew failure mode: clean clap unknown-argument error).Test plan
Unit: wire-format round-trips both sides (shared
encodeEnvStdinFrame/encode_frame, 4 MiB cap enforced with a config-facing error); token absent from argv and broker env (including a planted foreign-token case-variantHtTp_PrOxY); channel disjointness both arms. Cross-target clippy (-D warnings) both MSVC targets, fmt, typecheck, eslint clean; winsrt suites 39/0.Live Windows 11 arm64 verification: in-guest lib tests pin frame→spec→env-block delivery (2/2); new CI smoke
ci/smoke-env-stdin.ps1ALL OK — planted-token control proves the sweep detects argv tokens (S1), exec merges the frame (S2b), the real token appears on no command line host-side (S3) or sandbox-side (S5) and IS present in the child environment (S4, file-based oracle). The smoke runs on both Windows CI legs.Known follow-up (separate PR): on Linux, bwrap
--setenvargv exposes the token to other local users via world-readable/proc/pid/cmdline; macOS argv==environ for same-uid readers and seatbelt already confines sandboxed peers.