Security fix: Reject newlines at end of header names

If a header name has multiple newlines at the end of its name, it causes the remaining headers to be pushed down into the body. Mitigate this using the `D` regex modifier. Thanks to Graham Campbell for the report and fix.
slimphp · Apr 11, 2023 · 4fea29e · 4fea29e
1 parent 5ccdd71
commit 4fea29e
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 2 deletions.
diff --git a/src/Headers.php b/src/Headers.php
@@ -266,7 +266,7 @@ protected function validateHeader($name, $value): void
      */
     protected function validateHeaderName($name): void
     {
-        if (!is_string($name) || preg_match("@^[!#$%&'*+.^_`|~0-9A-Za-z-]+$@", $name) !== 1) {
+        if (!is_string($name) || preg_match("@^[!#$%&'*+.^_`|~0-9A-Za-z-]+$@D", $name) !== 1) {
             throw new InvalidArgumentException('Header name must be an RFC 7230 compatible string.');
         }
     }
@@ -286,7 +286,7 @@ protected function validateHeaderValue($value): void
             );
         }
 
-        $pattern = "@^[ \t\x21-\x7E\x80-\xFF]*$@";
+        $pattern = "@^[ \t\x21-\x7E\x80-\xFF]*$@D";
         foreach ($items as $item) {
             $hasInvalidType = !is_numeric($item) && !is_string($item);
             $rejected = $hasInvalidType || preg_match($pattern, (string) $item) !== 1;

diff --git a/tests/HeadersTest.php b/tests/HeadersTest.php
@@ -209,4 +209,80 @@ public function testParseAuthorizationHeader()
         $headers = new Headers([], ['PHP_AUTH_DIGEST' => 'digest']);
         $this->assertEquals(['digest'], $headers->getHeader('Authorization'));
     }
+
+    /**
+     * @dataProvider provideInvalidHeaderNames
+     */
+    public function testWithInvalidHeaderName($headerName): void
+    {
+        $headers = new Headers();
+
+        $this->expectException(\InvalidArgumentException::class);
+
+        $headers->setHeader($headerName, 'foo');
+    }
+
+    public static function provideInvalidHeaderNames(): array
+    {
+        return [
+            [[]],
+            [false],
+            [new \stdClass()],
+            ["Content-Type\r\n\r\n"],
+            ["Content-Type\r\n"],
+            ["Content-Type\n"],
+            ["\r\nContent-Type"],
+            ["\nContent-Type"],
+            ["\n"],
+            ["\r\n"],
+            ["\t"],
+        ];
+    }
+
+    /**
+     * @dataProvider provideInvalidHeaderValues
+     */
+    public function testSetInvalidHeaderValue($headerValue)
+    {
+        $headers = new Headers();
+
+        $this->expectException(\InvalidArgumentException::class);
+
+        $headers->setHeader('Content-Type', $headerValue);
+    }
+
+    public static function provideInvalidHeaderValues(): array
+    {
+        // Explicit tests for newlines as the most common exploit vector.
+        $tests = [
+            ["new\nline"],
+            ["new\r\nline"],
+            ["new\rline"],
+            ["new\r\n line"],
+            ["newline\n"],
+            ["\nnewline"],
+            ["newline\r\n"],
+            ["\n\rnewline"],
+        ];
+
+        for ($i = 0; $i <= 0xff; $i++) {
+            if (\chr($i) == "\t") {
+                continue;
+            }
+            if (\chr($i) == " ") {
+                continue;
+            }
+            if ($i >= 0x21 && $i <= 0x7e) {
+                continue;
+            }
+            if ($i >= 0x80) {
+                continue;
+            }
+
+            $tests[] = ["foo" . \chr($i) . "bar"];
+            $tests[] = ["foo" . \chr($i)];
+        }
+
+        return $tests;
+    }
 }