From 8cfd7df77ec9e2d3aa6060fd9283ccbd829696af Mon Sep 17 00:00:00 2001 From: Sourav Kunda Date: Wed, 12 Aug 2026 18:07:06 +0530 Subject: [PATCH 1/3] Fix OS command injection in Local::start()/stop()/isRunning() (CWE-78, CWE-88) Every caller-supplied value reached shell_exec()/system() by way of raw string interpolation, so any consumer that forwards untrusted input into Local::start() -- a wrapping HTTP service, a CI orchestrator splicing a repo-scoped variable into localIdentifier or proxyHost, a multi-tenant test runner -- handed the caller arbitrary command execution with the privileges of the PHP process. Confirmed reachable on eight distinct sinks: localIdentifier, proxyHost/Port/ User/Pass, hosts, logfile (both the -logFile argument and the truncating system() call in start()), an arbitrary argument NAME through the add_args() else-branch, the value side of that same branch, the public $pid property in isRunning(), and the localIdentifier fragment reused by stop_command(). The assembled line starts with the `exec` builtin, so a trailing `; cmd` chain does not detonate -- but command substitution is expanded before exec runs, and $(...) fires on all of them. - add_args() rejects any argument name outside [A-Za-z0-9_-]+ with a LocalException. A name is emitted as a `-` flag, so it cannot be quoted without ceasing to be a flag; it has to be validated instead. The charset keeps every documented custom flag working, dashes included. - 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 instead of asking ps about it. - start_command()/stop_command() assemble a filtered list of parts rather than interpolating one string and collapsing whitespace afterwards. That collapse only existed to squeeze out the gaps left by unset flags, and it rewrote whitespace inside quoted values too, which would now corrupt legitimately escaped arguments. - start()'s logfile truncation is quoted as well; its Windows branch used a single-quoted PHP string, so it had been truncating a file literally named '$this->logfile' rather than the configured one. - `$call . "2>&1"` was missing its separating space; it only worked because the old whitespace collapse left a trailing one. Values now reach the binary as single quoted argv elements, which is what the binary already received for benign input -- no behavioural change there. The emitted command line does change shape (values are quoted), so the tests that assert on it are updated. Tests: eight injection regression tests that execute the assembled command line for real against /bin/echo and assert the payload never runs. All eight fail on the pre-fix code. tests/manual/injection-poc.php is the same proof as a standalone script (8 of 9 arms vulnerable before, 0 after). The test harness is modernised to phpunit ^9.6 with a CI workflow, matching the open TLS-verification PR, because phpunit 4.6 cannot boot on a supported PHP and the regression tests would otherwise never execute. Residual, deliberately not in scope: the access key is still a positional argument and so is still visible in `ps`/`/proc//cmdline`. It can no longer inject, and moving it off the command line needs binary-side support -- tracked separately. --- .github/workflows/php.yml | 42 ++++++++ .gitignore | 1 + composer.json | 2 +- lib/Local.php | 129 +++++++++++++++++++---- phpunit.xml | 9 +- tests/LocalTest.php | 184 ++++++++++++++++++++++++++++++--- tests/manual/injection-poc.php | 161 +++++++++++++++++++++++++++++ 7 files changed, 485 insertions(+), 43 deletions(-) create mode 100644 .github/workflows/php.yml create mode 100644 tests/manual/injection-poc.php diff --git a/.github/workflows/php.yml b/.github/workflows/php.yml new file mode 100644 index 0000000..1dd0d79 --- /dev/null +++ b/.github/workflows/php.yml @@ -0,0 +1,42 @@ +name: PHP + +on: + pull_request: + branches: ["master", "main"] + push: + branches: ["master", "main"] + +permissions: + contents: read + +jobs: + test: + name: lint + phpunit + runs-on: ubuntu-latest + # 7.4 rather than 8.x: lib/ sets properties dynamically, which PHP 8.2 + # deprecates, and the library's own floor is php >= 5.3.19. phpunit 9.6 + # supports 7.3+. + container: php:7.4-cli + steps: + - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3 + + # Runs first and on its own: a syntax error must fail the build even if the + # suite cannot boot. + - name: Syntax check + run: | + for f in lib/*.php tests/*.php tests/manual/*.php; do + [ -e "$f" ] || continue + php -l "$f" + done + + # git + unzip are not in the official php image, and Composer needs one of + # them to unpack downloaded packages (ext-zip is not built in either). + - name: Install dependencies + run: | + apt-get update -qq && apt-get install -y -qq --no-install-recommends git unzip >/dev/null + curl -sS https://getcomposer.org/installer | php + php composer.phar install --no-interaction --no-progress + + # Excludes @group network — those tests reach badssl.com and the real S3 host. + - name: PHPUnit + run: ./vendor/bin/phpunit --exclude-group network diff --git a/.gitignore b/.gitignore index f01fad7..886dd88 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ composer.phar vendor/** composer.lock local.log +.phpunit.result.cache diff --git a/composer.json b/composer.json index 165a2e2..0c7172c 100644 --- a/composer.json +++ b/composer.json @@ -13,7 +13,7 @@ "php": ">=5.3.19" }, "require-dev": { - "phpunit/phpunit": "4.6.*" + "phpunit/phpunit": "^9.6" }, "suggest": { "phpdocumentor/phpdocumentor": "2.*" diff --git a/lib/Local.php b/lib/Local.php index 69a9973..cff9b65 100644 --- a/lib/Local.php +++ b/lib/Local.php @@ -10,8 +10,46 @@ class Local { + /** + * Argument names are emitted as `-` flags straight into the command + * line, so the name itself is an injection vector (CWE-78). Only these + * characters may reach the shell as a flag name; anything else is rejected + * by add_args() rather than quoted, because a quoted flag name would not be + * a flag any more. + */ + const ARG_KEY_PATTERN = '/^[A-Za-z0-9_-]+$/'; + public $pid = NULL; - + + /** + * Quote a value so the shell treats it as exactly one literal argument. + * escapeshellarg() has been available since PHP 4, so this keeps the + * library's declared floor of PHP >= 5.3.19. + */ + private static function esc($value) { + return escapeshellarg((string) $value); + } + + /** + * Join pre-escaped command fragments, dropping the empty ones. + * + * This replaces the old `preg_replace('/\s+/S', " ", $command)` collapse. + * That collapse existed only to squeeze out the gaps left by unset flags, + * but it rewrote whitespace *inside* quoted values too — which would now + * corrupt legitimately escaped arguments (a value of "a b" would arrive at + * the binary as "a b"). Filtering the parts achieves the same tidy command + * line without touching the arguments themselves. + */ + private static function join_parts($parts) { + $out = array(); + foreach ($parts as $part) { + $part = trim((string) $part); + if ($part !== "") + $out[] = $part; + } + return implode(" ", $out); + } + public function __construct() { $this->key = getenv("BROWSERSTACK_ACCESS_KEY"); $this->logfile = getcwd() . "/local.log"; @@ -54,7 +92,15 @@ public function isRunning() { return False; } else { - $return_message = shell_exec("ps -" . "$this->pid " . "| wc -l"); + // $pid is public and is also populated from the spawned process's stdout + // (see start()), so it must never be concatenated into a shell string + // raw. A PID is an integer by definition — cast, and treat anything that + // is not a positive integer as "not running" rather than asking ps about + // it. + $pid = intval($this->pid); + if ($pid <= 0) + return False; + $return_message = shell_exec("ps -" . $pid . " | wc -l"); if (intval($return_message) > 1) { return True; @@ -64,6 +110,12 @@ public function isRunning() { } public function add_args($arg_key, $value = NULL) { + if (!is_string($arg_key) || !preg_match(self::ARG_KEY_PATTERN, $arg_key)) + throw new LocalException( + "Invalid BrowserStack Local argument name. Argument names may only " . + "contain letters, digits, '-' and '_'; got: " . var_export($arg_key, true) + ); + if ($arg_key == "key") $this->key = $value; elseif ($arg_key == "binaryPath") @@ -81,15 +133,15 @@ public function add_args($arg_key, $value = NULL) { elseif ($arg_key == "forcelocal") $this->force_local_flag = "-forcelocal"; elseif ($arg_key == "localIdentifier") - $this->local_identifier_flag = "-localIdentifier $value"; + $this->local_identifier_flag = "-localIdentifier " . self::esc($value); elseif ($arg_key == "proxyHost") - $this->proxy_host = "-proxyHost $value"; + $this->proxy_host = "-proxyHost " . self::esc($value); elseif ($arg_key == "proxyPort") - $this->proxy_port = "-proxyPort $value"; + $this->proxy_port = "-proxyPort " . self::esc($value); elseif ($arg_key == "proxyUser") - $this->proxy_user = "-proxyUser $value"; + $this->proxy_user = "-proxyUser " . self::esc($value); elseif ($arg_key == "proxyPass") - $this->proxy_pass = "-proxyPass $value"; + $this->proxy_pass = "-proxyPass " . self::esc($value); elseif ($arg_key == "forceproxy") $this->force_proxy_flag = "-forceproxy"; elseif ($arg_key == "hosts") @@ -98,11 +150,11 @@ public function add_args($arg_key, $value = NULL) { $this->folder_flag = "-f"; $this->folder_path = $value; } - elseif (strtolower($value) == "true"){ + elseif ($value !== NULL && strtolower((string) $value) == "true"){ array_push($this->user_args, "-$arg_key"); } else { - array_push($this->user_args, "-$arg_key '$value'"); + array_push($this->user_args, "-$arg_key " . self::esc($value)); } } @@ -114,11 +166,12 @@ public function start($arguments) { $this->binary_path = $this->binary->binary_path(); $call = $this->start_command(); - if(strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') - system('echo "" > '. '$this->logfile'); - else - system("echo \"\" > '$this->logfile' "); - $call = $call . "2>&1"; + // The logfile path is caller-supplied (add_args('logfile', ...)), so it is + // quoted here too — the old single-quote wrapper was escapable. The Windows + // branch additionally used a single-quoted PHP string, so it truncated a + // file literally named '$this->logfile' instead of the configured one. + system("echo \"\" > " . self::esc($this->logfile)); + $call = $call . " 2>&1"; $return_message = shell_exec($call); $data = json_decode($return_message,true); if ($data["state"] != "connected") { @@ -140,10 +193,38 @@ public function start_command() { if(strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') $exec = "call"; - $user_args = join(' ', $this->user_args); - $command = "$exec $this->binary_path -d start -logFile '$this->logfile' $this->folder_flag $this->key $this->folder_path $this->force_local_flag $this->local_identifier_flag $this->only_flag $this->only_automate_flag $this->proxy_host $this->proxy_port $this->proxy_user $this->proxy_pass $this->force_proxy_flag $this->force_flag $this->verbose_flag $this->hosts $user_args"; - $command = preg_replace('/\s+/S', " ", $command); - return $command; + // Every caller-supplied value is quoted so the shell sees it as one literal + // argument. The fixed flag names and the $exec builtin are the only tokens + // that stay unquoted, and none of them is caller-controlled. The flag + // fragments built in add_args() are already escaped there. + $parts = array($exec); + if ((string) $this->binary_path !== "") + $parts[] = self::esc($this->binary_path); + $parts[] = "-d"; + $parts[] = "start"; + $parts[] = "-logFile"; + $parts[] = self::esc($this->logfile); + $parts[] = $this->folder_flag; + if ((string) $this->key !== "") + $parts[] = self::esc($this->key); + if ((string) $this->folder_path !== "") + $parts[] = self::esc($this->folder_path); + $parts[] = $this->force_local_flag; + $parts[] = $this->local_identifier_flag; + $parts[] = $this->only_flag; + $parts[] = $this->only_automate_flag; + $parts[] = $this->proxy_host; + $parts[] = $this->proxy_port; + $parts[] = $this->proxy_user; + $parts[] = $this->proxy_pass; + $parts[] = $this->force_proxy_flag; + $parts[] = $this->force_flag; + $parts[] = $this->verbose_flag; + if ((string) $this->hosts !== "") + $parts[] = self::esc($this->hosts); + $parts = array_merge($parts, $this->user_args); + + return self::join_parts($parts); } public function stop_command() { @@ -152,10 +233,14 @@ public function stop_command() { if(strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') $exec = "call"; - $user_args = join(' ', $this->user_args); - $command = "$exec $this->binary_path -d stop $this->local_identifier_flag"; - $command = preg_replace('/\s+/S', " ", $command); - return $command; + $parts = array($exec); + if ((string) $this->binary_path !== "") + $parts[] = self::esc($this->binary_path); + $parts[] = "-d"; + $parts[] = "stop"; + $parts[] = $this->local_identifier_flag; + + return self::join_parts($parts); } } diff --git a/phpunit.xml b/phpunit.xml index c5544cc..1abbab8 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -1,8 +1,11 @@ - + + - tests + tests - diff --git a/tests/LocalTest.php b/tests/LocalTest.php index bee33df..b579567 100644 --- a/tests/LocalTest.php +++ b/tests/LocalTest.php @@ -8,27 +8,27 @@ require_once __DIR__ . '/../vendor/autoload.php'; -class LocalTest extends \PHPUnit_Framework_TestCase { +class LocalTest extends \PHPUnit\Framework\TestCase { private $bs_local; - public function setUp(){ + protected function setUp(): void { $this->bs_local = new Local(); } - public function tearDown(){ + protected function tearDown(): void { $this->bs_local->stop(); } public function test_verbose() { $this->bs_local->add_args('v'); - $this->assertContains('-v',$this->bs_local->start_command()); + $this->assertStringContainsString('-v',$this->bs_local->start_command()); } public function test_set_folder() { $this->bs_local->add_args('f', "/"); - $this->assertContains('-f',$this->bs_local->start_command()); - $this->assertContains('/',$this->bs_local->start_command()); + $this->assertStringContainsString('-f',$this->bs_local->start_command()); + $this->assertStringContainsString('/',$this->bs_local->start_command()); } public function test_enable_force() { @@ -37,36 +37,36 @@ public function test_enable_force() { public function test_set_local_identifier() { $this->bs_local->add_args("localIdentifier", "randomString"); - $this->assertContains('-localIdentifier randomString',$this->bs_local->start_command()); + $this->assertStringContainsString("-localIdentifier 'randomString'",$this->bs_local->start_command()); } public function test_enable_only() { $this->bs_local->add_args("only"); - $this->assertContains('-only',$this->bs_local->start_command()); + $this->assertStringContainsString('-only',$this->bs_local->start_command()); } public function test_enable_only_automate() { $this->bs_local->add_args("onlyAutomate"); - $this->assertContains('-onlyAutomate', $this->bs_local->start_command()); + $this->assertStringContainsString('-onlyAutomate', $this->bs_local->start_command()); } public function test_enable_force_local() { $this->bs_local->add_args("forcelocal"); - $this->assertContains('-forcelocal',$this->bs_local->start_command()); + $this->assertStringContainsString('-forcelocal',$this->bs_local->start_command()); } public function test_custom_boolean_argument() { $this->bs_local->add_args("boolArg1", true); $this->bs_local->add_args("boolArg2", true); - $this->assertContains('-boolArg1',$this->bs_local->start_command()); - $this->assertContains('-boolArg2',$this->bs_local->start_command()); + $this->assertStringContainsString('-boolArg1',$this->bs_local->start_command()); + $this->assertStringContainsString('-boolArg2',$this->bs_local->start_command()); } public function test_custom_keyval() { $this->bs_local->add_args("customKey1", "custom value1"); $this->bs_local->add_args("customKey2", "custom value2"); - $this->assertContains('-customKey1 \'custom value1\'',$this->bs_local->start_command()); - $this->assertContains('-customKey2 \'custom value2\'',$this->bs_local->start_command()); + $this->assertStringContainsString('-customKey1 \'custom value1\'',$this->bs_local->start_command()); + $this->assertStringContainsString('-customKey2 \'custom value2\'',$this->bs_local->start_command()); } public function test_set_proxy() { @@ -74,19 +74,159 @@ public function test_set_proxy() { $this->bs_local->add_args("proxyPort", 8080); $this->bs_local->add_args("proxyUser", "user"); $this->bs_local->add_args("proxyPass", "pass"); - $this->assertContains('-proxyHost localhost -proxyPort 8080 -proxyUser user -proxyPass pass',$this->bs_local->start_command()); + $this->assertStringContainsString("-proxyHost 'localhost' -proxyPort '8080' -proxyUser 'user' -proxyPass 'pass'",$this->bs_local->start_command()); } public function test_enable_force_proxy() { $this->bs_local->add_args("-forceproxy"); - $this->assertContains('-forceproxy',$this->bs_local->start_command()); + $this->assertStringContainsString('-forceproxy',$this->bs_local->start_command()); } public function test_hosts() { $this->bs_local->add_args("-hosts", "localhost,8080,0"); - $this->assertContains('localhost,8080,0',$this->bs_local->start_command()); + $this->assertStringContainsString('localhost,8080,0',$this->bs_local->start_command()); } + // --------------------------------------------------------------------------- + // Command-injection regression tests (CWE-78 / CWE-88). + // + // These execute the assembled command line for real, but never the + // BrowserStackLocal binary: binary_path is pointed at /bin/echo, so the only + // thing that can run besides echo is an injected payload. Each test drops a + // marker file; the marker existing means the shell executed attacker bytes. + // + // The payloads use command substitution rather than a `; cmd` chain on + // purpose: the command line starts with the `exec` builtin, which replaces + // the shell, so a trailing chain never gets its turn -- while $(...) is + // expanded before exec runs. Every one of these fails on the pre-fix code. + // --------------------------------------------------------------------------- + + /** @return string a marker path that does not exist yet */ + private function marker($name) { + $path = rtrim(sys_get_temp_dir(), '/') . '/bsl_test_' . $name . '_' . getmypid(); + if (file_exists($path)) { unlink($path); } + return $path; + } + + private function assertNotExecuted($marker, $command) { + $fired = file_exists($marker); + if ($fired) { unlink($marker); } + $this->assertFalse($fired, "payload executed -- command injection is live. Command was: " . $command); + } + + public function test_no_injection_via_local_identifier() { + $marker = $this->marker('lid'); + $this->bs_local->binary_path = '/bin/echo'; + $this->bs_local->add_args('key', 'dummykey'); + $this->bs_local->add_args('localIdentifier', 'x$(touch ' . $marker . ')'); + $command = $this->bs_local->start_command(); + shell_exec($command . ' 2>&1'); + $this->assertNotExecuted($marker, $command); + } + + public function test_no_injection_via_proxy_host_backticks() { + $marker = $this->marker('proxy'); + $this->bs_local->binary_path = '/bin/echo'; + $this->bs_local->add_args('key', 'dummykey'); + $this->bs_local->add_args('proxyHost', 'x`touch ' . $marker . '`'); + $command = $this->bs_local->start_command(); + shell_exec($command . ' 2>&1'); + $this->assertNotExecuted($marker, $command); + } + + public function test_no_injection_via_hosts() { + $marker = $this->marker('hosts'); + $this->bs_local->binary_path = '/bin/echo'; + $this->bs_local->add_args('key', 'dummykey'); + $this->bs_local->add_args('hosts', 'x$(touch ' . $marker . ')'); + $command = $this->bs_local->start_command(); + shell_exec($command . ' 2>&1'); + $this->assertNotExecuted($marker, $command); + } + + public function test_no_injection_via_logfile() { + $marker = $this->marker('logfile'); + $this->bs_local->binary_path = '/bin/echo'; + $this->bs_local->add_args('key', 'dummykey'); + $this->bs_local->add_args('logfile', "/tmp/bsl_test.log'\$(touch " . $marker . ")'"); + $command = $this->bs_local->start_command(); + shell_exec($command . ' 2>&1'); + $this->assertNotExecuted($marker, $command); + } + + public function test_no_injection_via_custom_flag_value() { + $marker = $this->marker('customval'); + $this->bs_local->binary_path = '/bin/echo'; + $this->bs_local->add_args('key', 'dummykey'); + $this->bs_local->add_args('customFlag', "' \$(touch " . $marker . ") '"); + $command = $this->bs_local->start_command(); + shell_exec($command . ' 2>&1'); + $this->assertNotExecuted($marker, $command); + } + + public function test_no_injection_via_stop_command() { + $marker = $this->marker('stop'); + $this->bs_local->binary_path = '/bin/echo'; + $this->bs_local->add_args('localIdentifier', 'x$(touch ' . $marker . ')'); + $command = $this->bs_local->stop_command(); + shell_exec($command . ' 2>&1'); + $this->assertNotExecuted($marker, $command); + } + + public function test_no_injection_via_pid_in_is_running() { + $marker = $this->marker('pid'); + $this->bs_local->pid = 'aux$(touch ' . $marker . ')'; + $running = $this->bs_local->isRunning(); + $this->bs_local->pid = NULL; // keep tearDown()'s stop() a no-op + $this->assertNotExecuted($marker, 'isRunning() with a non-numeric $pid'); + $this->assertFalse($running, 'a non-numeric pid must not be reported as running'); + } + + public function test_rejects_an_injectable_argument_name() { + $marker = $this->marker('argkey'); + $thrown = false; + try { + $this->bs_local->add_args('x$(touch ' . $marker . ')', 'v'); + } catch (LocalException $e) { + $thrown = true; + } + $this->assertTrue($thrown, 'add_args() must reject an argument name that is not [A-Za-z0-9_-]+'); + $this->assertNotExecuted($marker, 'add_args() with an injectable argument name'); + } + + public function test_argument_names_the_library_documents_are_still_accepted() { + // Regression guard on the argument-name gate: everything the README and the + // existing tests use must keep working, dashes included. + foreach (array('v', 'force', 'only', 'onlyAutomate', 'forcelocal', 'forceproxy', + '-forceproxy', '-hosts', 'customKey1', 'custom_key_2', 'a1') as $name) { + $local = new Local(); + $local->add_args($name, 'true'); + $this->assertNotEmpty($local->start_command(), "argument name '$name' must stay usable"); + } + } + + public function test_unset_options_leave_no_empty_arguments() { + // start_command() used to squeeze out the gaps left by unset flags with a + // whitespace collapse. That collapse also rewrote whitespace inside quoted + // values, so it was replaced by dropping the empty parts -- this asserts the + // command line stays free of stray empty tokens. + $this->bs_local->add_args('key', 'dummykey'); + $command = $this->bs_local->start_command(); + $this->assertStringNotContainsString(" ", $command); + $this->assertStringNotContainsString(" '' ", $command); + } + + public function test_values_keep_their_internal_whitespace() { + $this->bs_local->add_args('key', 'dummykey'); + $this->bs_local->add_args('localIdentifier', "two spaces"); + $this->assertStringContainsString("-localIdentifier 'two spaces'", $this->bs_local->start_command()); + } + + /** + * Starts the real binary — needs BROWSERSTACK_ACCESS_KEY and outbound network. + * + * @group network + */ public function test_isRunning() { $this->assertFalse($this->bs_local->isRunning()); $this->bs_local->start(array('v' => true)); @@ -97,12 +237,22 @@ public function test_isRunning() { $this->assertTrue($this->bs_local->isRunning()); } + /** + * Starts the real binary — needs BROWSERSTACK_ACCESS_KEY and outbound network. + * + * @group network + */ public function test_checkPid() { $this->assertFalse($this->bs_local->isRunning()); $this->bs_local->start(array('v' => true)); $this->assertTrue($this->bs_local->pid > 0); } + /** + * Starts the real binary twice — needs BROWSERSTACK_ACCESS_KEY and outbound network. + * + * @group network + */ public function test_multiple_binary() { $this->bs_local->start(array('v' => true)); $bs_local_2 = new Local(); diff --git a/tests/manual/injection-poc.php b/tests/manual/injection-poc.php new file mode 100644 index 0000000..73fe1a4 --- /dev/null +++ b/tests/manual/injection-poc.php @@ -0,0 +1,161 @@ +getMessage(); + } + if ($command !== null) { + shell_exec($command . ' 2>&1'); + } + $fired = file_exists($marker); + printf("ARM %d %-44s payload_executed=%s\n", $arm, $label, $fired ? 'YES <-- VULNERABLE' : 'no'); + if ($note !== '') { + printf(" %s\n", $note); + } + if ($command !== null) { + printf(" command: %s\n", $command); + } + if ($fired) { $vulnerable++; unlink($marker); } +} + +function fresh_local() { + $local = new BrowserStack\Local(); + $local->binary_path = '/bin/echo'; + $local->add_args('key', 'dummykey'); + return $local; +} + +// -- F-002: known argument fields interpolated into the command string -------- +$m = marker(1); +run_arm('localIdentifier, command substitution', $m, function () use ($m) { + $local = fresh_local(); + $local->add_args('localIdentifier', 'x$(touch ' . $m . ')'); + return $local->start_command(); +}); + +$m = marker(2); +run_arm('proxyHost, backticks', $m, function () use ($m) { + $local = fresh_local(); + $local->add_args('proxyHost', 'x`touch ' . $m . '`'); + return $local->start_command(); +}); + +$m = marker(3); +run_arm('hosts, command substitution', $m, function () use ($m) { + $local = fresh_local(); + $local->add_args('hosts', 'x$(touch ' . $m . ')'); + return $local->start_command(); +}); + +$m = marker(4); +run_arm('localIdentifier, semicolon chain', $m, function () use ($m) { + $local = fresh_local(); + $local->add_args('localIdentifier', 'x; touch ' . $m . '; echo'); + return $local->start_command(); +}); + +// -- F-002: the logfile path reaches both shell sinks ------------------------- +$m = marker(5); +run_arm('logfile, single-quote break-out', $m, function () use ($m) { + $local = fresh_local(); + $local->add_args('logfile', "/tmp/bsl_poc.log'\$(touch " . $m . ")'"); + return $local->start_command(); +}); + +// (start()'s own `system("echo \"\" > ")` truncation call takes the same +// caller-supplied logfile and is quoted the same way; it is not exercised here +// because reaching start() would download and launch the real binary.) + +// -- F-003: the add_args() else-branch, via the KEY and via the VALUE --------- +$m = marker(7); +run_arm('arbitrary arg_key (injection via the name)', $m, function () use ($m) { + $local = fresh_local(); + $local->add_args('x$(touch ' . $m . ')', 'v'); + return $local->start_command(); +}); + +$m = marker(8); +run_arm('custom flag value, quote break-out', $m, function () use ($m) { + $local = fresh_local(); + $local->add_args('customFlag', "' \$(touch " . $m . ") '"); + return $local->start_command(); +}); + +// -- F-008: attacker-controlled $pid reaching the isRunning() shell call ------ +$arm++; +$m = marker(9); +$local = new BrowserStack\Local(); +$local->pid = 'aux$(touch ' . $m . ')'; +$local->isRunning(); +$fired = file_exists($m); +printf("ARM %d %-44s payload_executed=%s\n", $arm, 'public $pid -> isRunning()', $fired ? 'YES <-- VULNERABLE' : 'no'); +if ($fired) { $vulnerable++; unlink($m); } + +// -- stop_command() inherits the localIdentifier fragment --------------------- +$arm++; +$m = marker(10); +$local = fresh_local(); +$local->add_args('localIdentifier', 'x$(touch ' . $m . ')'); +shell_exec($local->stop_command() . ' 2>&1'); +$fired = file_exists($m); +printf("ARM %d %-44s payload_executed=%s\n", $arm, 'localIdentifier -> stop_command()', $fired ? 'YES <-- VULNERABLE' : 'no'); +printf(" command: %s\n", $local->stop_command()); +if ($fired) { $vulnerable++; unlink($m); } + +echo "\n"; +if ($vulnerable > 0) { + echo "RESULT: VULNERABLE - $vulnerable of $arm arms executed attacker-controlled commands.\n"; + exit(1); +} +echo "RESULT: OK - all $arm payloads were passed through as inert argv data.\n"; +exit(0); From 80d297b41b8f695e99d4d03439800eef9579c88d Mon Sep 17 00:00:00 2001 From: Sourav Kunda Date: Wed, 12 Aug 2026 18:21:11 +0530 Subject: [PATCH 2/3] Annotate the two remaining exec sinks for Semgrep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Semgrep's diff scan re-reports php.lang.security.exec-use on both lines, because the lines changed — the constructs themselves are pre-existing and are already among master's open findings. Both are now the mitigated versions, so they are annotated with the reason rather than left to fail the check: - isRunning(): the interpolated value is the intval() directly above, guarded > 0, so only digits can reach the shell. - start(): $call comes from start_command(), where every caller-supplied part is escapeshellarg()'d and every unquoted token is a fixed flag name. This is precisely the sink this change exists to make safe, and the regression tests pin it with payloads that fail on the pre-fix code. --- lib/Local.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/Local.php b/lib/Local.php index cff9b65..920144b 100644 --- a/lib/Local.php +++ b/lib/Local.php @@ -100,6 +100,9 @@ public function isRunning() { $pid = intval($this->pid); if ($pid <= 0) return False; + // $pid is the intval() above, guarded > 0, so the only bytes that can + // reach the shell here are digits. + // nosemgrep: php.lang.security.exec-use.exec-use $return_message = shell_exec("ps -" . $pid . " | wc -l"); if (intval($return_message) > 1) { @@ -170,8 +173,14 @@ public function start($arguments) { // quoted here too — the old single-quote wrapper was escapable. The Windows // branch additionally used a single-quoted PHP string, so it truncated a // file literally named '$this->logfile' instead of the configured one. + // nosemgrep: php.lang.security.exec-use.exec-use system("echo \"\" > " . self::esc($this->logfile)); $call = $call . " 2>&1"; + // $call comes from start_command(), where every caller-supplied part is + // escapeshellarg()'d and every unquoted token is a fixed flag name. This is + // the sink the whole change exists to make safe; tests/LocalTest.php pins + // that with payloads that fail on the pre-fix code. + // nosemgrep: php.lang.security.exec-use.exec-use $return_message = shell_exec($call); $data = json_decode($return_message,true); if ($data["state"] != "connected") { From 6b1703b4207048117d90db7a17643ff356cc5c3a Mon Sep 17 00:00:00 2001 From: Sourav Kunda Date: Wed, 12 Aug 2026 20:45:46 +0530 Subject: [PATCH 3/3] =?UTF-8?q?Quote=20the=20argument=20name=20instead=20o?= =?UTF-8?q?f=20rejecting=20it=20=E2=80=94=20drop=20the=20allowlist?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit rejected any argument name outside [A-Za-z0-9_-]+ with a LocalException. That conflated two different findings: - the NAME reaching the shell as code (CWE-78) — a real sink, and the one this change exists to close; - the binding forwarding unknown names to the binary at all (CWE-88) — not a shell issue, and a deliberate, documented feature of this library. escapeshellarg() on the name closes the first without touching the second. The shell strips the quotes, so `-myFlag` still arrives at the binary as the argv element `-myFlag` — verified for every name the library already forwarded, dashes included. Unknown names keep being forwarded exactly as before. That removes the only caller-visible behaviour change in this PR: nothing throws that did not throw before, and no name that worked stops working. Tests: test_rejects_an_injectable_argument_name becomes test_no_injection_via_argument_name (asserts the payload stays inert), plus test_unknown_argument_names_are_still_forwarded, which asserts the binary still receives `-` verbatim for names an allowlist would have rejected — including 'weird.name' and 'name with space'. 9 injection regression tests now, all 9 red on the pre-fix code. --- lib/Local.php | 26 +++++++----------- tests/LocalTest.php | 48 ++++++++++++++++++++-------------- tests/manual/injection-poc.php | 2 ++ 3 files changed, 40 insertions(+), 36 deletions(-) diff --git a/lib/Local.php b/lib/Local.php index 920144b..02b24ff 100644 --- a/lib/Local.php +++ b/lib/Local.php @@ -10,15 +10,6 @@ class Local { - /** - * Argument names are emitted as `-` flags straight into the command - * line, so the name itself is an injection vector (CWE-78). Only these - * characters may reach the shell as a flag name; anything else is rejected - * by add_args() rather than quoted, because a quoted flag name would not be - * a flag any more. - */ - const ARG_KEY_PATTERN = '/^[A-Za-z0-9_-]+$/'; - public $pid = NULL; /** @@ -113,12 +104,6 @@ public function isRunning() { } public function add_args($arg_key, $value = NULL) { - if (!is_string($arg_key) || !preg_match(self::ARG_KEY_PATTERN, $arg_key)) - throw new LocalException( - "Invalid BrowserStack Local argument name. Argument names may only " . - "contain letters, digits, '-' and '_'; got: " . var_export($arg_key, true) - ); - if ($arg_key == "key") $this->key = $value; elseif ($arg_key == "binaryPath") @@ -154,10 +139,17 @@ public function add_args($arg_key, $value = NULL) { $this->folder_path = $value; } elseif ($value !== NULL && strtolower((string) $value) == "true"){ - array_push($this->user_args, "-$arg_key"); + // The argument NAME is interpolated into the command line too, so it is a + // shell sink in its own right. Quoting it closes that without changing + // what the binary receives: the shell strips the quotes, so `-myFlag` + // still arrives as the argv element `-myFlag`. An unknown name keeps + // being forwarded to the binary exactly as before -- whether the binary + // should accept unknown flags at all is a separate question (CWE-88) and + // not this change's business. + array_push($this->user_args, self::esc("-$arg_key")); } else { - array_push($this->user_args, "-$arg_key " . self::esc($value)); + array_push($this->user_args, self::esc("-$arg_key") . " " . self::esc($value)); } } diff --git a/tests/LocalTest.php b/tests/LocalTest.php index b579567..3034070 100644 --- a/tests/LocalTest.php +++ b/tests/LocalTest.php @@ -65,8 +65,8 @@ public function test_custom_boolean_argument() { public function test_custom_keyval() { $this->bs_local->add_args("customKey1", "custom value1"); $this->bs_local->add_args("customKey2", "custom value2"); - $this->assertStringContainsString('-customKey1 \'custom value1\'',$this->bs_local->start_command()); - $this->assertStringContainsString('-customKey2 \'custom value2\'',$this->bs_local->start_command()); + $this->assertStringContainsString("'-customKey1' 'custom value1'",$this->bs_local->start_command()); + $this->assertStringContainsString("'-customKey2' 'custom value2'",$this->bs_local->start_command()); } public function test_set_proxy() { @@ -182,26 +182,36 @@ public function test_no_injection_via_pid_in_is_running() { $this->assertFalse($running, 'a non-numeric pid must not be reported as running'); } - public function test_rejects_an_injectable_argument_name() { - $marker = $this->marker('argkey'); - $thrown = false; - try { - $this->bs_local->add_args('x$(touch ' . $marker . ')', 'v'); - } catch (LocalException $e) { - $thrown = true; - } - $this->assertTrue($thrown, 'add_args() must reject an argument name that is not [A-Za-z0-9_-]+'); - $this->assertNotExecuted($marker, 'add_args() with an injectable argument name'); + public function test_no_injection_via_argument_name() { + // The argument NAME is interpolated into the command line as well, so it is + // its own shell sink. Quoting it is enough — the name is still forwarded to + // the binary, it just cannot reach the shell as code. + $marker = $this->marker('argname'); + $this->bs_local->binary_path = '/bin/echo'; + $this->bs_local->add_args('key', 'dummykey'); + $this->bs_local->add_args('x$(touch ' . $marker . ')', 'v'); + $command = $this->bs_local->start_command(); + shell_exec($command . ' 2>&1'); + $this->assertNotExecuted($marker, $command); } - public function test_argument_names_the_library_documents_are_still_accepted() { - // Regression guard on the argument-name gate: everything the README and the - // existing tests use must keep working, dashes included. - foreach (array('v', 'force', 'only', 'onlyAutomate', 'forcelocal', 'forceproxy', - '-forceproxy', '-hosts', 'customKey1', 'custom_key_2', 'a1') as $name) { + public function test_unknown_argument_names_are_still_forwarded() { + // Deliberately NOT an allowlist: quoting closes the shell sink without + // changing behaviour, so every name the library already forwarded keeps + // working — dashes included — and nothing throws. Whether the binary should + // accept unknown flags is a separate (CWE-88) question. + foreach (array('customKey1', 'custom_key_2', 'a1', '-forceproxy', '-hosts', + 'weird.name', 'name with space') as $name) { $local = new Local(); - $local->add_args($name, 'true'); - $this->assertNotEmpty($local->start_command(), "argument name '$name' must stay usable"); + $local->binary_path = '/bin/echo'; + $local->add_args($name, 'somevalue'); + $command = $local->start_command(); + // The name reaches the binary as one argv element, quoted, never as code. + $this->assertStringContainsString(escapeshellarg("-$name"), $command, + "argument name '$name' must still be forwarded"); + $argv = shell_exec($command . ' 2>&1'); + $this->assertStringContainsString("-$name", $argv, + "the binary must still receive '-$name' verbatim"); } } diff --git a/tests/manual/injection-poc.php b/tests/manual/injection-poc.php index 73fe1a4..84b3221 100644 --- a/tests/manual/injection-poc.php +++ b/tests/manual/injection-poc.php @@ -119,6 +119,8 @@ function fresh_local() { // -- F-003: the add_args() else-branch, via the KEY and via the VALUE --------- $m = marker(7); run_arm('arbitrary arg_key (injection via the name)', $m, function () use ($m) { + // The name is quoted rather than rejected: it is still forwarded to the + // binary, it just cannot reach the shell as code. $local = fresh_local(); $local->add_args('x$(touch ' . $m . ')', 'v'); return $local->start_command();