Reject "name=value" INI settings whose value contains a line-break character

Author: sebastianbergmann

src/Util/PHP/JobRunner.php

@@ -28,6 +28,7 @@
 use function is_resource;
 use function proc_close;
 use function proc_open;
+use function sprintf;
 use function str_contains;
 use function str_replace;
 use function str_starts_with;
@@ -321,6 +322,8 @@ private function cliIniOverrides(array $alreadySet): array
     /**
      * @param list<string> $settings
      *
+     * @throws PhpProcessException
+     *
      * @return list<string>
      */
     private function settingsToParameters(array $settings): array
@@ -329,23 +332,31 @@ private function settingsToParameters(array $settings): array
 
         foreach ($settings as $setting) {
             $buffer[] = '-d';
-            $buffer[] = $this->quoteSettingValue($setting);
+            $buffer[] = $this->processSettingValue($setting);
         }
 
         return $buffer;
     }
 
     /**
-     * Quotes the value portion of a "name=value" INI setting only when it
-     * contains characters PHP's INI parser would otherwise interpret as
-     * metacharacters (`;` starts a comment, `"` is a string delimiter).
+     * Rejects "name=value" INI settings whose value contains a line-break
+     * character. A newline cannot legitimately appear in a PHP INI value and
+     * would, if forwarded unchanged, be parsed by the child process as a
+     * directive separator — turning a single setting into an attacker-
+     * controlled sequence of directives.
+     *
+     * Otherwise quotes the value portion only when it contains characters
+     * PHP's INI parser would interpret as metacharacters (`;` starts a
+     * comment, `"` is a string delimiter).
      *
      * Quoting is avoided for plain values so that boolean keywords such as
      * `On` / `Off` keep their special INI semantics; wrapping them in quotes
      * turns them into the literal strings `"On"` / `"Off"` and breaks
      * settings like `output_buffering`.
+     *
+     * @throws PhpProcessException
      */
-    private function quoteSettingValue(string $setting): string
+    private function processSettingValue(string $setting): string
     {
         $parts = explode('=', $setting, 2);
 
@@ -355,6 +366,15 @@ private function quoteSettingValue(string $setting): string
 
         [$name, $value] = $parts;
 
+        if (str_contains($value, "\n") || str_contains($value, "\r")) {
+            throw new PhpProcessException(
+                sprintf(
+                    'PHP setting "%s" contains a line-break character, which is not permitted',
+                    $name,
+                ),
+            );
+        }
+
         if (!str_contains($value, ';') && !str_contains($value, '"')) {
             return $setting;
         }

tests/unit/Util/PHP/JobRunnerTest.php

@@ -155,4 +155,29 @@ public function testRunsJobInSeparateProcess(Result $expected, Job $job): void
         $this->assertSame($expected->stdout(), $result->stdout());
         $this->assertSame($expected->stderr(), $result->stderr());
     }
+
+    public function testRejectsPhpSettingValueContainingLineBreak(): void
+    {
+        $jobRunner = new JobRunner(
+            new ChildProcessResultProcessor(
+                new Facade,
+                $this->createStub(Emitter::class),
+                new PassedTests,
+                new CodeCoverage,
+            ),
+        );
+
+        $job = new Job(
+            <<<'EOT'
+<?php declare(strict_types=1);
+
+EOT,
+            phpSettings: ["highlight.string=foo\nauto_prepend_file=/tmp/evil.php"],
+        );
+
+        $this->expectException(PhpProcessException::class);
+        $this->expectExceptionMessage('PHP setting "highlight.string" contains a line-break character, which is not permitted');
+
+        $jobRunner->run($job);
+    }
 }