Limit HTTP headers, respond with 431 "Entity Too Large"

[thekid]

This would prevent out of memory scenarios. Defaults vary, around 8K - 16K, see https://stackoverflow.com/questions/686217/maximum-on-http-header-values - however, 8K is easily reached by some of the newer JWT-cookies.

Motivation

Uncapped HTTP header size keeps the server exposed to attacks and can bring down its capacity to serve organic traffic.

• GHSA-2p2c-vwq7-7vg6
• https://nodejs.org/en/blog/vulnerability/november-2018-security-releases#denial-of-service-with-large-http-headers-cve-2018-12121
• Max header size of 8KB is a breaking change nodejs/node#24731
• Increase HTTP_MAX_HEADER_SIZE to 16kb nodejs/node#27645
• When http header size limit is reached, instead of a generic 400 response, return 431 Request Header Fields Too Large nodejs/node#25528
• Add an error message warning developers of oversized headers. meteor/meteor#10877
• https://cbonte.github.io/haproxy-dconv/configuration-1.5.html#3.2-tune.http.maxhdr

[thekid]

Note: Our HTTP server always runs behind a proxy in production scenarios, but we shouldn't solely rely on these blocking such requests.

[thekid]

First implementation:

diff --git a/src/main/php/xp/web/srv/HttpProtocol.class.php b/src/main/php/xp/web/srv/HttpProtocol.class.php
index 1303e2b..0cc6200 100755
--- a/src/main/php/xp/web/srv/HttpProtocol.class.php
+++ b/src/main/php/xp/web/srv/HttpProtocol.class.php
@@ -153,17 +153,27 @@ class HttpProtocol implements ServerProtocol {
         clearstatcache();
         \xp::gc();
       }
-    } else if (Input::CLOSE === $input->kind) {
-      $socket->close();
+      return;
+    }
+
+    // Handle request errors, closing the socket
+    if (Input::CLOSE === $input->kind) {
+      goto close;
+    } else if (Input::EXCESSIVE === $input->kind) {
+      $status= '431 Request Header Fields Too Large';
+      $error= 'Error processing status line and request headers';
     } else {
-      $error= 'Incomplete HTTP request: "'.addcslashes($input->kind, "\0..\37!\177..\377").'"';
-      $socket->write(sprintf(
-        "HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\nContent-Length: %d\r\nConnection: close\r\n\r\n%s",
-        strlen($error),
-        $error
-      ));
-      $socket->close();
+      $status= '400 Bad Request';
+      $error= 'Incomplete HTTP request: "'.addcslashes($input->buffer, "\0..\37!\177..\377").'"';
     }
+
+    $socket->write(sprintf(
+      "HTTP/1.1 %s\r\nContent-Type: text/plain\r\nContent-Length: %d\r\nConnection: close\r\n\r\n%s",
+      $status,
+      strlen($error),
+      $error
+    ));
+    close: $socket->close();
   }
 
   /**
diff --git a/src/main/php/xp/web/srv/Input.class.php b/src/main/php/xp/web/srv/Input.class.php
index 30fec44..862e3bd 100755
--- a/src/main/php/xp/web/srv/Input.class.php
+++ b/src/main/php/xp/web/srv/Input.class.php
@@ -6,13 +6,15 @@ use web\Headers;
 use web\io\{ReadChunks, ReadLength, Parts, Input as Base};
 
 class Input implements Base {
-  const CLOSE   = 0;
-  const REQUEST = 1;
+  const CLOSE      = 0;
+  const REQUEST    = 1;
+  const INCOMPLETE = 2;
+  const EXCESSIVE  = 3;
 
   public $kind;
+  public $buffer= null;
   private $socket;
   private $method, $uri, $version;
-  private $buffer= null;
   private $incoming= null;
 
   /**
@@ -29,21 +31,26 @@ class Input implements Base {
       return;
     }
 
-    // Read status line cautiously. If a client does not send complete line
-    // with the initial write (which it typically does), wait for another
-    // 100 milliseconds. If no more data is transmitted, give up.
-    if (false === ($p= strpos($initial, "\r\n"))) {
-      if ($socket->canRead(0.1)) {
-        $initial.= $socket->readBinary();
+    // Read status line and headers. Limit status line and headers to 16 kB,
+    // and the time taken to process headers to 500 milliseconds.
+    $start= microtime(true);
+    while (false === ($p= strpos($initial, "\r\n\r\n"))) {
+      $socket->canRead(0.05) && $initial.= $socket->readBinary();
+
+      if (strlen($initial) > 16384 || microtime(true) - $start > 0.5) {
+        $this->buffer= $initial;
+        $this->kind= self::EXCESSIVE;
+        return;
       }
     }
 
     if (3 === sscanf($initial, "%s %s HTTP/%[0-9.]\r\n", $this->method, $this->uri, $this->version)) {
-      $this->buffer= substr($initial, $p + 2);
+      $this->buffer= substr($initial, strpos($initial, "\r\n") + 2);
       $this->socket= $socket;
       $this->kind= self::REQUEST;
     } else {
-      $this->kind= rtrim($initial);
+      $this->buffer= $initial;
+      $this->kind= self::INCOMPLETE;
     }
   }
 
diff --git a/src/test/php/web/unittest/HttpProtocolTest.class.php b/src/test/php/web/unittest/HttpProtocolTest.class.php
index 13dfa71..fd0f6f8 100755
--- a/src/test/php/web/unittest/HttpProtocolTest.class.php
+++ b/src/test/php/web/unittest/HttpProtocolTest.class.php
@@ -2,7 +2,7 @@
 
 use io\streams\Streams;
 use peer\SocketException;
-use test\{Assert, Test, Values};
+use test\{Assert, AssertionFailed, Test, Values};
 use web\{Application, Environment, Logging};
 use xp\web\srv\{CannotWrite, HttpProtocol};
 
@@ -37,7 +37,7 @@ class HttpProtocolTest {
   private function assertHttp($expected, $out) {
     $actual= implode('', $out);
     if (!preg_match('#^'.$expected.'$#', $actual)) {
-      $this->fail('=~', $actual, $expected);
+      throw new AssertionFailed($actual.' =~ '.$expected);
     }
   }
 
diff --git a/src/test/php/web/unittest/server/InputTest.class.php b/src/test/php/web/unittest/server/InputTest.class.php
index 8335323..0f04116 100755
--- a/src/test/php/web/unittest/server/InputTest.class.php
+++ b/src/test/php/web/unittest/server/InputTest.class.php
@@ -56,8 +56,14 @@ class InputTest {
   }
 
   #[Test]
-  public function malformed_kind() {
-    Assert::equals('EHLO example.org', (new Input($this->socket("EHLO example.org\r\n")))->kind);
+  public function incomplete_request() {
+    Assert::equals(Input::INCOMPLETE, (new Input($this->socket("EHLO example.org\r\n\r\n")))->kind);
+  }
+
+  #[Test]
+  public function header_limit_exceeded() {
+    $headers= 'Cookie: excess='.str_repeat('x', 16384)."\r\n";
+    Assert::equals(Input::EXCESSIVE, (new Input($this->socket("GET / HTTP/1.1\r\n{$headers}\r\n")))->kind);
   }
 
   #[Test]

Is 500 milliseconds too high / too low?

[thekid]

See also https://stackoverflow.com/questions/686217/maximum-on-http-header-values

[thekid]

The default header timeout is 60 seconds in Nginx, see https://nginx.org/en/docs/http/ngx_http_core_module.html#client_header_timeout, and exceeding this limit yields an HTTP 408.

[thekid]

For Golang, see https://blog.cloudflare.com/exposing-go-on-the-internet/

[thekid]

Is 500 milliseconds too high / too low?

Worked around this by making header reading async. We now use the default socket timeout for reading headers.

[thekid]

Implemented in https://github.com/xp-forge/web/releases/tag/v3.8.0