Fix OS command injection in Local::start()/stop()/isRunning() (CWE-78, CWE-88) - #30

What

BrowserStack\Local built its command line by interpolating caller-supplied values straight into a string and handing it to shell_exec() / system(). Any consumer that forwards untrusted input into Local::start() — a service exposing a "configure tunnel" endpoint, a CI orchestrator splicing a repo-scoped variable into localIdentifier or proxyHost, a multi-tenant test runner — therefore handed that input arbitrary command execution with the privileges of the PHP process. CWE-78 (OS command injection) and CWE-88 (argument injection).

Confirmed reachable on eight distinct sinks:

One nuance worth recording: the assembled line begins with the exec builtin, which replaces the shell with the binary, so a trailing ; cmd chain never gets its turn. Command substitution is expanded before exec runs, so $(...) and backticks detonate on every sink above. The regression tests use that primitive rather than the ; form.

How

• escapeshellarg() on the argument NAME as well as the value. The name is interpolated as -<name>, so it is its own shell sink. An earlier revision of this branch rejected names outside [A-Za-z0-9_-] — that was wrong, and is removed. Quoting the name closes the sink with no behaviour change: the shell strips the quotes, so -myFlag still arrives at the binary as the argv element -myFlag. Whether the binary should accept unknown flags at all is a separate argument-injection (CWE-88) question, and not this change's business.
• Every caller-supplied value is wrapped in escapeshellarg(). Available since PHP 4, so the declared php >= 5.3.19 floor is untouched.
• isRunning() casts $pid to int and reports a non-integer pid as not running rather than asking ps about it.
• start_command() / stop_command() assemble a filtered list of parts instead of interpolating one string and collapsing whitespace afterwards. That preg_replace('/\s+/S', " ", ...) collapse existed only to squeeze out the gaps left by unset flags, and it rewrote whitespace inside quoted values too — which would now corrupt legitimately escaped arguments ("a b" would reach the binary as "a b").
• Two adjacent bugs the rewrite surfaced: start()'s Windows branch used a single-quoted PHP string, so it had been truncating a file literally named $this->logfile rather than the configured one; and $call . "2>&1" was missing its separating space, working only because the old collapse left a trailing one.

proc_open() with an array argument list was considered and not taken — the array form needs PHP 7.4+, well above this library's floor, and it would be a much larger rewrite of the daemon-stdout handling for no additional safety over full escaping.

Compatibility

Values now reach the binary as single quoted argv elements, which is exactly what the binary already received for benign input — no behavioural change at the binary boundary. The emitted command string does change shape (values are quoted), so the tests that assert on it are updated.

No caller-visible behaviour change. Nothing throws that did not throw before, and no argument name or value that worked stops working — test_unknown_argument_names_are_still_forwarded pins that, asserting the binary still receives -<name> verbatim for names an allowlist would have rejected (weird.name, name with space included).

The one behaviour change is a bug fix on Windows: start() used a single-quoted PHP string, so it had been truncating a file literally named $this->logfile rather than the configured logfile. The intended behaviour never worked there; it does now. Untested on Windows (no host) — see below.

Testing

• 9 injection regression tests in tests/LocalTest.php that execute the assembled command line for real against /bin/echo and assert the payload never runs. All 9 fail on the pre-fix code (verified: 13 failures pre-fix, 0 post-fix).
• tests/manual/injection-poc.php — the same proof as a standalone script. 8 of 9 arms vulnerable before the fix, 0 of 9 after.
• Full suite: 23 tests, 40 assertions green (1 pre-existing risky test, test_enable_force, asserts nothing — unrelated).
• Real end-to-end tunnel through the patched binding: start() → isRunning() === true → a BrowserStack Automate session driven over the tunnel to a local site via bs-local.com → stop() → isRunning() === false, pid === NULL, no stray processes.

The test harness is migrated to phpunit ^9.6 with a CI workflow, because phpunit 4.6.* cannot boot on any supported PHP and the regression tests would otherwise never execute. That migration is intentionally byte-identical to the one in the open TLS-verification PR (#29) so the two branches converge rather than conflict; the only divergence is the syntax-check step, which globs lib/*.php tests/*.php instead of naming files, so it works on either branch.

Not in scope

The access key is still passed as a positional argument, so it remains visible in ps / /proc/<pid>/cmdline (CWE-214). It can no longer inject, but moving it off the command line requires binary-side support for an environment variable or a mode-0600 key file, so it is tracked separately.

CI status — read before reacting to the Semgrep red

Semgrep OSS is the GitHub code-scanning view of the uploaded SARIF, not the gating scan. It lists the php.lang.security.exec-use findings at the lines carrying a nosemgrep annotation: the shell_exec in isRunning(), the system() in start(), and shell_exec($call). The semgrep run itself honours those suppressions (hence semgrep/ci passing with 0 blocking), but the SARIF still enumerates them, so code-scanning surfaces them as "new alerts in code changed by this pull request" purely because the diff touched those lines.

For context, master already carries four open alerts of the exact same rule on lib/Local.php (lines 57, 120, 122, 133) — the identical constructs, in their pre-fix, genuinely-injectable form. This PR does not add a new class of finding; it makes those very sinks safe and annotates the three it touches with the reason.

The annotations are deliberately narrow — one rule id, one line each, with the argument-safety reasoning in the comment directly above:

• isRunning() — the interpolated value is the intval() two lines up, guarded > 0, so only digits reach the shell.
• start()'s system() — the only interpolated value is escapeshellarg()'d.
• start()'s shell_exec($call) — $call comes from start_command(), where every caller-supplied part is escaped and every unquoted token is a fixed flag name.

No code-scanning alert was dismissed, and no rule was disabled repo-wide.

Sibling bindings — this bug is not php-only

The bindings mirror each other, so for whoever picks this up next:

The escaping approach here transfers to ruby directly. Worth doing as its own change rather than folding it in.