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

[07souravkunda]

What

LocalBinary::download_binary() downloaded the BrowserStack Local binary with CURLOPT_SSL_VERIFYPEER => false, ignored cURL/HTTP errors, and chmod 0755'd whatever bytes came back. An on-path attacker who redirected s3.amazonaws.com could substitute the binary and get code execution as the developer or CI user who later calls Local::start().

Jira: LOC-6740 · HackerOne #3695294 · CWE-295 → CWE-494 → CWE-829

Changes — lib/LocalBinary.php

• TLS: enforce CURLOPT_SSL_VERIFYPEER + CURLOPT_SSL_VERIFYHOST => 2; restrict the transfer and any redirect to HTTPS so a redirect cannot downgrade to plaintext.
• Error handling: raise LocalException on cURL failure or non-2xx status instead of storing the response body.
• verify_binary(): the file must be ≥ 1 MiB and carry the platform's executable magic (Mach-O / PE / ELF). The download is deliberately not executed to test it.
• Ordering: chmod 0755 only after verification passes; delete the file on failure so a later run cannot pick up and execute an unverified download. Cached binaries are verified too.
• Connect/total timeouts, 3 download attempts.
• platform_url() private → protected so tests can point the download at a fixture without patching installed source (reporter mitigation force proxy #4).

Verification

• MITM reproduction at the libcurl layer (evidence/mitm-repro.sh in the session folder): self-signed cert for s3.amazonaws.com, payload served locally.
  • pre-fix (-k ≡ VERIFYPEER => false): 200, 67 bytes written, mode 755, payload executed as the local user
  • post-fix (default validation): cURL error 60 CURLE_PEER_FAILED_VERIFICATION, nothing written
  • control: the same validation still accepts the real S3 host (206, ELF magic 7f454c46)
• Magic-byte gate checked against every real artifact, on both s3.amazonaws.com and local-downloads.browserstack.com: darwin-x64 cffaedfe, .exe 4d5a9000, linux-x64 / linux-ia32 / linux-arm64 7f454c46; sizes 36–40 MB. The gate does not reject genuine binaries.
• Regression tests — tests/LocalBinaryTest.php: untrusted cert rejected (asserts cURL error 60), hostname mismatch rejected, non-2xx rejected, no file left behind in any failure case, plus offline verify_binary() cases (error page, empty, large non-executable, valid executable).

Important

PHP was not available in the authoring environment (no PHP runtime, no Docker; brew install php unavailable), so the PHPUnit suite and the PHP-level PoC have not been executed. The evidence above is libcurl-level and artifact-level. evidence/php-poc.sh is a ready-to-run 4-arm harness (pre-fix contrast / MITM negative / real-download positive / phpunit) — please run it before marking ready:

brew install php composer
cd <checkout> && composer install
bash php-poc.sh <checkout>

Follow-ups (not in this PR)

1. A new git tag is required for this to reach customers. Packagist still serves v1.1.0 (2016-09-19); it syncs from GitHub tags, so the fix has zero customer effect until a release is tagged.
2. Cryptographic integrity verification. Security asked for SHA-256 verification. There is no authenticated channel publishing digests today — POST local.browserstack.com/binary/api/v1/endpoint returns only data.endpoint, and no .sha256/.sig artifact exists next to the binaries. Publishing per-artifact digests there would let all wrappers verify; verify_binary() here is a corruption/error-page guard, not an authenticity check.
3. The hardcoded S3 path serves a four-year-stale binary (Last-Modified: 2022-05-12, deprecated Apple signing identity CL769MBK8C) while the current CDN serves 2026 builds. Tracked by LOC-5458 / Update binary url to cloudflare R2 #28; deliberately out of scope here to keep the security diff auditable.

🤖 Generated with Claude Code

[07souravkunda]

Automated security-fix review (locsec pipeline, round 0). 1 blocking, 3 nits, 1 for-human. Keeping this a Draft; a human owns approval.

The security change itself is correct and well-evidenced. I re-derived the load-bearing claims rather than taking them on trust:

• CVSS 7.5 HIGH recomputed independently from CVSS:3.1/AV:N/AC:H/PR:N/UI:R/S:U/C:H/I:H/A:H — ISS 0.914816 → Impact 5.8731, Exploitability 1.62015, roundup(7.49325) = 7.5 ✔ matches. The ticket's own CVSS/CVSS Vector fields really are blank as flagged.
• Pre-fix anchors verified on master — CURLOPT_SSL_VERIFYPEER, false at lib/LocalBinary.php:81 and the unconditional chmod(…, 0755) at :89, exactly as the assessment states. Call chain Local::start() → lib/Local.php:113 → binary_path() confirmed.
• Ruby half verified live — browserstack-local-ruby lib/browserstack/localbinary.rb:121 has OpenSSL::SSL::VERIFY_PEER, and rubygems.org serves 1.5.0. The "PHP was the only wrapper still carrying the primitive" claim holds.
• Diff matches the assessment on every point (VERIFYPEER/VERIFYHOST, HTTPS-pinned redirects, error handling, verify_binary() size+magic gate, chmod-after-verify, delete-on-failure, cached-binary verification, timeouts/retries, platform_url() → protected). Scoped to 2 files, no drive-by changes. No dependency bump, so no manifest/lockfile question.
• Not executing the download to test it, and labelling verify_binary() a corruption gate rather than an authenticity control, are both the right calls — and the un-closed SHA-256 half of security's ask is disclosed honestly with a correct reason (no authenticated digest channel exists today).

What blocks: the verification story, not the code. The 7 new tests have never run, nothing in the repo will ever run them (.github/workflows/ has only Semgrep.yml; .travis.yml is dead travis-ci.org on PHP 5.5/5.6/7.0), and the manual checklist meant to compensate is not executable by anyone — evidence/php-poc.sh is not in the repo or the PR, the command on LOC-6740 resolves to a path that does not exist, and brew install php + phpunit 4.6 cannot both succeed. For a HIGH-severity fix whose author could not even run php -l, that needs closing before merge. See the inline comment on tests/LocalBinaryTest.php for a ready-to-paste workflow.

For the human, not for another fix round: the download directory can still fall back to getcwd()//tmp, leaving the same CWE-494 primitive locally reachable (pre-existing, not widened here) — widen scope or file a follow-up. Also note follow-up #1 is load-bearing: merging alone changes nothing for customers, since Packagist still serves v1.1.0 (2016-09-19) and syncs from tags.

[07souravkunda]

[blocking] These 7 tests have never been executed, nothing in CI will ever execute them, and the manual checklist that was supposed to compensate cannot be run by anyone.

Evidence

1. Nothing in the repo runs this suite. .github/workflows/ contains only Semgrep.yml; .travis.yml targets PHP 5.5/5.6/7.0 on travis-ci.org (dead). There is no PHP job at all.
2. The suite was never executed locally either — the PR description states PHP was unavailable in the authoring environment, including php -l. A syntax error anywhere in the 145 changed lines of lib/LocalBinary.php would ship undetected.
3. The compensating handoff is not runnable. evidence/php-poc.sh is not in this repo, not in this diff, and not attached to the PR. The PR body says bash php-poc.sh <checkout> with no way to obtain the script. The corresponding command on LOC-6740 is ... && bash work/sec-pipeline/WI-30fccf3b:.../evidence/php-poc.sh /tmp/bslocal-php — a relative path evaluated after cd /tmp/bslocal-php, so it resolves to /tmp/bslocal-php/work/sec-pipeline/..., which does not exist; the machine-readable copy of the same step carries a literal <session> placeholder.
4. The prescribed setup cannot succeed as written. brew install php composer installs PHP 8.x, and composer.json pins phpunit/phpunit: 4.6.*, which will not install or boot on PHP 8 — the checklist itself notes this, but still prescribes brew install php + composer install. php@7.4 is EOL and no longer in homebrew-core, so there is no working path from the instructions as given.

Net: the only regression coverage for a HIGH-severity security fix is unexecuted, unrunnable, and unreproducible.

Fix — make the PR carry its own verification instead of relying on a private, unreproducible checklist:

1. Commit the PoC harness into the repo (e.g. tests/manual/php-poc.sh) so it travels with the change.
2. Add a minimal workflow so this is checked permanently:

# .github/workflows/php.yml
name: PHP
on: [push, pull_request]
jobs:
  test:
    runs-on: ubuntu-latest
    container: php:7.4-cli
    steps:
      - uses: actions/checkout@v4
      - run: php -l lib/LocalBinary.php && php -l tests/LocalBinaryTest.php
      - run: curl -sS https://getcomposer.org/installer | php && php composer.phar install
      - run: ./vendor/bin/phpunit --exclude-group network

At an absolute minimum, replace the placeholder command with a self-contained one that works on a clean machine, and pin the PHP version it requires.

[07souravkunda]

Fixed in d0f1b91 — all four points, both prescribed changes.

(1) + (2) Nothing ran the suite; no php -l either. Added .github/workflows/php.yml, php:7.4-cli, on push + PR. php -l runs first and as its own step over every file in lib/ and tests/ (not just the two changed ones), so a syntax error fails the build even if the suite cannot boot — that was the load-bearing gap. Then composer install, then phpunit --exclude-group network.

(4) The prescribed setup could not succeed. You are right, and it also blocked your own suggested job: phpunit 4.6.* predates PHP 7 entirely, so there is no PHP the CI image offers that it runs on. Bumped require-dev to 4.8.* — the last 4.x, the first with PHP 7 support, and still the PHPUnit_Framework_TestCase API that both test files use, so tests/LocalTest.php is unaffected. A bump past 4.x would have broken it.

(3) The handoff was unrunnable. Committed as tests/manual/php-poc.sh, so it travels with the change. It no longer takes a path argument at all — it locates the checkout via git rev-parse --show-toplevel from its own directory, which removes the relative-path and <session>-placeholder failure modes at the root rather than patching the wording. README.md documents it under Contribute.

I still cannot execute PHP, so I verified what was verifiable and am saying plainly which is which. Verified: the YAML parses (4 steps); bash -n on the harness; and the harness's sed rewrite of platform_url() replaces all 4 URLs with 0 leftover S3 URLs on both the pre-fix (private) and post-fix (protected) shape — so ARM 1 and ARM 2 genuinely target the local server; master resolves locally so ARM 1 runs. Not verified: whether phpunit 4.8 boots clean on 7.4, and the tests themselves. CI on this push is now the first real execution — if it goes red, that is the job doing its work, and I would rather that be visible than papered over.

I dropped a shellcheck step I had drafted: you did not ask for it, I have no shellcheck locally, and I was not willing to add an unverifiable gate to a security PR.

[07souravkunda]

Update — CI is green, and your finding surfaced two things I could not have found by reading. 803abb6.

Run: https://github.com/browserstack/browserstack-local-php/actions/runs/30534636453

No syntax errors detected in lib/Local.php
No syntax errors detected in lib/LocalBinary.php
No syntax errors detected in lib/LocalException.php
No syntax errors detected in tests/LocalTest.php
No syntax errors detected in tests/LocalBinaryTest.php
PHPUnit 9.6.35 by Sebastian Bergmann and contributors.
Tests: 16, Assertions: 18, Risky: 1.

The 145 lines are syntactically valid — that was point (2), and it is now closed by execution rather than by my hand-review.

Two things the first CI run caught, both of which vindicate the finding:

1. phpunit 4.8.* cannot be installed at all. Composer 2.10 refuses it: every 4.8.x release carries published security advisories (PKSA-z3gr-8qht-p93v, PKSA-w57n-mhp6-c9sd). So the job I added in d0f1b91 went red at dependency install. I did not reach for policy.advisories.ignore-id — suppressing a security advisory to make a security fix's CI pass is the wrong trade. Moved to phpunit ^9.6 instead and migrated both test files: PHPUnit_Framework_TestCase → PHPUnit\Framework\TestCase, : void on setUp/tearDown, assertContains() on strings → assertStringContainsString() (removed in 9), phpunit 9 config schema. require-dev only — the library's runtime floor stays php >= 5.3.19.

2. tests/LocalTest.php had three tests that can never pass on a runner. test_isRunning, test_checkPid and test_multiple_binary all call Local::start(), which downloads and executes the real binary and needs BROWSERSTACK_ACCESS_KEY. Tagged them @group network. Worth being explicit that this is not me hiding failures behind a skip: 16 tests / 18 assertions actually ran, which is exactly 12 of 15 in LocalTest plus 4 of 7 in LocalBinaryTest — i.e. all four offline verify_binary() cases (error page, empty, 2 MiB non-executable, valid executable) executed and passed, on Linux, exercising the ELF branch of the new gate. The three excluded LocalBinaryTest cases are the badssl.com / real-S3 ones.

Also added git + unzip to the job — neither is in the official php image and ext-zip is not built in, so Composer had no way to unpack packages.

Still not executed, and I am not claiming otherwise: the three @group network cases in LocalBinaryTest (the actual cert-rejection assertions) and tests/manual/php-poc.sh ARM 2/ARM 3. Those need outbound network and, for ARM 3, a real download; human_verify item 1 on LOC-6740 covers them and is now a runnable command. Risky: 1 is pre-existing — test_enable_force asserts nothing — left alone as unrelated to this PR.

[07souravkunda]

[nit] filesize() results are cached by PHP per-path for the life of the request, and the new retry loop calls this on the same path up to three times.

Scenario: attempt 1 stores a short S3 AccessDenied/503 body → verify_binary() stats the path, filesize() returns e.g. 243, gate fails → unlink() → attempt 2 downloads the genuine 36 MB binary → if the cached stat entry is still live, filesize() still reports 243, the gate rejects a perfectly good binary, it gets deleted, and after 3 attempts the user gets a hard LocalException on a working network.

What I could not confirm: whether PHP's automatic stat-cache invalidation on unlink() covers this. I have no PHP runtime available (the same wall the PR description hit), and the retry path has no test — so this is a question, not a confirmed defect.

Fix — one line makes it correct regardless of the answer, so it is cheaper than establishing which way the semantics fall:

protected function verify_binary($binary_path) {
    clearstatcache(true, $binary_path);
    if (!is_file($binary_path))
      return false;

Same reasoning applies to the is_executable() check in make_executable() (line 183), which is stat-cached too.

[07souravkunda]

Fixed in d0f1b91 — clearstatcache(true, $binary_path) at the top of verify_binary(), and the same in make_executable() for the stat-cached is_executable() you flagged.

Taking your framing: I could not establish PHP's invalidation semantics either (same missing runtime), and the one-line guarantee costs less than settling the question. The scenario you describe is the one that worried me too — a short error body poisoning the size check for the next attempt would turn a transient S3 blip into a hard LocalException on a working network, which is a worse failure than the one this PR fixes.

Worth noting the retry path still has no automated coverage — the existing tests exercise a single attempt. Recorded as a gap in the test report rather than papered over.

[07souravkunda]

[nit] This is a deliberate breaking change and it ships with no user-facing note anywhere.

The PR description and the assessment on LOC-6740 both record functional_impact: YES, and both justify it by pointing at the Ruby gem: "same break the Ruby gem's 1.5.0 CHANGELOG already announced." That comparison is the problem — the Ruby gem told its users. This repo has no CHANGELOG file at all, and this diff touches neither it nor README.md.

Why it bites harder here than in Ruby: the download path has no CURLOPT_PROXY and no custom-CA option, so a user behind a TLS-inspecting corporate proxy has exactly one remedy — install the corporate CA into the system trust store (which libcurl does honour). They go from silent success to a hard LocalException with no documented path forward. Whether that is a five-minute fix or a support ticket depends entirely on a note existing.

This is also time-sensitive relative to follow-up #1: the note should land before the release tag, since the tag is what actually delivers the break to composer require users.

Fix: add a short "Upgrading" section to README.md (and/or a CHANGELOG.md entry) naming three things — the new behaviour, the literal cURL error 60 / SSL certificate problem string users will see, and the trust-store remedy.

[07souravkunda]

Fixed in d0f1b91 — added an "Upgrading" section to README.md.

The Ruby comparison was doing work it had not earned, and you are right that it argued against me: the gem told its users, this repo had no CHANGELOG and I touched neither it nor the README. Your point about it being time-sensitive relative to the release tag is the part that decided it — the tag is what delivers the break, so the note has to precede it.

The section names the three things you asked for: the new behaviour, the literal Download failed (cURL error 60): SSL certificate problem… string users will actually see, and the trust-store remedy with per-OS steps (Debian/Ubuntu update-ca-certificates, macOS System keychain, Windows Trusted Root store), plus binaryPath as an out-of-band escape hatch. I also stated explicitly that there is deliberately no switch to disable verification, so nobody files that as a feature request.

I went with README over a new CHANGELOG.md: the repo has never had one, and starting one whose only entry is this change seemed less discoverable than the README a composer require user already reads.

[07souravkunda]

[for-human] Scope question — this PR closes the network substitution vector, but the same CWE-494 primitive remains locally reachable through the download directory, and that decision belongs to a human, not to another fix round.

Evidence: __construct() sets possible_binary_paths to $HOME/.browserstack, getcwd(), sys_get_temp_dir() (lines 21–25), and get_available_dirs() (line 188) returns the first that exists or can be created. When $HOME is unset or unwritable — a container, a CI runner with no HOME, a system daemon — the target becomes getcwd() or /tmp.

In a world-writable /tmp, a local attacker pre-plants BrowserStackLocal:

• Plant any ELF ≥ 1 MiB → verify_binary() passes (it is a format gate, not an authenticity check, as the comment on line 153 correctly says) → make_executable() chmods it 0755 → Local::start() executes it. The download never happens.
• Plant a symlink at that name → fetch_binary()'s fopen($dest_binary_path, "w+") (line 121) writes 36 MB through to the link target.

This is pre-existing, and this PR does not widen it — sys_get_temp_dir() and the unconditional chmod 0755 were both there before, and the new cached-file check on line 41 arguably narrows it, since a planted non-executable is now deleted instead of run. I am raising it because it is the same primitive in the same function this PR is hardening, and because the ticket's ask ("TLS validation and SHA verification") is broader than the network vector alone.

The call: widen scope here (e.g. drop sys_get_temp_dir()/getcwd() from the fallback list, or mkdir a 0700 per-user subdirectory and refuse a target that is world-writable or a symlink), or file it as a third follow-up next to the SHA-256 one. Either is defensible; it should not be decided silently.

[07souravkunda]

Not actioned in this round — escalating rather than deciding, which I think is what you are asking for. Recorded as follow-up #3 in the fix summary and the test report alongside the SHA-256 one, so it reaches the human with the ticket rather than dying in a PR thread.

Your read is correct on both the mechanism and the blast radius, and I verified the two facts it rests on: __construct() does fall back to getcwd() then sys_get_temp_dir(), and verify_binary() is explicitly a format gate, so a planted ≥1 MiB ELF passes it and make_executable() then chmods it 0755. The symlink case through fopen(..., "w+") is real too.

My reasoning for not widening here, offered as input to that decision rather than as the decision:

1. It is a different threat model — local attacker with write access to a shared directory, versus the on-path network attacker LOC-6740 is about. Fixing both in one diff makes the security review of the TLS change harder, and the TLS change is the one with the 31-July commitment.
2. Every candidate remedy is a behaviour change with real breakage risk. Dropping sys_get_temp_dir()/getcwd() breaks exactly the container and no-HOME CI cases you named — those users currently work, and would start hard-failing. A 0700 per-user subdirectory changes the cached-binary location, so every existing install re-downloads once. Both need functional testing across three platforms, and I have no PHP runtime to do it — shipping that untested next to the security fix is how you get a rollback that takes the security fix with it.
3. It is pre-existing and arguably narrowed by this PR, as you note: a planted non-executable is now deleted instead of run.

My recommendation for the follow-up, if it helps: mkdir a 0700 per-user directory and refuse a target that is a symlink or world-writable, sequenced after the release tag for this fix so the two changes can be reverted independently. Happy to implement it in a separate PR if the human agrees — it should not ride this one.

[07souravkunda]

[nit] This PR adds two net-new open code-scanning alerts — php.lang.security.unlink-use on this line and on line 41 (alerts #6 and #7, posted by github-advanced-security on this PR).

Both are false positives, and I checked rather than assumed: the unlink target is <dir>/BrowserStackLocal or <dir>/BrowserStackLocal.exe in both cases, with the basename produced by dest_binary_name() (line 67) — no user-controlled component in the filename, so no traversal. download_binary($path) is public and its $path is caller-supplied, but the basename is still fixed, so the worst a caller can do is delete a file they already named.

On the red semgrep/ci check: not a regression from this PR — that job fails identically on master on every scheduled run going back to at least 2026-07-20. Do not treat it as a gate here; it is separate repo hygiene.

Fix: dismiss both alerts as false-positive in the code-scanning UI, or annotate them so the dashboard for a security ticket does not accrue new open findings:

// nosemgrep: php.lang.security.unlink-use.unlink-use -- fixed basename from dest_binary_name(), no user input in path
unlink($dest_binary_path);

[07souravkunda]

Fixed in d0f1b91 — added the nosemgrep annotation you suggested to both unlink() calls, with the reason inline (basename fixed by dest_binary_name(), no user-controlled path component).

Your analysis matches mine: download_binary($path) is public and its $path is caller-supplied, but the basename is never attacker-influenced, so the worst a caller achieves is deleting a file they themselves named. Annotating rather than dismissing in the UI so the reasoning is in the diff and survives a re-scan.

Agreed on the red semgrep/ci check being pre-existing repo hygiene and not a gate for this PR.

[07souravkunda]

Automated security-fix review, round 1 (re-read from scratch on 803abb6). 2 blocking, 2 nits, 1 for-human. Still a Draft; a human owns approval.

Round-0 findings — verified individually, not assumed

Round 0	Status
[blocking] tests unexecuted, no CI, handoff unrunnable	Resolved in the repo. php.yml runs and is green — Tests: 16, Assertions: 18, all five files php -l clean. PoC committed at tests/manual/php-poc.sh, self-locating via git rev-parse --show-toplevel, no placeholders. phpunit 4.6 → ^9.6 + phpunit.xml bootstrap made the job installable — which was the specific blocker I named.
[nit] filesize() stat cache in the retry loop	Resolved. clearstatcache(true, $path) in both verify_binary() and make_executable().
[nit] breaking change with no user-facing note	Partly resolved. The README Upgrading section is good — exact cURL error 60 string, per-OS trust-store remedy. But it also adds a binaryPath escape hatch that does not work → new blocking finding.
[nit] two new Semgrep alerts	Not resolved. Both annotations added, both alerts still open on this head at the new line numbers. Root cause inline.
[for-human] /tmp download-target fallback	Unchanged, re-raised as for-human.

This is real progress and the security change itself remains correct — I re-confirmed the diff still matches the assessment, scope is clean, and CI is the first genuine execution of any of this code.

What blocks

1. The README's binaryPath remedy does not work. Local.php:114 overwrites the user's path with $this->binary->binary_path() unconditionally — this is exactly what open PR #27 exists to fix. It is the only option offered to someone who cannot install a CA into the system trust store, and it ships in the release that first enforces verification.
2. The posted locsec-fix-done comment on LOC-6740 was never updated. The ticket still holds 16 comments with no edit to 2247851. It still says "no PHP CI in the repo … nothing downstream will run them either", "PHP syntax was not linted", phpunit 4.6, and the <session>-placeholder command. The local drafts were updated; the record a human reads was not — so the reader is understating the work as well as following a broken path.

Worth knowing, not gating

CI is green but excludes the three tests that actually pin the CVE — the certificate/hostname/HTTP-status cases are all @group network, so what runs is the offline format gate (11ms, zero network). The committed harness already shows how to make the certificate assertion hermetic.

One more, below the finding cap: require-dev now needs PHP ≥ 7.3 (phpunit ^9.6) while require still declares php >= 5.3.19, so contributors on the library's own declared floor can no longer composer install the dev deps. CI pins 7.4 so nothing breaks — but the two constraints now disagree, and the test-report draft still describes the bump as 4.8.*.

Unchanged from round 0 and still the thing that decides customer impact: merging alone changes nothing — Packagist serves v1.1.0 (2016-09-19) and syncs from tags.

[07souravkunda]

[blocking] This escape hatch does not work on this branch, and it is the only remedy offered to a user who cannot modify the system trust store.

Evidence — binaryPath is accepted and then immediately discarded:

// lib/Local.php:66-70  — add_args() stores it
elseif ($arg_key == "binaryPath")
  $this->binary_path = $value;

// lib/Local.php:112-114 — start() overwrites it unconditionally
$this->binary = new LocalBinary();
$this->binary_path = $this->binary->binary_path();   // ← the user's path is gone

LocalBinary is constructed with no arguments and has no way to receive the caller's path, so binary_path() goes down the download path regardless. For the exact user this README section addresses — behind a TLS-inspecting proxy — that download now raises LocalException, so following this advice leaves them with the same hard failure they started with.

This is a known open bug, not my inference: PR #27 ("bug: BrowserStack\Local does not pass along binary_path to BrowserStack\LocalBinary") exists to fix it and is still open.

Why this is blocking rather than a doc nit: a user with no admin rights on a locked-down machine cannot install a CA into the system trust store, so binaryPath is their only listed option. Shipping it as a remedy sends them down a path that cannot work, and this text lands in the release that first enforces verification.

Fix — pick one:

1. Drop the sentence, and instead point at pinning the previous release (composer require browserstack/browserstack-local:<old>) as the stopgap.
2. Land the one-line binaryPath plumbing here (or merge bug: BrowserStack\Local does not pass along binary_path to BrowserStack\LocalBinary #27 first) and keep the sentence — then it is true.

If you take (2), please also add a test that start(array('binaryPath' => …)) does not download, since nothing currently covers it.

[07souravkunda]

[blocking] Committing this harness fixes the repo, but the record a human actually acts on was not updated — the locsec-fix-done comment on LOC-6740 is unchanged since 09:58 UTC and is now false in five places.

I checked the ticket directly: it still holds 16 comments, with no follow-up and no edit to comment 2247851. It currently tells the reviewer:

Posted claim	Reality after round 1
"there is also no PHP CI in the repo (travis is dead, only Semgrep runs), so nothing downstream will run them either"	.github/workflows/php.yml exists and is green — 16 tests, 18 assertions
"PHP syntax was not linted"	CI runs php -l over all five lib/ and tests/ files
"not_tested": "PHPUnit suite, PHP-level PoC, php -l syntax check, …"	suite and php -l both run in CI now
bash <session>/evidence/php-poc.sh and bash work/sec-pipeline/WI-30fccf3b:…/evidence/php-poc.sh /tmp/bslocal-php	superseded by in-repo tests/manual/php-poc.sh, which takes no argument
"note: phpunit 4.6 will not boot on PHP 8 — use PHP 7.4"	composer.json now pins ^9.6

The local drafts under the fix session were updated; the Jira comment was not. A reader following the posted instructions still hits the broken path that was the original blocking finding, and — worse — is told the fix has no automated coverage when it now does, which understates the work.

Fix: edit comment 2247851 in place (or post a short follow-up marked as superseding it) so test_deferred_reason, not_tested, and all three human_verify commands match the branch. The one command that matters reduces to:

git clone https://github.com/browserstack/browserstack-local-php.git /tmp/bslocal-php
cd /tmp/bslocal-php && git fetch origin locsec/WI-30fccf3b master
git checkout locsec/WI-30fccf3b && git branch -f master origin/master
curl -sS https://getcomposer.org/installer | php && php composer.phar install
tests/manual/php-poc.sh

Two notes on the harness itself, both fine as-is: ARM 1 silently SKIPPEDs without a local master ref (the git branch -f above covers it), and ARM 3 needs outbound network. Worth stating in the ticket so a skipped arm is not read as a pass.

[07souravkunda]

[nit] The new CI is real and green, but the three tests that actually pin the CVE fix are the exact three it excludes.

Evidence — from run 30534636453: Tests: 16, Assertions: 18, Risky: 1 in 00:00.011. Eleven milliseconds confirms zero network I/O. The arithmetic: LocalTest 15 − 3 network + LocalBinaryTest 7 − 3 network = 16. So the four cases that ran from this file are the offline verify_binary() ones (error page, empty, large non-executable, valid magic).

The certificate assertions — test_download_rejects_untrusted_certificate (which is the one that asserts cURL error 60 and therefore pins the failure to chain validation), test_download_rejects_hostname_mismatch, and test_download_rejects_http_error_status — are all @group network and never execute in CI. Net: CI verifies the format gate, and the TLS control that closes LOC-6740 is covered only by the round-0 libcurl repro and the manual harness. Tagging them @group network is correct given they reach badssl.com; the gap is that nothing hermetic replaces them.

Fix — tests/manual/php-poc.sh already contains the recipe for doing this without external network: a local python3 HTTPS server with a self-signed cert, which TestableLocalBinary::set_url() can be pointed at. Lifting that into a PHPUnit fixture would let the certificate assertion run on every push with no external dependency:

public function test_download_rejects_untrusted_certificate_locally() {
  $srv = $this->startSelfSignedServer();          // python3 -c …, bind 127.0.0.1:0
  $this->binary->set_url("https://127.0.0.1:{$srv['port']}/BrowserStackLocal-linux-x64");
  // …assert LocalException, 'cURL error 60', and no leftover file
}

Skip it with markTestSkipped() when python3 is absent so it degrades cleanly. Not gating the merge — the control is proven, just not by the automation.

Unrelated, while here: test_enable_force is flagged risky ("did not perform any assertions"). Pre-existing, but you are already touching this suite, so a one-line assertion on start_command() would clear the only warning in the run.

[07souravkunda]

[nit] Both nosemgrep annotations are ineffective — I verified against the API rather than assuming, and alerts #6 and #7 are still open on this head.

GET /repos/browserstack/browserstack-local-php/code-scanning/alerts?ref=refs/pull/29/merge
7  open  php.lang.security.unlink-use.unlink-use  lib/LocalBinary.php  115
6  open  php.lang.security.unlink-use.unlink-use  lib/LocalBinary.php  42

Those line numbers are the post-round-1 layout (they were 41 and 113 before the annotations were inserted), so the scan did re-run on the new code and still reports both.

Two separate causes:

1. The trailing prose breaks rule-ID matching (affects both sites). Semgrep parses everything after nosemgrep: as a comma-separated rule-ID list. Yours becomes php.lang.security.unlink-use.unlink-use -- basename is fixed by dest_binary_name() plus no user input in the path — neither is a valid ID, so nothing is suppressed. The reason has to live outside the directive.
2. Site 2 is on the wrong line. The directive at line 113 applies to line 114 (if (file_exists(...))), but the finding is on line 115 (unlink(...)).

Fix — justification on its own comment line, bare directive immediately above the flagged statement:

      // Never leave an unverified file behind for a later run to pick up and execute.
      // Path basename is fixed by dest_binary_name(); no user-controlled component.
      if (file_exists($dest_binary_path))
        // nosemgrep: php.lang.security.unlink-use.unlink-use
        unlink($dest_binary_path);

Same shape for line 41/42. Still cosmetic — both findings are genuine false positives — but worth getting right so a security ticket does not leave two open alerts behind. And to repeat from round 0 so it is not misread as a regression: the red semgrep/ci check fails identically on master on every scheduled run and is not caused by this PR.

[07souravkunda]

[for-human] Carried forward from round 0, unchanged in this diff and still a human scope call — not a defect to fix in another round.

possible_binary_paths (lines 21–25) is still $HOME/.browserstack, getcwd(), sys_get_temp_dir(), and get_available_dirs() still returns the first that exists or can be created. When $HOME is unset or unwritable — a container, a CI runner with no HOME, a system daemon — the download target becomes getcwd() or /tmp.

In a world-writable /tmp, a local attacker can pre-plant BrowserStackLocal: any ELF ≥ 1 MiB satisfies verify_binary() (correctly documented at line 155 as a format gate, not an authenticity check), make_executable() grants 0755, and Local::start() executes it — the download never happens. Or plant a symlink, and fetch_binary()'s fopen($dest_binary_path, "w+") at line 122 writes through to the link target.

Pre-existing and not widened by this PR — sys_get_temp_dir() and the unconditional chmod 0755 both predate it, and the new cached-file check at line 42 arguably narrows it, since a planted non-executable is now deleted rather than run. I am re-raising it only because it is the same CWE-494 primitive in the function this PR hardens, and security's ask on the ticket is broader than the network vector.

The call: widen scope here (drop getcwd()/sys_get_temp_dir() from the fallback, or mkdir a 0700 per-user directory and refuse a world-writable or symlinked target), or file it as a fourth follow-up beside the SHA-256 one. Either is defensible — it just should not pass silently a second time.