LOC-6740: validate TLS chain and verify the binary before granting exec

.github/workflows/php.yml

@@ -0,0 +1,43 @@
+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: |
+          php -l lib/Local.php
+          php -l lib/LocalBinary.php
+          php -l lib/LocalException.php
+          php -l tests/LocalTest.php
+          php -l tests/LocalBinaryTest.php
+
+      # 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

README.md

@@ -134,12 +134,45 @@ To specify the path to file where the logs will be saved -
 $bs_local_args = array("key" => "<browserstack-accesskey>", "logfile" => "/browserstack/logs.txt");
 ```
 
+## Upgrading
+
+### TLS verification on the binary download
+
+Earlier releases downloaded the BrowserStack Local binary with TLS certificate
+verification disabled. **Verification is now enforced**, and the download also
+rejects a response that is not a valid binary rather than storing it.
+
+If your machine sits behind a TLS-inspecting proxy whose certificate authority is
+not in the system trust store, the download will now fail instead of silently
+succeeding, and you will see:
+
+```
+Download failed (cURL error 60): SSL certificate problem: self signed certificate in certificate chain
+```
+
+**Remedy:** install your organisation's CA certificate into the system trust store
+(libcurl honours it) — on Debian/Ubuntu, drop the PEM into
+`/usr/local/share/ca-certificates/` and run `update-ca-certificates`; on macOS, add
+it to the System keychain and mark it trusted; on Windows, import it into
+*Trusted Root Certification Authorities*. Alternatively, download the binary out of
+band and point the library at it with the `binaryPath` argument.
+
+We do not provide a switch to disable verification: it is what protects the binary
+you are about to execute from being substituted in transit.
+
 ## Contribute
 
 Testing is possible using [PHPUnit](https://phpunit.de/).
 
 To run the tests, run the command: `phpunit`
 
+`@group network` tests reach external hosts. To skip them:
+`phpunit --exclude-group network`.
+
+`tests/manual/php-poc.sh` is an end-to-end demonstration that the binary download
+refuses a substituted binary served over an untrusted TLS certificate. It needs
+`php`, `git`, `openssl` and `python3`, and is run by hand — not part of the suite.
+
 ### Reporting bugs
 
 You can submit bug reports either in the Github issue tracker.

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/LocalBinary.php

@@ -9,6 +9,14 @@
 
 class LocalBinary {
 
+  const DOWNLOAD_ATTEMPTS = 3;
+  const CONNECT_TIMEOUT = 10;
+  const DOWNLOAD_TIMEOUT = 300;
+
+  // Any real BrowserStackLocal build is tens of MB. A gateway error page or a
+  // truncated transfer is orders of magnitude smaller.
+  const MIN_BINARY_SIZE = 1048576;
+
   public function __construct() {
     $this->possible_binary_paths = array(
       $this->server_home() . "/.browserstack",
@@ -22,17 +30,18 @@
 
   public function binary_path() {
     $dest_parent_dir = $this->get_available_dirs();
-    $dest_binary_name = "BrowserStackLocal";
-    if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') {
-      $dest_binary_name = $dest_binary_name. ".exe";
-    }
-    $binary_path = $dest_parent_dir. "/". $dest_binary_name;
-    if(file_exists($binary_path)){
-      return $binary_path;
-    }
-    else {
-      return $this->download_binary($dest_parent_dir);
+    $binary_path = $dest_parent_dir. "/". $this->dest_binary_name();
+    if (file_exists($binary_path)) {
+      if ($this->verify_binary($binary_path)) {
+        $this->make_executable($binary_path);
+        return $binary_path;
+      }
+      // A cached file that is not a usable binary is discarded rather than
+      // executed — it may be a stored error page or a partial download.
+      // nosemgrep: php.lang.security.unlink-use.unlink-use -- basename is fixed by dest_binary_name(), no user input in the path
+      unlink($binary_path);
     }
+    return $this->download_binary($dest_parent_dir);
   }
 
   private function server_home() {
@@ -52,10 +61,20 @@
     return empty($home) ? NULL : $home;
   }
 
-  private function platform_url(){
+  private function is_windows() {
+    return strtoupper(substr(PHP_OS, 0, 3)) === 'WIN';
+  }
+
+  private function dest_binary_name() {
+    return $this->is_windows() ? "BrowserStackLocal.exe" : "BrowserStackLocal";
+  }
+
+  // protected so tests can point the download at a local fixture without
+  // patching installed source.
+  protected function platform_url(){
     if (PHP_OS == "Darwin")
       return 'https://s3.amazonaws.com/browserStack/browserstack-local/BrowserStackLocal-darwin-x64';
-    else if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN')
+    else if ($this->is_windows())
       return 'https://s3.amazonaws.com/browserStack/browserstack-local/BrowserStackLocal.exe';
     if ((strtoupper(PHP_OS)) == "LINUX") {
       if (PHP_INT_SIZE * 8 == 64)
@@ -67,27 +86,109 @@
 
   public function download_binary($path) {
     $url = $this->platform_url();
+    if (empty($url))
+      throw new LocalException("No BrowserStack Local binary is available for platform " . PHP_OS);
+
     if (!file_exists($path))
       mkdir($path, 0777, true);
 
+    $dest_binary_path = $path. '/'. $this->dest_binary_name();
+    $last_error = "unknown error";
 
-    $dest_binary_name = "BrowserStackLocal";
-    if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') {
-      $dest_binary_name = $dest_binary_name. ".exe";
+    for ($attempt = 1; $attempt <= self::DOWNLOAD_ATTEMPTS; $attempt++) {
+      try {
+        $this->fetch_binary($url, $dest_binary_path);
+        if ($this->verify_binary($dest_binary_path)) {
+          // Execute permission is granted only after the download has been
+          // verified, never before.
+          $this->make_executable($dest_binary_path);
+          return $dest_binary_path;
+        }
+        $last_error = "downloaded file is not a valid BrowserStackLocal binary";
+      }
+      catch (LocalException $e) {
+        $last_error = $e->getMessage();
+      }
+      // Never leave an unverified file behind for a later run to pick up and execute.
+      // nosemgrep: php.lang.security.unlink-use.unlink-use -- basename is fixed by dest_binary_name(), no user input in the path
+      if (file_exists($dest_binary_path))
+        unlink($dest_binary_path);
     }
-    $dest_binary_path = $path. '/'. $dest_binary_name;
-    $file = fopen($dest_binary_path , "w+");
-    $ch = curl_init("");
-    curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, false);
+
+    throw new LocalException("Error trying to download BrowserStack Local binary from " .
+      $url . " after " . self::DOWNLOAD_ATTEMPTS . " attempts. Last error: " . $last_error);
+  }
+
+  protected function fetch_binary($url, $dest_binary_path) {
+    $file = fopen($dest_binary_path, "w+");
+    if ($file === false)
+      throw new LocalException("Unable to open " . $dest_binary_path . " for writing");
+
+    $ch = curl_init();
     curl_setopt($ch, CURLOPT_URL, $url);
-    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
+    // The binary is executed on the user's machine, so the transport that
+    // delivers it must be authenticated: validate the certificate chain and
+    // that the certificate matches the host we asked for.
+    curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, true);
+    curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2);
+    curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
+    // A redirect must not be able to downgrade the transfer to plaintext.
+    curl_setopt($ch, CURLOPT_PROTOCOLS, CURLPROTO_HTTPS);
+    curl_setopt($ch, CURLOPT_REDIR_PROTOCOLS, CURLPROTO_HTTPS);
     curl_setopt($ch, CURLOPT_FILE, $file);
-    $data = curl_exec ($ch);
-    curl_close ($ch);
-
+    curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, self::CONNECT_TIMEOUT);
+    curl_setopt($ch, CURLOPT_TIMEOUT, self::DOWNLOAD_TIMEOUT);
+    $result = curl_exec($ch);
+    $curl_errno = curl_errno($ch);
+    $curl_error = curl_error($ch);
+    $http_status = curl_getinfo($ch, CURLINFO_HTTP_CODE);
+    curl_close($ch);
     fclose($file);
-    chmod($dest_binary_path, 0755);
-    return $dest_binary_path;
+
+    if ($result === false || $curl_errno !== 0)
+      throw new LocalException("Download failed (cURL error " . $curl_errno . "): " . $curl_error);
+
+    if ($http_status < 200 || $http_status >= 300)
+      throw new LocalException("Download failed with HTTP status " . $http_status);
+  }
+
+  // Confirms the bytes on disk are a platform executable of plausible size.
+  // This catches error pages, truncated transfers and empty files; it is not an
+  // authenticity check — that is the job of TLS chain validation in
+  // fetch_binary(). The file is deliberately not executed to test it.
+  protected function verify_binary($binary_path) {
+    // The retry loop stats the same path up to DOWNLOAD_ATTEMPTS times, so a
+    // cached stat entry from a previous attempt must not decide this one.
+    clearstatcache(true, $binary_path);
+    if (!is_file($binary_path))
+      return false;
+    if (filesize($binary_path) < self::MIN_BINARY_SIZE)
+      return false;
+
+    $handle = fopen($binary_path, "rb");
+    if ($handle === false)
+      return false;
+    $magic = fread($handle, 4);
+    fclose($handle);
+    if ($magic === false || strlen($magic) < 4)
+      return false;
+
+    if ($this->is_windows())
+      $valid = (substr($magic, 0, 2) === "MZ");
+    else if (PHP_OS == "Darwin")
+      // Mach-O 64/32-bit little-endian, plus the universal ("fat") header.
+      $valid = in_array($magic, array("\xcf\xfa\xed\xfe", "\xce\xfa\xed\xfe", "\xca\xfe\xba\xbe"), true);
+    else
+      $valid = ($magic === "\x7f" . "ELF");
+
+    return $valid;
+  }
+
+  private function make_executable($binary_path) {
+    clearstatcache(true, $binary_path);
+    if ($this->is_windows() || is_executable($binary_path))
+      return true;
+    return @chmod($binary_path, 0755);
   }
 
   private function get_available_dirs() {

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