Keep the access key out of child argv; honour useCaCertificate without a proxy

[07souravkunda]

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:

# master:  useCaCertificate set, no proxy
Got Error in binary downloading request Error: getaddrinfo ENOTFOUND undefined
--- bytes downloaded: 0 ---

# this branch: same invocation
Done
--- bytes downloaded: 40605816 ---

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

• eslint lib/* index.js — clean.
• mocha --grep LocalBinary — Retries (the retryBinaryDownload path changed
  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.
• 15 targeted checks covering all five fixes — argv contains no key while the
  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.
• Live: endpoint resolved and the 36 MB binary downloaded through the patched
  sync path with the token supplied only via env.
• Live: a tunnel started through this binding carried a real BrowserStack
  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.

[07souravkunda]

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:

1. 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.
2. 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.

[07souravkunda]

[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.)

[07souravkunda]

[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.

[07souravkunda]

[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.