From 10f05b65c4c84c21e203271301dc5187253c66dc Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 10 Aug 2019 23:10:54 +0200
Subject: [PATCH] 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: https://github.com/nodejs/node/pull/29124
PR-URL: https://github.com/nodejs/node/pull/29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 src/node_http2.cc                             | 16 ++++++++----
 src/node_revert.h                             |  1 +
 .../parallel/test-http2-zero-length-header.js | 25 +++++++++++++++++++
 3 files changed, 37 insertions(+), 5 deletions(-)
 create mode 100644 test/parallel/test-http2-zero-length-header.js

diff --git a/src/node_http2.cc b/src/node_http2.cc
index 0989bda18b8cb0..6ef0f5105b32bc 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -1328,6 +1328,8 @@ inline 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;
@@ -2021,6 +2023,7 @@ Http2Stream::~Http2Stream() {
   if (session_ == nullptr)
     return;
   DEBUG_HTTP2STREAM(this, "tearing down stream");
+  session_->DecrementCurrentSessionMemory(current_headers_length_);
   session_->RemoveStream(this);
   session_ = nullptr;
 
@@ -2032,6 +2035,7 @@ Http2Stream::~Http2Stream() {
 void Http2Stream::StartHeaders(nghttp2_headers_category category) {
   DEBUG_HTTP2STREAM2(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;
@@ -2323,10 +2327,6 @@ inline int Http2Stream::DoWrite(WriteWrap* req_wrap,
   return 0;
 }
 
-inline size_t GetBufferLength(nghttp2_rcbuf* buf) {
-  return nghttp2_rcbuf_get_buf(buf).len;
-}
-
 // Ads a header to the Http2Stream. Note that the header name and value are
 // provided using a buffer structure provided by nghttp2 that allows us to
 // avoid unnecessary memcpy's. Those buffers are ref counted. The ref count
@@ -2338,7 +2338,12 @@ inline bool Http2Stream::AddHeader(nghttp2_rcbuf* name,
   CHECK(!this->IsDestroyed());
   if (this->statistics_.first_header == 0)
     this->statistics_.first_header = uv_hrtime();
-  size_t length = GetBufferLength(name) + GetBufferLength(value) + 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) ||
@@ -2354,6 +2359,7 @@ inline 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
index 1bb381c2f108de..1cb3e996a9c4b5 100644
--- a/src/node_revert.h
+++ b/src/node_revert.h
@@ -18,6 +18,7 @@ namespace node {
 #define SECURITY_REVERSIONS(XX) \
   XX(CVE_2018_12116, "CVE-2018-12116", "HTTP request splitting")           \
   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
new file mode 100644
index 00000000000000..7b142d75f003b6
--- /dev/null
+++ 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();
+}));