http2: handle 0-length headers better

Ignore headers with 0-length names and track memory for headers the way we track it for other HTTP/2 session memory too. This is intended to mitigate CVE-2019-9516. Backport-PR-URL: #29123 PR-URL: #29122 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs · Oct 30, 2018 · Jan 30, 2019 · Mar 29, 2019 · Apr 18, 2019 · Jun 27, 2019
commit f4242e24f9f4fb185909f040cbd2dd889d79439b
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -1318,6 +1318,8 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
     return;
 
   std::vector<nghttp2_header> headers(stream->move_headers());
+  DecrementCurrentSessionMemory(stream->current_headers_length_);
+  stream->current_headers_length_ = 0;
 
   Local<String> name_str;
   Local<String> value_str;
@@ -1975,6 +1977,7 @@ Http2Stream::~Http2Stream() {
   if (session_ == nullptr)
     return;
   Debug(this, "tearing down stream");
+  session_->DecrementCurrentSessionMemory(current_headers_length_);
   session_->RemoveStream(this);
   session_ = nullptr;
 }
@@ -1989,6 +1992,7 @@ std::string Http2Stream::diagnostic_name() const {
 void Http2Stream::StartHeaders(nghttp2_headers_category category) {
   Debug(this, "starting headers, category: %d", id_, category);
   CHECK(!this->IsDestroyed());
+  session_->DecrementCurrentSessionMemory(current_headers_length_);
   current_headers_length_ = 0;
   current_headers_.clear();
   current_headers_category_ = category;
@@ -2260,8 +2264,12 @@ bool Http2Stream::AddHeader(nghttp2_rcbuf* name,
   CHECK(!this->IsDestroyed());
   if (this->statistics_.first_header == 0)
     this->statistics_.first_header = uv_hrtime();
-  size_t length = nghttp2_rcbuf_get_buf(name).len +
-                  nghttp2_rcbuf_get_buf(value).len + 32;
+  size_t name_len = nghttp2_rcbuf_get_buf(name).len;
+  if (name_len == 0 && !IsReverted(SECURITY_REVERT_CVE_2019_9516)) {
+    return true;  // Ignore headers with empty names.
+  }
+  size_t value_len = nghttp2_rcbuf_get_buf(value).len;
+  size_t length = name_len + value_len + 32;
   // A header can only be added if we have not exceeded the maximum number
   // of headers and the session has memory available for it.
   if (!session_->IsAvailableSessionMemory(length) ||
@@ -2277,6 +2285,7 @@ bool Http2Stream::AddHeader(nghttp2_rcbuf* name,
   nghttp2_rcbuf_incref(name);
   nghttp2_rcbuf_incref(value);
   current_headers_length_ += length;
+  session_->IncrementCurrentSessionMemory(length);
   return true;
 }
 

diff --git a/src/node_revert.h b/src/node_revert.h
@@ -17,6 +17,7 @@ namespace node {
 
 #define SECURITY_REVERSIONS(XX)                                            \
   XX(CVE_2019_9514, "CVE-2019-9514", "HTTP/2 Reset Flood")                 \
+  XX(CVE_2019_9516, "CVE-2019-9516", "HTTP/2 0-Length Headers Leak")       \
 //  XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title")
   // TODO(addaleax): Remove all of the above before Node.js 13 as the comment
   // at the start of the file indicates.

diff --git a/test/parallel/test-http2-zero-length-header.js b/test/parallel/test-http2-zero-length-header.js
@@ -0,0 +1,25 @@
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const http2 = require('http2');
+
+const server = http2.createServer();
+server.on('stream', (stream, headers) => {
+  assert.deepStrictEqual(headers, {
+    ':scheme': 'http',
+    ':authority': `localhost:${server.address().port}`,
+    ':method': 'GET',
+    ':path': '/',
+    'bar': '',
+    '__proto__': null
+  });
+  stream.session.destroy();
+  server.close();
+});
+server.listen(0, common.mustCall(() => {
+  const client = http2.connect(`http://localhost:${server.address().port}/`);
+  client.request({ ':path': '/', '': 'foo', 'bar': '' }).end();
+}));