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

.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

.gitignore

@@ -2,3 +2,4 @@ composer.phar
 vendor/**
 composer.lock
 local.log
+.phpunit.result.cache

composer.json

@@ -13,7 +13,7 @@
     "php": ">=5.3.19"
   },
   "require-dev": {
-    "phpunit/phpunit": "4.6.*"
+    "phpunit/phpunit": "^9.6"
   },
   "suggest": {
     "phpdocumentor/phpdocumentor": "2.*"

lib/Local.php

@@ -11,7 +11,36 @@
 class Local {
 
   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 +83,18 @@
       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;
+      // $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)
       {
         return True;
@@ -81,15 +121,15 @@
     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 +138,18 @@
       $this->folder_flag = "-f";
       $this->folder_path = $value;
     }
-    elseif (strtolower($value) == "true"){
-      array_push($this->user_args, "-$arg_key");
+    elseif ($value !== NULL && strtolower((string) $value) == "true"){
+      // 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 '$value'");
+      array_push($this->user_args, self::esc("-$arg_key") . " " . self::esc($value));
     }
   }
 
@@ -114,11 +161,18 @@
     $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.
+    // 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") {
@@ -140,10 +194,38 @@
     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 +234,14 @@
     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);
   }
 
 }

phpunit.xml

@@ -1,8 +1,11 @@
-<phpunit>
+<?xml version="1.0" encoding="UTF-8"?>
+<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.6/phpunit.xsd"
+         bootstrap="vendor/autoload.php"
+         colors="true">
   <testsuites>
     <testsuite name="local">
-      <directory>tests</directory>
+      <directory suffix="Test.php">tests</directory>
     </testsuite>
   </testsuites>
 </phpunit>
-