Skip to content

Commit b6cda1f

Browse files
sebastianbergmannsebastianbergmann
committed
Verify child process result file with a random nonce
PHPUnit runs isolated tests by having the child process serialize a result object and write it to a temporary file under the system's directory for temporary files. The parent process then reads the file and calls unserialize() on its contents. The serialized payload contains Event, TestResult, PassedTests, and CodeCoverage instances, all types whose constructors and __wakeup() / __destruct() methods execute during deserialization. An attacker who can write to the temporary file between the child's exit and the parent's read can substitute a crafted serialized payload and achieve code execution in the parent PHPUnit process. This change defends against that write-after-exit race: * SeparateProcessTestRunner generates a 32-character hex nonce (bin2hex(random_bytes(16))) per isolated test, injects it into the child template as {processResultNonce}, and passes the same value to JobRunnerRegistry::runTestJob(). * The child template prepends the nonce to the serialized payload before writing it to the result file. * ChildProcessResultProcessor compares the file's prefix against the expected nonce with hash_equals(). A mismatch or short read is treated as tampering: childProcessErrored is emitted, the test is marked errored with an AssertionFailedError, and unserialize() is never called. On a match the prefix is stripped and processing continues as before. What this blocks: * A co-tenant on a shared CI runner (or any local process that can write to the temp directory) replacing the result file with a crafted serialized payload between the child's exit and the parent's read. Without the nonce, the payload cannot pass the hash_equals() check and is rejected before reaching unserialize(). * Accidental reuse of a stale result file from an unrelated process that happens to occupy the same temp path. What this does not block: * A compromised or attacker-controlled child process. The child must know the nonce in order to emit a valid result, so any code running inside the child (including malicious test code executed by an untrusted pull request on an unisolated CI job) can produce a payload that passes the prefix check. Closing that vector requires converting the wire format away from serialize() of live objects. * Attacks that do not involve swapping the result file, such as exploiting bugs in the Event subsystem itself once a legitimate payload has been deserialized. The nonce is a defence-in-depth measure against the local race, not a substitute for treating the serialized wire format as untrusted input.
1 parent e130965 commit b6cda1f

File tree

7 files changed

+102
-9
lines changed

7 files changed

+102
-9
lines changed

‎src/Framework/TestRunner/ChildProcessResultProcessor.php‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@
1010
namespace PHPUnit\Framework;
1111

1212
use function assert;
13+
use function hash_equals;
14+
use function strlen;
15+
use function substr;
1316
use function trim;
1417
use function unserialize;
1518
use PHPUnit\Event\Code\TestMethodBuilder;
@@ -37,7 +40,10 @@ public function __construct(Facade $eventFacade, Emitter $emitter, PassedTests $
3740
$this->codeCoverage = $codeCoverage;
3841
}
3942

40-
public function process(Test $test, string $serializedProcessResult, string $stderr): void
43+
/**
44+
* @param ?non-empty-string $processResultNonce
45+
*/
46+
public function process(Test $test, string $serializedProcessResult, string $stderr, ?string $processResultNonce = null): void
4147
{
4248
if ($stderr !== '') {
4349
$exception = new Exception(trim($stderr));
@@ -52,6 +58,35 @@ public function process(Test $test, string $serializedProcessResult, string $std
5258
return;
5359
}
5460

61+
if ($processResultNonce !== null && $serializedProcessResult !== '') {
62+
$nonceLength = strlen($processResultNonce);
63+
64+
if (strlen($serializedProcessResult) < $nonceLength ||
65+
!hash_equals($processResultNonce, substr($serializedProcessResult, 0, $nonceLength))) {
66+
$this->emitter->childProcessErrored();
67+
68+
$exception = new AssertionFailedError(
69+
'Test was run in child process and the result file was tampered with or written by an unexpected process',
70+
);
71+
72+
assert($test instanceof TestCase);
73+
74+
$this->emitter->testErrored(
75+
TestMethodBuilder::fromTestCase($test),
76+
ThrowableBuilder::from($exception),
77+
);
78+
79+
$this->emitter->testFinished(
80+
TestMethodBuilder::fromTestCase($test),
81+
0,
82+
);
83+
84+
return;
85+
}
86+
87+
$serializedProcessResult = substr($serializedProcessResult, $nonceLength);
88+
}
89+
5590
$childResult = @unserialize($serializedProcessResult);
5691

5792
if ($childResult === false) {

‎src/Framework/TestRunner/SeparateProcessTestRunner.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,11 @@
1010
namespace PHPUnit\Framework;
1111

1212
use function assert;
13+
use function bin2hex;
1314
use function defined;
1415
use function get_include_path;
1516
use function hrtime;
17+
use function random_bytes;
1618
use function serialize;
1719
use function sprintf;
1820
use function sys_get_temp_dir;
@@ -118,6 +120,7 @@ public function run(TestCase $test, bool $runEntireClass, bool $preserveGlobalSt
118120
$offset = hrtime();
119121
$serializedConfiguration = $this->saveConfigurationForChildProcess();
120122
$processResultFile = $this->pathForCachedSourceMap();
123+
$processResultNonce = bin2hex(random_bytes(16));
121124
$sourceMapFile = $this->sourceMapFileForChildProcess();
122125

123126
$file = $class->getFileName();
@@ -144,6 +147,7 @@ public function run(TestCase $test, bool $runEntireClass, bool $preserveGlobalSt
144147
'offsetNanoseconds' => (string) $offset[1],
145148
'serializedConfiguration' => $serializedConfiguration,
146149
'processResultFile' => $processResultFile,
150+
'processResultNonce' => $processResultNonce,
147151
'sourceMapFile' => $sourceMapFile,
148152
];
149153

@@ -157,7 +161,7 @@ public function run(TestCase $test, bool $runEntireClass, bool $preserveGlobalSt
157161

158162
assert($code !== '');
159163

160-
JobRunnerRegistry::runTestJob(new Job($code, requiresXdebug: $requiresXdebug), $processResultFile, $test);
164+
JobRunnerRegistry::runTestJob(new Job($code, requiresXdebug: $requiresXdebug), $processResultFile, $test, $processResultNonce);
161165

162166
@unlink($serializedConfiguration);
163167
}

‎src/Framework/TestRunner/templates/class.tpl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ function __phpunit_run_isolated_test()
104104

105105
file_put_contents(
106106
'{processResultFile}',
107-
serialize(
107+
'{processResultNonce}' . serialize(
108108
(object)[
109109
'testResult' => $test->result(),
110110
'codeCoverage' => {collectCodeCoverageInformation} ? CodeCoverage::instance()->codeCoverage() : null,

‎src/Framework/TestRunner/templates/method.tpl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ function __phpunit_run_isolated_test()
104104

105105
file_put_contents(
106106
'{processResultFile}',
107-
serialize(
107+
'{processResultNonce}' . serialize(
108108
(object)[
109109
'testResult' => $test->result(),
110110
'codeCoverage' => {collectCodeCoverageInformation} ? CodeCoverage::instance()->codeCoverage() : null,

‎src/Util/PHP/JobRunner.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,10 @@ public function __construct(ChildProcessResultProcessor $processor)
6060
}
6161

6262
/**
63-
* @param non-empty-string $processResultFile
63+
* @param non-empty-string $processResultFile
64+
* @param ?non-empty-string $processResultNonce
6465
*/
65-
public function runTestJob(Job $job, string $processResultFile, Test $test): void
66+
public function runTestJob(Job $job, string $processResultFile, Test $test, ?string $processResultNonce = null): void
6667
{
6768
$result = $this->run($job);
6869

@@ -80,6 +81,7 @@ public function runTestJob(Job $job, string $processResultFile, Test $test): voi
8081
$test,
8182
$processResult,
8283
$result->stderr(),
84+
$processResultNonce,
8385
);
8486

8587
EventFacade::emitter()->childProcessFinished($result->stdout(), $result->stderr());

‎src/Util/PHP/JobRunnerRegistry.php‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,12 @@ public static function run(Job $job): Result
3030
}
3131

3232
/**
33-
* @param non-empty-string $processResultFile
33+
* @param non-empty-string $processResultFile
34+
* @param ?non-empty-string $processResultNonce
3435
*/
35-
public static function runTestJob(Job $job, string $processResultFile, Test $test): void
36+
public static function runTestJob(Job $job, string $processResultFile, Test $test, ?string $processResultNonce = null): void
3637
{
37-
self::runner()->runTestJob($job, $processResultFile, $test);
38+
self::runner()->runTestJob($job, $processResultFile, $test, $processResultNonce);
3839
}
3940

4041
public static function set(JobRunner $runner): void

‎tests/unit/Framework/TestRunner/ChildProcessResultProcessorTest.php‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,57 @@ public function testEmitsErrorEventWhenProcessResultCannotBeUnserialized(): void
4646
$this->processor($emitter)->process(new Success('testOne'), '', '');
4747
}
4848

49+
#[TestDox('Emits Test\Errored event when process result nonce is shorter than the expected nonce')]
50+
public function testEmitsErrorEventWhenProcessResultIsShorterThanExpectedNonce(): void
51+
{
52+
$emitter = $this->createMock(Emitter::class);
53+
54+
$emitter
55+
->expects($this->once())
56+
->method('testErrored');
57+
58+
$this->processor($emitter)->process(
59+
new Success('testOne'),
60+
'too-short',
61+
'',
62+
'abcdef0123456789abcdef0123456789',
63+
);
64+
}
65+
66+
#[TestDox('Emits Test\Errored event when process result nonce does not match the expected nonce')]
67+
public function testEmitsErrorEventWhenProcessResultNonceDoesNotMatchExpectedNonce(): void
68+
{
69+
$emitter = $this->createMock(Emitter::class);
70+
71+
$emitter
72+
->expects($this->once())
73+
->method('testErrored');
74+
75+
$this->processor($emitter)->process(
76+
new Success('testOne'),
77+
'00000000000000000000000000000000payload',
78+
'',
79+
'abcdef0123456789abcdef0123456789',
80+
);
81+
}
82+
83+
#[TestDox('Emits Test\Errored event when process result contains only the expected nonce and no serialized data')]
84+
public function testEmitsErrorEventWhenProcessResultContainsOnlyExpectedNonceAndNoSerializedData(): void
85+
{
86+
$emitter = $this->createMock(Emitter::class);
87+
88+
$emitter
89+
->expects($this->once())
90+
->method('testErrored');
91+
92+
$this->processor($emitter)->process(
93+
new Success('testOne'),
94+
'abcdef0123456789abcdef0123456789',
95+
'',
96+
'abcdef0123456789abcdef0123456789',
97+
);
98+
}
99+
49100
private function processor(Emitter $emitter): ChildProcessResultProcessor
50101
{
51102
return new ChildProcessResultProcessor(

0 commit comments

Comments
 (0)