From e876c0c308bb98ed08ec9cae9f04a65a48517be5 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 1 Jul 2020 01:20:09 +0200
Subject: [PATCH] http2: add support for sensitive headers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Add support for “sensitive”/“never-indexed” HTTP2 headers.

Fixes: https://github.com/nodejs/node/issues/34091

PR-URL: https://github.com/nodejs/node/pull/34145
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
---
 doc/api/http2.md                              | 41 +++++++++++++++-
 lib/http2.js                                  |  2 +
 lib/internal/http2/core.js                    | 11 ++++-
 lib/internal/http2/util.js                    | 20 ++++++--
 src/node_http2.cc                             | 21 ++++++---
 src/node_http2.h                              |  1 -
 src/node_http_common-inl.h                    |  8 +++-
 src/node_http_common.h                        |  2 +
 src/quic/node_quic_http3_application.h        |  1 -
 test/parallel/test-http2-sensitive-headers.js | 47 +++++++++++++++++++
 test/parallel/test-http2-util-headers-list.js | 37 +++++++++++----
 .../parallel/test-http2-zero-length-header.js |  3 +-
 12 files changed, 167 insertions(+), 27 deletions(-)
 create mode 100644 test/parallel/test-http2-sensitive-headers.js

diff --git a/doc/api/http2.md b/doc/api/http2.md
index c24e43f12b0c6b..122d9deb517974 100644
--- a/doc/api/http2.md
+++ b/doc/api/http2.md
@@ -2461,6 +2461,17 @@ added: v8.4.0
 Returns a [HTTP/2 Settings Object][] containing the deserialized settings from
 the given `Buffer` as generated by `http2.getPackedSettings()`.
 
+### `http2.sensitiveHeaders`
+<!-- YAML
+added: REPLACEME
+-->
+
+* {symbol}
+
+This symbol can be set as a property on the HTTP/2 headers object with an array
+value in order to provide a list of headers considered sensitive.
+See [Sensitive headers][] for more details.
+
 ### Headers object
 
 Headers are represented as own-properties on JavaScript objects. The property
@@ -2509,6 +2520,33 @@ server.on('stream', (stream, headers) => {
 });
 ```
 
+<a id="http2-sensitive-headers"></a>
+#### Sensitive headers
+
+HTTP2 headers can be marked as sensitive, which means that the HTTP/2
+header compression algorithm will never index them. This can make sense for
+header values with low entropy and that may be considered valuable to an
+attacker, for example `Cookie` or `Authorization`. To achieve this, add
+the header name to the `[http2.sensitiveHeaders]` property as an array:
+
+```js
+const headers = {
+  ':status': '200',
+  'content-type': 'text-plain',
+  'cookie': 'some-cookie',
+  'other-sensitive-header': 'very secret data',
+  [http2.sensitiveHeaders]: ['cookie', 'other-sensitive-header']
+};
+
+stream.respond(headers);
+```
+
+For some headers, such as `Authorization` and short `Cookie` headers,
+this flag is set automatically.
+
+This property is also set for received headers. It will contain the names of
+all headers marked as sensitive, including ones marked that way automatically.
+
 ### Settings object
 <!-- YAML
 added: v8.4.0
@@ -3696,5 +3734,6 @@ following additional properties:
 [`tls.TLSSocket`]: tls.html#tls_class_tls_tlssocket
 [`tls.connect()`]: tls.html#tls_tls_connect_options_callback
 [`tls.createServer()`]: tls.html#tls_tls_createserver_options_secureconnectionlistener
-[error code]: #error_codes
 [`writable.writableFinished`]: stream.html#stream_writable_writablefinished
+[error code]: #error_codes
+[Sensitive headers]: #http2-sensitive-headers
diff --git a/lib/http2.js b/lib/http2.js
index c3ecd82072ee53..14b4f57acdabd1 100644
--- a/lib/http2.js
+++ b/lib/http2.js
@@ -8,6 +8,7 @@ const {
   getDefaultSettings,
   getPackedSettings,
   getUnpackedSettings,
+  sensitiveHeaders,
   Http2ServerRequest,
   Http2ServerResponse
 } = require('internal/http2/core');
@@ -20,6 +21,7 @@ module.exports = {
   getDefaultSettings,
   getPackedSettings,
   getUnpackedSettings,
+  sensitiveHeaders,
   Http2ServerRequest,
   Http2ServerResponse
 };
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 1503130f9b4752..0eedfaece2331d 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -123,6 +123,7 @@ const {
   getSettings,
   getStreamState,
   isPayloadMeaningless,
+  kSensitiveHeaders,
   kSocket,
   kRequest,
   kProxySocket,
@@ -303,7 +304,7 @@ function emit(self, ...args) {
 // create the associated Http2Stream instance and emit the 'stream'
 // event. If the stream is not new, emit the 'headers' event to pass
 // the block of headers on.
-function onSessionHeaders(handle, id, cat, flags, headers) {
+function onSessionHeaders(handle, id, cat, flags, headers, sensitiveHeaders) {
   const session = this[kOwner];
   if (session.destroyed)
     return;
@@ -317,7 +318,7 @@ function onSessionHeaders(handle, id, cat, flags, headers) {
   let stream = streams.get(id);
 
   // Convert the array of header name value pairs into an object
-  const obj = toHeaderObject(headers);
+  const obj = toHeaderObject(headers, sensitiveHeaders);
 
   if (stream === undefined) {
     if (session.closed) {
@@ -2232,6 +2233,7 @@ function processHeaders(oldHeaders) {
         headers[key] = oldHeaders[key];
       }
     }
+    headers[kSensitiveHeaders] = oldHeaders[kSensitiveHeaders];
   }
 
   const statusCode =
@@ -2251,6 +2253,10 @@ function processHeaders(oldHeaders) {
   if (statusCode < 200 || statusCode > 599)
     throw new ERR_HTTP2_STATUS_INVALID(headers[HTTP2_HEADER_STATUS]);
 
+  const neverIndex = headers[kSensitiveHeaders];
+  if (neverIndex !== undefined && !ArrayIsArray(neverIndex))
+    throw new ERR_INVALID_OPT_VALUE('headers[http2.neverIndex]', neverIndex);
+
   return headers;
 }
 
@@ -3166,6 +3172,7 @@ module.exports = {
   getDefaultSettings,
   getPackedSettings,
   getUnpackedSettings,
+  sensitiveHeaders: kSensitiveHeaders,
   Http2Session,
   Http2Stream,
   Http2ServerRequest,
diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js
index 8327a1d248f803..bc3e490db19790 100644
--- a/lib/internal/http2/util.js
+++ b/lib/internal/http2/util.js
@@ -8,6 +8,7 @@ const {
   ObjectCreate,
   ObjectKeys,
   Set,
+  StringPrototypeToLowerCase,
   Symbol,
 } = primordials;
 
@@ -25,11 +26,14 @@ const {
   hideStackFrames
 } = require('internal/errors');
 
+const kSensitiveHeaders = Symbol('nodejs.http2.sensitiveHeaders');
 const kSocket = Symbol('socket');
 const kProxySocket = Symbol('proxySocket');
 const kRequest = Symbol('request');
 
 const {
+  NGHTTP2_NV_FLAG_NONE,
+  NGHTTP2_NV_FLAG_NO_INDEX,
   NGHTTP2_SESSION_CLIENT,
   NGHTTP2_SESSION_SERVER,
 
@@ -443,6 +447,9 @@ const assertValidPseudoHeaderTrailer = hideStackFrames((key) => {
   throw new ERR_HTTP2_INVALID_PSEUDOHEADER(key);
 });
 
+const emptyArray = [];
+const kNeverIndexFlag = String.fromCharCode(NGHTTP2_NV_FLAG_NO_INDEX);
+const kNoHeaderFlags = String.fromCharCode(NGHTTP2_NV_FLAG_NONE);
 function mapToHeaders(map,
                       assertValuePseudoHeader = assertValidPseudoHeader) {
   let ret = '';
@@ -455,6 +462,8 @@ function mapToHeaders(map,
   let value;
   let isSingleValueHeader;
   let err;
+  const neverIndex =
+    (map[kSensitiveHeaders] || emptyArray).map(StringPrototypeToLowerCase);
   for (i = 0; i < keys.length; ++i) {
     key = keys[i];
     value = map[key];
@@ -483,11 +492,12 @@ function mapToHeaders(map,
         throw new ERR_HTTP2_HEADER_SINGLE_VALUE(key);
       singles.add(key);
     }
+    const flags = neverIndex.includes(key) ? kNeverIndexFlag : kNoHeaderFlags;
     if (key[0] === ':') {
       err = assertValuePseudoHeader(key);
       if (err !== undefined)
         throw err;
-      ret = `${key}\0${value}\0${ret}`;
+      ret = `${key}\0${value}\0${flags}${ret}`;
       count++;
       continue;
     }
@@ -500,12 +510,12 @@ function mapToHeaders(map,
     if (isArray) {
       for (j = 0; j < value.length; ++j) {
         const val = String(value[j]);
-        ret += `${key}\0${val}\0`;
+        ret += `${key}\0${val}\0${flags}`;
       }
       count += value.length;
       continue;
     }
-    ret += `${key}\0${value}\0`;
+    ret += `${key}\0${value}\0${flags}`;
     count++;
   }
 
@@ -544,7 +554,7 @@ const assertWithinRange = hideStackFrames(
   }
 );
 
-function toHeaderObject(headers) {
+function toHeaderObject(headers, sensitiveHeaders) {
   const obj = ObjectCreate(null);
   for (var n = 0; n < headers.length; n = n + 2) {
     const name = headers[n];
@@ -585,6 +595,7 @@ function toHeaderObject(headers) {
       }
     }
   }
+  obj[kSensitiveHeaders] = sensitiveHeaders;
   return obj;
 }
 
@@ -614,6 +625,7 @@ module.exports = {
   getSettings,
   getStreamState,
   isPayloadMeaningless,
+  kSensitiveHeaders,
   kSocket,
   kProxySocket,
   kRequest,
diff --git a/src/node_http2.cc b/src/node_http2.cc
index 3227ae66a2dead..0d18ca15c190fa 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -1213,22 +1213,29 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
   // this way for performance reasons (it's faster to generate and pass an
   // array than it is to generate and pass the object).
 
-  std::vector<Local<Value>> headers_v(stream->headers_count() * 2);
+  MaybeStackBuffer<Local<Value>, 64> headers_v(stream->headers_count() * 2);
+  MaybeStackBuffer<Local<Value>, 32> sensitive_v(stream->headers_count());
+  size_t sensitive_count = 0;
+
   stream->TransferHeaders([&](const Http2Header& header, size_t i) {
     headers_v[i * 2] = header.GetName(this).ToLocalChecked();
     headers_v[i * 2 + 1] = header.GetValue(this).ToLocalChecked();
+    if (header.flags() & NGHTTP2_NV_FLAG_NO_INDEX)
+      sensitive_v[sensitive_count++] = headers_v[i * 2];
   });
   CHECK_EQ(stream->headers_count(), 0);
 
   DecrementCurrentSessionMemory(stream->current_headers_length_);
   stream->current_headers_length_ = 0;
 
-  Local<Value> args[5] = {
-      stream->object(),
-      Integer::New(isolate, id),
-      Integer::New(isolate, stream->headers_category()),
-      Integer::New(isolate, frame->hd.flags),
-      Array::New(isolate, headers_v.data(), headers_v.size())};
+  Local<Value> args[] = {
+    stream->object(),
+    Integer::New(isolate, id),
+    Integer::New(isolate, stream->headers_category()),
+    Integer::New(isolate, frame->hd.flags),
+    Array::New(isolate, headers_v.out(), headers_v.length()),
+    Array::New(isolate, sensitive_v.out(), sensitive_count),
+  };
   MakeCallback(env()->http2session_on_headers_function(),
                arraysize(args), args);
 }
diff --git a/src/node_http2.h b/src/node_http2.h
index 4ef9e9fbab0c43..f0c79cd27de56e 100644
--- a/src/node_http2.h
+++ b/src/node_http2.h
@@ -116,7 +116,6 @@ using Nghttp2SessionCallbacksPointer =
 
 struct Http2HeadersTraits {
   typedef nghttp2_nv nv_t;
-  static const uint8_t kNoneFlag = NGHTTP2_NV_FLAG_NONE;
 };
 
 struct Http2RcBufferPointerTraits {
diff --git a/src/node_http_common-inl.h b/src/node_http_common-inl.h
index 7cd4bdec99c3d6..6ddc99e7d4e897 100644
--- a/src/node_http_common-inl.h
+++ b/src/node_http_common-inl.h
@@ -55,13 +55,14 @@ NgHeaders<T>::NgHeaders(Environment* env, v8::Local<v8::Array> headers) {
       return;
     }
 
-    nva[n].flags = T::kNoneFlag;
     nva[n].name = reinterpret_cast<uint8_t*>(p);
     nva[n].namelen = strlen(p);
     p += nva[n].namelen + 1;
     nva[n].value = reinterpret_cast<uint8_t*>(p);
     nva[n].valuelen = strlen(p);
     p += nva[n].valuelen + 1;
+    nva[n].flags = *p;
+    p++;
   }
 }
 
@@ -189,6 +190,11 @@ size_t NgHeader<T>::length() const {
   return name_.len() + value_.len();
 }
 
+template <typename T>
+uint8_t NgHeader<T>::flags() const {
+  return flags_;
+}
+
 }  // namespace node
 
 #endif  // SRC_NODE_HTTP_COMMON_INL_H_
diff --git a/src/node_http_common.h b/src/node_http_common.h
index c7e4d34af24196..8017c0d7aad30c 100644
--- a/src/node_http_common.h
+++ b/src/node_http_common.h
@@ -460,6 +460,7 @@ struct NgHeaderBase : public MemoryRetainer {
   virtual std::string name() const = 0;
   virtual std::string value() const = 0;
   virtual size_t length() const = 0;
+  virtual uint8_t flags() const = 0;
   virtual std::string ToString() const;
 };
 
@@ -505,6 +506,7 @@ class NgHeader final : public NgHeaderBase<typename T::allocator_t> {
   inline std::string name() const override;
   inline std::string value() const override;
   inline size_t length() const override;
+  inline uint8_t flags() const override;
 
   void MemoryInfo(MemoryTracker* tracker) const override;
 
diff --git a/src/quic/node_quic_http3_application.h b/src/quic/node_quic_http3_application.h
index fdedb1a45aff5d..353874146257fd 100644
--- a/src/quic/node_quic_http3_application.h
+++ b/src/quic/node_quic_http3_application.h
@@ -42,7 +42,6 @@ struct Http3RcBufferPointerTraits {
 
 struct Http3HeadersTraits {
   typedef nghttp3_nv nv_t;
-  static const uint8_t kNoneFlag = NGHTTP3_NV_FLAG_NONE;
 };
 
 using Http3ConnectionPointer = DeleteFnPtr<nghttp3_conn, nghttp3_conn_del>;
diff --git a/test/parallel/test-http2-sensitive-headers.js b/test/parallel/test-http2-sensitive-headers.js
new file mode 100644
index 00000000000000..7d4d775a55d4c2
--- /dev/null
+++ b/test/parallel/test-http2-sensitive-headers.js
@@ -0,0 +1,47 @@
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const http2 = require('http2');
+const makeDuplexPair = require('../common/duplexpair');
+
+{
+  const testData = '<h1>Hello World</h1>';
+  const server = http2.createServer();
+  server.on('stream', common.mustCall((stream, headers) => {
+    stream.respond({
+      'content-type': 'text/html',
+      ':status': 200,
+      'cookie': 'donotindex',
+      'not-sensitive': 'foo',
+      'sensitive': 'bar',
+      // sensitiveHeaders entries are case-insensitive
+      [http2.sensitiveHeaders]: ['Sensitive']
+    });
+    stream.end(testData);
+  }));
+
+  const { clientSide, serverSide } = makeDuplexPair();
+  server.emit('connection', serverSide);
+
+  const client = http2.connect('http://localhost:80', {
+    createConnection: common.mustCall(() => clientSide)
+  });
+
+  const req = client.request({ ':path': '/' });
+
+  req.on('response', common.mustCall((headers) => {
+    assert.strictEqual(headers[':status'], 200);
+    assert.strictEqual(headers.cookie, 'donotindex');
+    assert.deepStrictEqual(headers[http2.sensitiveHeaders],
+                           ['cookie', 'sensitive']);
+  }));
+
+  req.on('end', common.mustCall(() => {
+    clientSide.destroy();
+    clientSide.end();
+  }));
+  req.resume();
+  req.end();
+}
diff --git a/test/parallel/test-http2-util-headers-list.js b/test/parallel/test-http2-util-headers-list.js
index a964c361b78815..6b9ce079076ce8 100644
--- a/test/parallel/test-http2-util-headers-list.js
+++ b/test/parallel/test-http2-util-headers-list.js
@@ -9,6 +9,7 @@ if (!common.hasCrypto)
   common.skip('missing crypto');
 const assert = require('assert');
 const { mapToHeaders, toHeaderObject } = require('internal/http2/util');
+const { sensitiveHeaders } = require('http2');
 const { internalBinding } = require('internal/test/binding');
 const {
   HTTP2_HEADER_STATUS,
@@ -102,8 +103,9 @@ const {
 
   assert.deepStrictEqual(
     mapToHeaders(headers),
-    [ [ ':path', 'abc', ':status', '200', 'abc', '1', 'xyz', '1', 'xyz', '2',
-        'xyz', '3', 'xyz', '4', 'bar', '1', '' ].join('\0'), 8 ]
+    [ [ ':path', 'abc\0', ':status', '200\0', 'abc', '1\0', 'xyz', '1\0',
+        'xyz', '2\0', 'xyz', '3\0', 'xyz', '4\0', 'bar', '1\0', '' ].join('\0'),
+      8 ]
   );
 }
 
@@ -118,8 +120,8 @@ const {
 
   assert.deepStrictEqual(
     mapToHeaders(headers),
-    [ [ ':status', '200', ':path', 'abc', 'abc', '1', 'xyz', '1', 'xyz', '2',
-        'xyz', '3', 'xyz', '4', '' ].join('\0'), 7 ]
+    [ [ ':status', '200\0', ':path', 'abc\0', 'abc', '1\0', 'xyz', '1\0',
+        'xyz', '2\0', 'xyz', '3\0', 'xyz', '4\0', '' ].join('\0'), 7 ]
   );
 }
 
@@ -135,8 +137,8 @@ const {
 
   assert.deepStrictEqual(
     mapToHeaders(headers),
-    [ [ ':status', '200', ':path', 'abc', 'abc', '1', 'xyz', '1', 'xyz', '2',
-        'xyz', '3', 'xyz', '4', '' ].join('\0'), 7 ]
+    [ [ ':status', '200\0', ':path', 'abc\0', 'abc', '1\0', 'xyz', '1\0',
+        'xyz', '2\0', 'xyz', '3\0', 'xyz', '4\0', '' ].join('\0'), 7 ]
   );
 }
 
@@ -151,8 +153,8 @@ const {
 
   assert.deepStrictEqual(
     mapToHeaders(headers),
-    [ [ ':status', '200', ':path', 'abc', 'xyz', '1', 'xyz', '2', 'xyz', '3',
-        'xyz', '4', '' ].join('\0'), 6 ]
+    [ [ ':status', '200\0', ':path', 'abc\0', 'xyz', '1\0', 'xyz', '2\0',
+        'xyz', '3\0', 'xyz', '4\0', '' ].join('\0'), 6 ]
   );
 }
 
@@ -164,7 +166,7 @@ const {
   };
   assert.deepStrictEqual(
     mapToHeaders(headers),
-    [ [ 'set-cookie', 'foo=bar', '' ].join('\0'), 1 ]
+    [ [ 'set-cookie', 'foo=bar\0', '' ].join('\0'), 1 ]
   );
 }
 
@@ -182,6 +184,23 @@ const {
   });
 }
 
+{
+  const headers = {
+    'abc': 1,
+    ':path': 'abc',
+    ':status': [200],
+    ':authority': [],
+    'xyz': [1, 2, 3, 4],
+    [sensitiveHeaders]: ['xyz']
+  };
+
+  assert.deepStrictEqual(
+    mapToHeaders(headers),
+    [ ':status\x00200\x00\x00:path\x00abc\x00\x00abc\x001\x00\x00' +
+      'xyz\x001\x00\x01xyz\x002\x00\x01xyz\x003\x00\x01xyz\x004\x00\x01', 7 ]
+  );
+}
+
 // The following are not allowed to have multiple values
 [
   HTTP2_HEADER_STATUS,
diff --git a/test/parallel/test-http2-zero-length-header.js b/test/parallel/test-http2-zero-length-header.js
index 7b142d75f003b6..2e7876858aaace 100644
--- a/test/parallel/test-http2-zero-length-header.js
+++ b/test/parallel/test-http2-zero-length-header.js
@@ -14,7 +14,8 @@ server.on('stream', (stream, headers) => {
     ':method': 'GET',
     ':path': '/',
     'bar': '',
-    '__proto__': null
+    '__proto__': null,
+    [http2.sensitiveHeaders]: []
   });
   stream.session.destroy();
   server.close();