Keep the access key out of child argv; honour useCaCertificate without a proxy - #180
Keep the access key out of child argv; honour useCaCertificate without a proxy#18007souravkunda wants to merge 1 commit into
Conversation
Sorry, something went wrong.
…t a proxy Four hardening fixes to the binary download path, all in the same area: - The access key was passed to lib/fetchDownloadSourceUrl.js as a positional argv element, so it was readable by any local user via `ps` or /proc/<pid>/cmdline for the lifetime of the spawn. It now travels in the child's environment (/proc/<pid>/environ is restricted to the owning user) and the remaining argv slots shift down by one accordingly. (CWE-214) - lib/download.js only applied `useCaCertificate` inside the `if (proxyHost && proxyPort)` branch, so a caller-supplied TLS trust anchor was ignored whenever no proxy was configured. Worse, the parent passes the literal `undefined` placeholders for the proxy slots in exactly that case, which arrive as the truthy *string* "undefined" — so the sync download built a proxy agent for host "undefined" and failed outright with `getaddrinfo ENOTFOUND undefined`. Both are fixed: the CA is applied unconditionally, and the proxy slots are compared with the existing `isUndefined` helper (as lib/fetchDownloadSourceUrl.js already does). (CWE-295) - retryBinaryDownload did an async fs.stat followed by a synchronous fs.unlinkSync inside the callback. Collapsed to a single ENOENT-tolerant fs.unlink, removing the window between the two and the uncatchable throw a failing unlinkSync raised from within the stat callback. (CWE-362) - getAvailableDirs fell back to os.tmpdir() itself — /tmp on Linux, which is world-writable — under a fixed, predictable binary name. It now uses a per-uid subdirectory created 0700, and that fallback is rejected unless it is a real directory owned by us and not group/world-writable, so a pre-created symlink or shared directory cannot be used as the destination for a binary we are about to execute. Only the temp fallback is subjected to this check; $HOME/.browserstack and cwd are unchanged. (CWE-377) Also pins the Semgrep CI container to an immutable digest so a mutated tag cannot redirect the workflow to a different image. (CWE-829)
07souravkunda
left a comment
There was a problem hiding this comment.
Automated security-fix review (round 0). Two blocking items, one nit — the substance of all five fixes checks out.
Verified independently:
- Argv shift in getSourceUrlSync maps 1:1 onto the new fetchDownloadSourceUrl.js reads; nothing else in the repo spawns that script, and the token is set on a copied env object so it does not leak into process.env or the daemon's environment.
- isUndefined handles all three parent invocations correctly (real proxy → agent built; CA-only → "undefined" placeholders rejected, CA applied; neither → no agent, no CA). The "mirrors the async download path" claim holds — the async download() already applies the CA outside the proxy branch.
- Semgrep digest resolves: docker-content-digest for returntocorp/semgrep:1.166.0 is exactly sha256:c180f0c93a17b420c0af5006214a29d3c747c5459c732b740191adf657dd0068.
- The retry change preserves semantics — fs.stat was already async, so returning downloadSync from inside the callback is unchanged, not newly broken.
- LocalBinary > Retries 2/2 and Binary filename 5/5 on this branch; the Download block's Invalid auth token is byte-identical on unmodified origin/master (pid line aside), so it is pre-existing.
- Single commit off origin/master, no drive-by edits, no manifest/lockfile involved, no internal tracker id in the title, body, commit message or diff.
Blocking:
- mode: 0o700 is applied to all three orderedPaths, changing ~/.browserstack from 0755 → 0700 — an undisclosed behaviour change that contradicts the PR body and the ticket comment, for no security gain on that path.
- No regression test committed for four behavioural changes; the "no offline seam" rationale doesn't hold — 11 of the 15 session checks are credential-free and network-free, and mocha's default spec would pick up a new test/ file with no config change.
For a human, not for this PR: the chain ticket for the tmpdir/TOCTOU chain is being closed at its entry rather than at its named breaker (binary integrity verification, which isn't implementable today), and the SSRF finding is routed to the still-open PR #176 rather than fixed here. Both are stated honestly in their completion comments and need an owner's accept, not more code.
Draft left as-is; approval is a human's call.
Sorry, something went wrong.
| try { | ||
| if(!this.checkPath(path)){ | ||
| fs.mkdirSync(path); | ||
| fs.mkdirSync(path, { mode: 0o700 }); |
There was a problem hiding this comment.
[blocking] mode: 0o700 here applies to every entry in orderedPaths, not just the temp fallback — so $HOME/.browserstack now gets created 0700 instead of 0755. That contradicts the PR description ("$HOME/.browserstack and process.cwd() keep their existing behaviour") and the same sentence in the ticket's completion comment.
Evidence — the same fs.mkdirSync call each branch makes, under the default umask 022:
umask : 22 master ~/.browserstack : 0755 PR#180 ~/.browserstack : 0700
getAvailableDirs passes requirePrivate = true only for i === length - 1 (line 320), so the check is correctly scoped — but the create mode is not, because it sits above the requirePrivate branch.
Why it matters: the pre-warm pattern — an image build or setup step downloads ~/.browserstack/BrowserStackLocal as one uid, the test step runs as another with the same $HOME — works today because the directory is world-traversable. At 0700 the second uid can no longer traverse it, so a setup that works on master breaks after upgrade, silently and with no note in the release. And the tightening buys nothing here: ~/.browserstack is still accepted with no ownership/permission check, so an attacker-writable one is used exactly as before.
Fix — scope the mode to the path that is actually being hardened:
if(!this.checkPath(path)){
fs.mkdirSync(path, requirePrivate ? { mode: 0o700 } : undefined);
}(Keeping 0700 everywhere is also defensible — but then the PR body and the completion comment both need to say so, and it should be called out as a behaviour change.)
Sorry, something went wrong.
| otherwise another local user can swap the binary between the download and | ||
| the exec, or pre-create the path as a symlink. Windows has no POSIX mode | ||
| bits; there this is a no-op. */ | ||
| this.isUserPrivateDir = function(dirPath){ |
There was a problem hiding this comment.
[blocking] Four of the five changes here are behavioural (argv→env token transport, isUndefined proxy guard + unconditional CA, single fs.unlink, per-uid private temp dir) and no test lands in the repo with them. The 15 targeted checks that validate them live in the fix session's scratch folder and are explicitly not committed, so they disappear with the session and the next person touching getAvailableDirs/getSourceUrlSync has nothing to break.
The stated reason is that the repo's only suite is a credential-gated live-integration suite with no offline seam for these paths. That isn't quite right — verify-fixes.js is the offline seam. Eleven of its fifteen checks need neither credentials nor network:
- argv probe: key absent from every argv element; child resolves it from env; all six argv positions map correctly after the shift
- source assertions on fetchDownloadSourceUrl.js and download.js (token from env; options.ca outside the proxy guard; isUndefined on the proxy slots)
- retryBinaryDownload: no fs.stat/unlinkSync left, and the runtime check that retry reaches download() on ENOENT
- all five F-021 checks: fallback is a per-uid subdir; created 0700 and accepted; world-writable dir rejected; symlink rejected; cwd still accepted
Only the two live-download checks need the network, and those are the ones worth skipping.
There's no .mocharc in the repo and test/ currently holds a single file, so mocha's default spec picks up a new test/localbinary-hardening.js with no config change. It also runs green today — LocalBinary > Retries already passes 2/2 on this branch, so a new offline file doesn't inherit the Download block's auth failure.
Ask: port those eleven checks into test/localbinary-hardening.js (describe/it around the same assertions). The two network checks can stay out, or sit behind an env guard. The cwd-still-accepted assertion is worth keeping either way — it's the guard against the ownership check creeping onto the non-temp paths.
No test is expected for the Semgrep digest pin — config-only, and a version-pin assertion would be against convention.
Sorry, something went wrong.
| const userAgent = [packageName, version].join('/'); | ||
| const env = Object.assign({ 'USER_AGENT': userAgent }, process.env); | ||
| if (this.key) { | ||
| env.BROWSERSTACK_LOCAL_AUTH_TOKEN = this.key; |
There was a problem hiding this comment.
[nit] The move off argv is right, and I confirmed the position shift lines up exactly with the new fetchDownloadSourceUrl.js reads (bsHost argv[2] → downloadFallback [3] → downloadErrorMessage [4] → proxyHost [5] → proxyPort [6] → useCaCertificate [7]).
One small thing: because env is seeded from process.env and the assignment is behind if (this.key), an ambient BROWSERSTACK_LOCAL_AUTH_TOKEN in the parent's environment now flows through to the child whenever this.key is falsy. On master that case sent the literal string "undefined" in argv, so it always failed cleanly; now it can silently authenticate with a value the caller never passed to Local.start(). Unlikely to bite given the variable name, but it makes the child's auth non-deterministic w.r.t. the caller's own config.
env.BROWSERSTACK_LOCAL_AUTH_TOKEN = this.key || '';Also worth a line in the README: this is a public package, and the variable is now a de-facto input to it.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Five fixes in the binary-download path, found by a security review of this repo.
Each is small and independent; they are batched because they touch the same
three files.
1. Access key no longer passed in child argv (CWE-214)
LocalBinary.getSourceUrlSync spawned lib/fetchDownloadSourceUrl.js with the
access key as a positional argument, so the key was visible to any local user
via ps aux or /proc/<pid>/cmdline for the duration of the spawn — a real
concern on shared CI runners and multi-tenant hosts.
The key now travels in the child's environment instead (/proc/<pid>/environ is
readable only by the owning user). The remaining argv slots shift down by one and
fetchDownloadSourceUrl.js was updated to match.
2. useCaCertificate is honoured when no proxy is configured (CWE-295)
lib/download.js only loaded the CA bundle inside the
if (proxyHost && proxyPort) branch, so a caller-supplied TLS trust anchor was
silently ignored whenever no proxy was set.
While fixing that I found the same lines were also functionally broken:
LocalBinary.downloadSync pushes literal undefined placeholders into the proxy
slots when only a CA is configured, and those reach the child as the string
"undefined" — which is truthy. The sync download therefore built an
HttpsProxyAgent for host "undefined" and failed outright.
Verified against unmodified master:
The proxy slots are now compared with the existing isUndefined helper (which
lib/fetchDownloadSourceUrl.js already uses for exactly this reason), and the CA
block moved outside the proxy branch. Confirmed the CA is genuinely the trust
anchor: with an unrelated self-signed CA the download now fails with
unable to get local issuer certificate instead of silently succeeding on the
system store.
3. Retry unlink is a single ENOENT-tolerant call (CWE-362)
retryBinaryDownload did an async fs.stat and then a synchronous
fs.unlinkSync inside its callback. Collapsed into one fs.unlink that ignores
the error: it removes the window between check and delete, and also removes the
uncatchable throw that a failing unlinkSync raised from inside the stat
callback.
4. Temp fallback is a user-private directory (CWE-377)
getAvailableDirs fell back to os.tmpdir() itself — /tmp on Linux, which is
world-writable — under the fixed, predictable name BrowserStackLocal. Since
that file is chmod'd 0755 and then executed, another local user could pre-create
the path or swap the file.
The fallback is now a per-uid subdirectory created 0700, and it is rejected
unless it is a real directory, owned by the current uid, and not group- or
world-writable. Only the temp fallback is checked — $HOME/.browserstack and
process.cwd() keep their existing behaviour, so a group-writable working
directory (common on shared CI) still works as before.
Note: on the temp-fallback path only, an existing /tmp/BrowserStackLocal is no
longer reused, so the binary is re-downloaded once.
5. Semgrep CI image pinned to a digest (CWE-829)
.github/workflows/Semgrep.yml referenced a mutable tag. Pinned to
@sha256:c180f0c9… so a re-pointed tag cannot redirect CI to a different image.
This matches the existing practice in the same file of pinning actions to commit
SHAs. Refresh with docker manifest inspect returntocorp/semgrep:<tag>.
Testing
here) and Binary filename pass. The Download block errors with
Invalid auth token on master too — those specs construct LocalBinary
directly and never set binary.key, so this is pre-existing and unrelated.
child still resolves it from env, argv positions still map correctly after the
shift, CA applied/enforced without a proxy, retry proceeds on ENOENT,
world-writable and symlinked temp dirs rejected, non-temp paths unaffected.
sync path with the token supplied only via env.
Automate session to a local page (bs-local.com), then stopped cleanly with no
dangling process.
No regression test was added for the digest pin (config-only).
Not addressed here
The daemon is still launched with --key <value> in its argv, so the key remains
visible in the process list for the tunnel's lifetime. Closing that needs a
--key-file/env option in the BrowserStackLocal binary itself and a matching
change across all six language bindings — tracked separately.