Comparing changes

…, 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.

Semgrep's diff scan re-reports php.lang.security.exec-use on both lines, because the lines changed — the constructs themselves are pre-existing and are already among master's open findings. Both are now the mitigated versions, so they are annotated with the reason rather than left to fail the check: - isRunning(): the interpolated value is the intval() directly above, guarded > 0, so only digits can reach the shell. - start(): $call comes from start_command(), where every caller-supplied part is escapeshellarg()'d and every unquoted token is a fixed flag name. This is precisely the sink this change exists to make safe, and the regression tests pin it with payloads that fail on the pre-fix code.

The previous commit rejected any argument name outside [A-Za-z0-9_-]+ with a LocalException. That conflated two different findings: - the NAME reaching the shell as code (CWE-78) — a real sink, and the one this change exists to close; - the binding forwarding unknown names to the binary at all (CWE-88) — not a shell issue, and a deliberate, documented feature of this library. escapeshellarg() on the name closes the first without touching the second. The shell strips the quotes, so `-myFlag` still arrives at the binary as the argv element `-myFlag` — verified for every name the library already forwarded, dashes included. Unknown names keep being forwarded exactly as before. That removes the only caller-visible behaviour change in this PR: nothing throws that did not throw before, and no name that worked stops working. Tests: test_rejects_an_injectable_argument_name becomes test_no_injection_via_argument_name (asserts the payload stays inert), plus test_unknown_argument_names_are_still_forwarded, which asserts the binary still receives `-<name>` verbatim for names an allowlist would have rejected — including 'weird.name' and 'name with space'. 9 injection regression tests now, all 9 red on the pre-fix code.

Navigation Menu

Uh oh!

Comparing changes

Open a pull request

Uh oh!

Commits on Aug 12, 2026

This comparison is taking too long to generate.

Uh oh!