Skip to content

Commit b46601b

Browse files
07souravkundaclaude
07souravkunda
and
claude
committed
LOC-6740: validate TLS chain and verify the binary before granting exec
download_binary() disabled certificate-chain validation (CURLOPT_SSL_VERIFYPEER => false), ignored cURL and HTTP errors, and chmod 0755'd whatever bytes came back. An on-path attacker who redirected s3.amazonaws.com could substitute the Local binary and get code execution as the developer or CI user that later calls Local::start(). - Enforce CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST => 2. - Restrict the transfer and any redirect to HTTPS so a redirect cannot downgrade to plaintext. - Raise LocalException on cURL failure or a non-2xx status instead of storing the response body. - Add verify_binary(): the file must be >= 1 MiB and carry the platform's executable magic (Mach-O / PE / ELF). Verified against the real darwin-x64, .exe, linux-x64, linux-ia32 and linux-arm64 artifacts on both s3.amazonaws.com and local-downloads.browserstack.com. The download is deliberately not executed to test it. - Grant 0755 only after verification passes, and delete the file on failure so a later run cannot pick up and execute an unverified download. A cached binary is verified too. - Bound the download with connect/total timeouts and retry 3 times. - platform_url() private -> protected so tests can point the download at a fixture without patching installed source. Note: this closes the transport-authenticity gap. Cryptographic integrity verification (a published SHA-256 per artifact) still needs a channel to publish the digests on -- see the ticket for that follow-up. Refs LOC-6740 (HackerOne #3695294) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 55b79c4 commit b46601b

2 files changed

Lines changed: 269 additions & 25 deletions

File tree

‎lib/LocalBinary.php‎

Lines changed: 120 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,14 @@
99

1010
class LocalBinary {
1111

12+
const DOWNLOAD_ATTEMPTS = 3;
13+
const CONNECT_TIMEOUT = 10;
14+
const DOWNLOAD_TIMEOUT = 300;
15+
16+
// Any real BrowserStackLocal build is tens of MB. A gateway error page or a
17+
// truncated transfer is orders of magnitude smaller.
18+
const MIN_BINARY_SIZE = 1048576;
19+
1220
public function __construct() {
1321
$this->possible_binary_paths = array(
1422
$this->server_home() . "/.browserstack",
@@ -22,17 +30,17 @@ public function __destruct() {
2230

2331
public function binary_path() {
2432
$dest_parent_dir = $this->get_available_dirs();
25-
$dest_binary_name = "BrowserStackLocal";
26-
if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') {
27-
$dest_binary_name = $dest_binary_name. ".exe";
28-
}
29-
$binary_path = $dest_parent_dir. "/". $dest_binary_name;
30-
if(file_exists($binary_path)){
31-
return $binary_path;
32-
}
33-
else {
34-
return $this->download_binary($dest_parent_dir);
33+
$binary_path = $dest_parent_dir. "/". $this->dest_binary_name();
34+
if (file_exists($binary_path)) {
35+
if ($this->verify_binary($binary_path)) {
36+
$this->make_executable($binary_path);
37+
return $binary_path;
38+
}
39+
// A cached file that is not a usable binary is discarded rather than
40+
// executed — it may be a stored error page or a partial download.
41+
unlink($binary_path);
3542
}
43+
return $this->download_binary($dest_parent_dir);
3644
}
3745

3846
private function server_home() {
@@ -52,10 +60,20 @@ private function server_home() {
5260
return empty($home) ? NULL : $home;
5361
}
5462

55-
private function platform_url(){
63+
private function is_windows() {
64+
return strtoupper(substr(PHP_OS, 0, 3)) === 'WIN';
65+
}
66+
67+
private function dest_binary_name() {
68+
return $this->is_windows() ? "BrowserStackLocal.exe" : "BrowserStackLocal";
69+
}
70+
71+
// protected so tests can point the download at a local fixture without
72+
// patching installed source.
73+
protected function platform_url(){
5674
if (PHP_OS == "Darwin")
5775
return 'https://s3.amazonaws.com/browserStack/browserstack-local/BrowserStackLocal-darwin-x64';
58-
else if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN')
76+
else if ($this->is_windows())
5977
return 'https://s3.amazonaws.com/browserStack/browserstack-local/BrowserStackLocal.exe';
6078
if ((strtoupper(PHP_OS)) == "LINUX") {
6179
if (PHP_INT_SIZE * 8 == 64)
@@ -67,27 +85,104 @@ private function platform_url(){
6785

6886
public function download_binary($path) {
6987
$url = $this->platform_url();
88+
if (empty($url))
89+
throw new LocalException("No BrowserStack Local binary is available for platform " . PHP_OS);
90+
7091
if (!file_exists($path))
7192
mkdir($path, 0777, true);
7293

94+
$dest_binary_path = $path. '/'. $this->dest_binary_name();
95+
$last_error = "unknown error";
7396

74-
$dest_binary_name = "BrowserStackLocal";
75-
if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') {
76-
$dest_binary_name = $dest_binary_name. ".exe";
97+
for ($attempt = 1; $attempt <= self::DOWNLOAD_ATTEMPTS; $attempt++) {
98+
try {
99+
$this->fetch_binary($url, $dest_binary_path);
100+
if ($this->verify_binary($dest_binary_path)) {
101+
// Execute permission is granted only after the download has been
102+
// verified, never before.
103+
$this->make_executable($dest_binary_path);
104+
return $dest_binary_path;
105+
}
106+
$last_error = "downloaded file is not a valid BrowserStackLocal binary";
107+
}
108+
catch (LocalException $e) {
109+
$last_error = $e->getMessage();
110+
}
111+
// Never leave an unverified file behind for a later run to pick up and execute.
112+
if (file_exists($dest_binary_path))
113+
unlink($dest_binary_path);
77114
}
78-
$dest_binary_path = $path. '/'. $dest_binary_name;
79-
$file = fopen($dest_binary_path , "w+");
80-
$ch = curl_init("");
81-
curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, false);
115+
116+
throw new LocalException("Error trying to download BrowserStack Local binary from " .
117+
$url . " after " . self::DOWNLOAD_ATTEMPTS . " attempts. Last error: " . $last_error);
118+
}
119+
120+
protected function fetch_binary($url, $dest_binary_path) {
121+
$file = fopen($dest_binary_path, "w+");
122+
if ($file === false)
123+
throw new LocalException("Unable to open " . $dest_binary_path . " for writing");
124+
125+
$ch = curl_init();
82126
curl_setopt($ch, CURLOPT_URL, $url);
83-
curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
127+
// The binary is executed on the user's machine, so the transport that
128+
// delivers it must be authenticated: validate the certificate chain and
129+
// that the certificate matches the host we asked for.
130+
curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, true);
131+
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 2);
132+
curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
133+
// A redirect must not be able to downgrade the transfer to plaintext.
134+
curl_setopt($ch, CURLOPT_PROTOCOLS, CURLPROTO_HTTPS);
135+
curl_setopt($ch, CURLOPT_REDIR_PROTOCOLS, CURLPROTO_HTTPS);
84136
curl_setopt($ch, CURLOPT_FILE, $file);
85-
$data = curl_exec ($ch);
86-
curl_close ($ch);
87-
137+
curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, self::CONNECT_TIMEOUT);
138+
curl_setopt($ch, CURLOPT_TIMEOUT, self::DOWNLOAD_TIMEOUT);
139+
$result = curl_exec($ch);
140+
$curl_errno = curl_errno($ch);
141+
$curl_error = curl_error($ch);
142+
$http_status = curl_getinfo($ch, CURLINFO_HTTP_CODE);
143+
curl_close($ch);
88144
fclose($file);
89-
chmod($dest_binary_path, 0755);
90-
return $dest_binary_path;
145+
146+
if ($result === false || $curl_errno !== 0)
147+
throw new LocalException("Download failed (cURL error " . $curl_errno . "): " . $curl_error);
148+
149+
if ($http_status < 200 || $http_status >= 300)
150+
throw new LocalException("Download failed with HTTP status " . $http_status);
151+
}
152+
153+
// Confirms the bytes on disk are a platform executable of plausible size.
154+
// This catches error pages, truncated transfers and empty files; it is not an
155+
// authenticity check — that is the job of TLS chain validation in
156+
// fetch_binary(). The file is deliberately not executed to test it.
157+
protected function verify_binary($binary_path) {
158+
if (!is_file($binary_path))
159+
return false;
160+
if (filesize($binary_path) < self::MIN_BINARY_SIZE)
161+
return false;
162+
163+
$handle = fopen($binary_path, "rb");
164+
if ($handle === false)
165+
return false;
166+
$magic = fread($handle, 4);
167+
fclose($handle);
168+
if ($magic === false || strlen($magic) < 4)
169+
return false;
170+
171+
if ($this->is_windows())
172+
$valid = (substr($magic, 0, 2) === "MZ");
173+
else if (PHP_OS == "Darwin")
174+
// Mach-O 64/32-bit little-endian, plus the universal ("fat") header.
175+
$valid = in_array($magic, array("\xcf\xfa\xed\xfe", "\xce\xfa\xed\xfe", "\xca\xfe\xba\xbe"), true);
176+
else
177+
$valid = ($magic === "\x7f" . "ELF");
178+
179+
return $valid;
180+
}
181+
182+
private function make_executable($binary_path) {
183+
if ($this->is_windows() || is_executable($binary_path))
184+
return true;
185+
return @chmod($binary_path, 0755);
91186
}
92187

93188
private function get_available_dirs() {

‎tests/LocalBinaryTest.php‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
<?php
2+
3+
namespace BrowserStack;
4+
5+
use BrowserStack\LocalBinary;
6+
use BrowserStack\LocalException;
7+
8+
require_once __DIR__ . '/../vendor/autoload.php';
9+
10+
/**
11+
* Test double that lets a test choose the download URL and reach the protected
12+
* verification helper. platform_url() is protected on LocalBinary precisely so
13+
* this is possible without patching installed source.
14+
*/
15+
class TestableLocalBinary extends LocalBinary {
16+
17+
private $url;
18+
19+
public function set_url($url) {
20+
$this->url = $url;
21+
}
22+
23+
protected function platform_url() {
24+
return $this->url;
25+
}
26+
27+
public function call_verify_binary($path) {
28+
return $this->verify_binary($path);
29+
}
30+
}
31+
32+
class LocalBinaryTest extends \PHPUnit_Framework_TestCase {
33+
34+
private $binary;
35+
private $dir;
36+
37+
public function setUp() {
38+
$this->binary = new TestableLocalBinary();
39+
$this->dir = sys_get_temp_dir() . '/bs-local-binary-test-' . getmypid() . '-' . mt_rand();
40+
mkdir($this->dir, 0777, true);
41+
}
42+
43+
public function tearDown() {
44+
foreach (glob($this->dir . '/*') as $file) {
45+
unlink($file);
46+
}
47+
if (is_dir($this->dir))
48+
rmdir($this->dir);
49+
}
50+
51+
private function dest_path() {
52+
$name = strtoupper(substr(PHP_OS, 0, 3)) === 'WIN' ? 'BrowserStackLocal.exe' : 'BrowserStackLocal';
53+
return $this->dir . '/' . $name;
54+
}
55+
56+
/**
57+
* LOC-6740: the download must refuse a server whose certificate chain does
58+
* not validate. Before the fix CURLOPT_SSL_VERIFYPEER was false, so this
59+
* returned a path to attacker-supplied bytes instead of raising.
60+
*
61+
* @group network
62+
*/
63+
public function test_download_rejects_untrusted_certificate() {
64+
$this->binary->set_url('https://self-signed.badssl.com/');
65+
$raised = null;
66+
try {
67+
$this->binary->download_binary($this->dir);
68+
}
69+
catch (LocalException $e) {
70+
$raised = $e;
71+
}
72+
$this->assertNotNull($raised, 'download_binary must reject an untrusted certificate');
73+
// cURL error 60 is CURLE_PEER_FAILED_VERIFICATION — pins the failure to
74+
// certificate validation rather than any later check.
75+
$this->assertContains('cURL error 60', $raised->getMessage());
76+
$this->assertFalse(file_exists($this->dest_path()), 'no file may be left behind on failure');
77+
}
78+
79+
/**
80+
* @group network
81+
*/
82+
public function test_download_rejects_hostname_mismatch() {
83+
$this->binary->set_url('https://wrong.host.badssl.com/');
84+
$raised = null;
85+
try {
86+
$this->binary->download_binary($this->dir);
87+
}
88+
catch (LocalException $e) {
89+
$raised = $e;
90+
}
91+
$this->assertNotNull($raised, 'download_binary must reject a certificate for another host');
92+
$this->assertFalse(file_exists($this->dest_path()));
93+
}
94+
95+
/**
96+
* A non-2xx response body must never be stored and made executable.
97+
*
98+
* @group network
99+
*/
100+
public function test_download_rejects_http_error_status() {
101+
$this->binary->set_url('https://s3.amazonaws.com/browserStack/browserstack-local/does-not-exist');
102+
$raised = null;
103+
try {
104+
$this->binary->download_binary($this->dir);
105+
}
106+
catch (LocalException $e) {
107+
$raised = $e;
108+
}
109+
$this->assertNotNull($raised, 'download_binary must reject a non-2xx response');
110+
$this->assertFalse(file_exists($this->dest_path()));
111+
}
112+
113+
public function test_verify_binary_rejects_error_page() {
114+
$path = $this->dir . '/error-page';
115+
file_put_contents($path, '<?xml version="1.0"?><Error><Code>AccessDenied</Code></Error>');
116+
$this->assertFalse($this->binary->call_verify_binary($path));
117+
}
118+
119+
public function test_verify_binary_rejects_empty_file() {
120+
$path = $this->dir . '/empty';
121+
file_put_contents($path, '');
122+
$this->assertFalse($this->binary->call_verify_binary($path));
123+
}
124+
125+
/**
126+
* A file of plausible size that is not a platform executable — e.g. a large
127+
* HTML interstitial from a captive portal — must still be rejected.
128+
*/
129+
public function test_verify_binary_rejects_large_non_executable() {
130+
$path = $this->dir . '/large-html';
131+
file_put_contents($path, '<html>' . str_repeat('a', 2 * 1024 * 1024) . '</html>');
132+
$this->assertFalse($this->binary->call_verify_binary($path));
133+
}
134+
135+
public function test_verify_binary_accepts_platform_executable() {
136+
if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN')
137+
$magic = "MZ\x90\x00";
138+
else if (PHP_OS == 'Darwin')
139+
$magic = "\xcf\xfa\xed\xfe";
140+
else
141+
$magic = "\x7f" . "ELF";
142+
143+
$path = $this->dir . '/fake-binary';
144+
file_put_contents($path, $magic . str_repeat("\x00", 2 * 1024 * 1024));
145+
$this->assertTrue($this->binary->call_verify_binary($path));
146+
}
147+
}
148+
149+
?>

0 commit comments

Comments
 (0)