Skip to content

Commit 8cfd7df

Browse files
07souravkunda07souravkunda
committed
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 `-<name>` 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/<pid>/cmdline`. It can no longer inject, and moving it off the command line needs binary-side support -- tracked separately.
1 parent 55b79c4 commit 8cfd7df

7 files changed

Lines changed: 485 additions & 43 deletions

File tree

‎.github/workflows/php.yml‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
name: PHP
2+
3+
on:
4+
pull_request:
5+
branches: ["master", "main"]
6+
push:
7+
branches: ["master", "main"]
8+
9+
permissions:
10+
contents: read
11+
12+
jobs:
13+
test:
14+
name: lint + phpunit
15+
runs-on: ubuntu-latest
16+
# 7.4 rather than 8.x: lib/ sets properties dynamically, which PHP 8.2
17+
# deprecates, and the library's own floor is php >= 5.3.19. phpunit 9.6
18+
# supports 7.3+.
19+
container: php:7.4-cli
20+
steps:
21+
- uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
22+
23+
# Runs first and on its own: a syntax error must fail the build even if the
24+
# suite cannot boot.
25+
- name: Syntax check
26+
run: |
27+
for f in lib/*.php tests/*.php tests/manual/*.php; do
28+
[ -e "$f" ] || continue
29+
php -l "$f"
30+
done
31+
32+
# git + unzip are not in the official php image, and Composer needs one of
33+
# them to unpack downloaded packages (ext-zip is not built in either).
34+
- name: Install dependencies
35+
run: |
36+
apt-get update -qq && apt-get install -y -qq --no-install-recommends git unzip >/dev/null
37+
curl -sS https://getcomposer.org/installer | php
38+
php composer.phar install --no-interaction --no-progress
39+
40+
# Excludes @group network — those tests reach badssl.com and the real S3 host.
41+
- name: PHPUnit
42+
run: ./vendor/bin/phpunit --exclude-group network

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,4 @@ composer.phar
22
vendor/**
33
composer.lock
44
local.log
5+
.phpunit.result.cache

‎composer.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
"php": ">=5.3.19"
1414
},
1515
"require-dev": {
16-
"phpunit/phpunit": "4.6.*"
16+
"phpunit/phpunit": "^9.6"
1717
},
1818
"suggest": {
1919
"phpdocumentor/phpdocumentor": "2.*"

‎lib/Local.php‎

Lines changed: 107 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,46 @@
1010

1111
class Local {
1212

13+
/**
14+
* Argument names are emitted as `-<name>` flags straight into the command
15+
* line, so the name itself is an injection vector (CWE-78). Only these
16+
* characters may reach the shell as a flag name; anything else is rejected
17+
* by add_args() rather than quoted, because a quoted flag name would not be
18+
* a flag any more.
19+
*/
20+
const ARG_KEY_PATTERN = '/^[A-Za-z0-9_-]+$/';
21+
1322
public $pid = NULL;
14-
23+
24+
/**
25+
* Quote a value so the shell treats it as exactly one literal argument.
26+
* escapeshellarg() has been available since PHP 4, so this keeps the
27+
* library's declared floor of PHP >= 5.3.19.
28+
*/
29+
private static function esc($value) {
30+
return escapeshellarg((string) $value);
31+
}
32+
33+
/**
34+
* Join pre-escaped command fragments, dropping the empty ones.
35+
*
36+
* This replaces the old `preg_replace('/\s+/S', " ", $command)` collapse.
37+
* That collapse existed only to squeeze out the gaps left by unset flags,
38+
* but it rewrote whitespace *inside* quoted values too — which would now
39+
* corrupt legitimately escaped arguments (a value of "a b" would arrive at
40+
* the binary as "a b"). Filtering the parts achieves the same tidy command
41+
* line without touching the arguments themselves.
42+
*/
43+
private static function join_parts($parts) {
44+
$out = array();
45+
foreach ($parts as $part) {
46+
$part = trim((string) $part);
47+
if ($part !== "")
48+
$out[] = $part;
49+
}
50+
return implode(" ", $out);
51+
}
52+
1553
public function __construct() {
1654
$this->key = getenv("BROWSERSTACK_ACCESS_KEY");
1755
$this->logfile = getcwd() . "/local.log";
@@ -54,7 +92,15 @@ public function isRunning() {
5492
return False;
5593
}
5694
else {
57-
$return_message = shell_exec("ps -" . "$this->pid " . "| wc -l");
95+
// $pid is public and is also populated from the spawned process's stdout
96+
// (see start()), so it must never be concatenated into a shell string
97+
// raw. A PID is an integer by definition — cast, and treat anything that
98+
// is not a positive integer as "not running" rather than asking ps about
99+
// it.
100+
$pid = intval($this->pid);
101+
if ($pid <= 0)
102+
return False;
103+
$return_message = shell_exec("ps -" . $pid . " | wc -l");
58104
if (intval($return_message) > 1)
59105
{
60106
return True;
@@ -64,6 +110,12 @@ public function isRunning() {
64110
}
65111

66112
public function add_args($arg_key, $value = NULL) {
113+
if (!is_string($arg_key) || !preg_match(self::ARG_KEY_PATTERN, $arg_key))
114+
throw new LocalException(
115+
"Invalid BrowserStack Local argument name. Argument names may only " .
116+
"contain letters, digits, '-' and '_'; got: " . var_export($arg_key, true)
117+
);
118+
67119
if ($arg_key == "key")
68120
$this->key = $value;
69121
elseif ($arg_key == "binaryPath")
@@ -81,15 +133,15 @@ public function add_args($arg_key, $value = NULL) {
81133
elseif ($arg_key == "forcelocal")
82134
$this->force_local_flag = "-forcelocal";
83135
elseif ($arg_key == "localIdentifier")
84-
$this->local_identifier_flag = "-localIdentifier $value";
136+
$this->local_identifier_flag = "-localIdentifier " . self::esc($value);
85137
elseif ($arg_key == "proxyHost")
86-
$this->proxy_host = "-proxyHost $value";
138+
$this->proxy_host = "-proxyHost " . self::esc($value);
87139
elseif ($arg_key == "proxyPort")
88-
$this->proxy_port = "-proxyPort $value";
140+
$this->proxy_port = "-proxyPort " . self::esc($value);
89141
elseif ($arg_key == "proxyUser")
90-
$this->proxy_user = "-proxyUser $value";
142+
$this->proxy_user = "-proxyUser " . self::esc($value);
91143
elseif ($arg_key == "proxyPass")
92-
$this->proxy_pass = "-proxyPass $value";
144+
$this->proxy_pass = "-proxyPass " . self::esc($value);
93145
elseif ($arg_key == "forceproxy")
94146
$this->force_proxy_flag = "-forceproxy";
95147
elseif ($arg_key == "hosts")
@@ -98,11 +150,11 @@ public function add_args($arg_key, $value = NULL) {
98150
$this->folder_flag = "-f";
99151
$this->folder_path = $value;
100152
}
101-
elseif (strtolower($value) == "true"){
153+
elseif ($value !== NULL && strtolower((string) $value) == "true"){
102154
array_push($this->user_args, "-$arg_key");
103155
}
104156
else {
105-
array_push($this->user_args, "-$arg_key '$value'");
157+
array_push($this->user_args, "-$arg_key " . self::esc($value));
106158
}
107159
}
108160

@@ -114,11 +166,12 @@ public function start($arguments) {
114166
$this->binary_path = $this->binary->binary_path();
115167

116168
$call = $this->start_command();
117-
if(strtoupper(substr(PHP_OS, 0, 3)) === 'WIN')
118-
system('echo "" > '. '$this->logfile');
119-
else
120-
system("echo \"\" > '$this->logfile' ");
121-
$call = $call . "2>&1";
169+
// The logfile path is caller-supplied (add_args('logfile', ...)), so it is
170+
// quoted here too — the old single-quote wrapper was escapable. The Windows
171+
// branch additionally used a single-quoted PHP string, so it truncated a
172+
// file literally named '$this->logfile' instead of the configured one.
173+
system("echo \"\" > " . self::esc($this->logfile));
174+
$call = $call . " 2>&1";
122175
$return_message = shell_exec($call);
123176
$data = json_decode($return_message,true);
124177
if ($data["state"] != "connected") {
@@ -140,10 +193,38 @@ public function start_command() {
140193
if(strtoupper(substr(PHP_OS, 0, 3)) === 'WIN')
141194
$exec = "call";
142195

143-
$user_args = join(' ', $this->user_args);
144-
$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";
145-
$command = preg_replace('/\s+/S', " ", $command);
146-
return $command;
196+
// Every caller-supplied value is quoted so the shell sees it as one literal
197+
// argument. The fixed flag names and the $exec builtin are the only tokens
198+
// that stay unquoted, and none of them is caller-controlled. The flag
199+
// fragments built in add_args() are already escaped there.
200+
$parts = array($exec);
201+
if ((string) $this->binary_path !== "")
202+
$parts[] = self::esc($this->binary_path);
203+
$parts[] = "-d";
204+
$parts[] = "start";
205+
$parts[] = "-logFile";
206+
$parts[] = self::esc($this->logfile);
207+
$parts[] = $this->folder_flag;
208+
if ((string) $this->key !== "")
209+
$parts[] = self::esc($this->key);
210+
if ((string) $this->folder_path !== "")
211+
$parts[] = self::esc($this->folder_path);
212+
$parts[] = $this->force_local_flag;
213+
$parts[] = $this->local_identifier_flag;
214+
$parts[] = $this->only_flag;
215+
$parts[] = $this->only_automate_flag;
216+
$parts[] = $this->proxy_host;
217+
$parts[] = $this->proxy_port;
218+
$parts[] = $this->proxy_user;
219+
$parts[] = $this->proxy_pass;
220+
$parts[] = $this->force_proxy_flag;
221+
$parts[] = $this->force_flag;
222+
$parts[] = $this->verbose_flag;
223+
if ((string) $this->hosts !== "")
224+
$parts[] = self::esc($this->hosts);
225+
$parts = array_merge($parts, $this->user_args);
226+
227+
return self::join_parts($parts);
147228
}
148229

149230
public function stop_command() {
@@ -152,10 +233,14 @@ public function stop_command() {
152233
if(strtoupper(substr(PHP_OS, 0, 3)) === 'WIN')
153234
$exec = "call";
154235

155-
$user_args = join(' ', $this->user_args);
156-
$command = "$exec $this->binary_path -d stop $this->local_identifier_flag";
157-
$command = preg_replace('/\s+/S', " ", $command);
158-
return $command;
236+
$parts = array($exec);
237+
if ((string) $this->binary_path !== "")
238+
$parts[] = self::esc($this->binary_path);
239+
$parts[] = "-d";
240+
$parts[] = "stop";
241+
$parts[] = $this->local_identifier_flag;
242+
243+
return self::join_parts($parts);
159244
}
160245

161246
}

‎phpunit.xml‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
1-
<phpunit>
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
3+
xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.6/phpunit.xsd"
4+
bootstrap="vendor/autoload.php"
5+
colors="true">
26
<testsuites>
37
<testsuite name="local">
4-
<directory>tests</directory>
8+
<directory suffix="Test.php">tests</directory>
59
</testsuite>
610
</testsuites>
711
</phpunit>
8-

0 commit comments

Comments
 (0)