fix CVE-2020-8265 CVE-2020-8287

2021-02-09 14:41:25 +08:00 · 2021-02-09 14:41:25 +08:00 · 49e2e7f39a
commit 49e2e7f39a
parent 4e9f077dc7
4 changed files with 402 additions and 1 deletions
--- a/CVE-2020-8265.patch
+++ b/CVE-2020-8265.patch
@ -0,0 +1,164 @@
+From 5b00de7d67a1372aa342115ad28edd3f78268bb6 Mon Sep 17 00:00:00 2001
+From: James M Snell <jasnell@gmail.com>
+Date: Thu, 12 Nov 2020 12:34:33 -0800
+Subject: [PATCH] src: retain pointers to WriteWrap/ShutdownWrap
+
+Avoids potential use-after-free when wrap req's are synchronously
+destroyed.
+
+CVE-ID: CVE-2020-8265
+Fixes: https://github.com/nodejs-private/node-private/issues/227
+PR-URL: https://github.com/nodejs-private/node-private/pull/230
+Refs: https://hackerone.com/bugs?subject=nodejs&report_id=988103
+Reviewed-By: Anna Henningsen <anna@addaleax.net>
+Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
+Reviewed-By: Rich Trott <rtrott@gmail.com>
+Reference: https://github.com/nodejs/node/commit/5b00de7d67a1372aa342115ad28edd3f78268bb6
+---
+ src/stream_base-inl.h                         | 11 +++-
+ src/stream_base.cc                            |  2 +-
+ src/stream_base.h                             |  1 +
+ .../test-tls-use-after-free-regression.js     | 58 +++++++++++++++++++
+ 4 files changed, 68 insertions(+), 4 deletions(-)
+ create mode 100644 test/parallel/test-tls-use-after-free-regression.js
+
+diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h
+index dd80683af10..1603a2fb2e0 100644
+--- a/src/stream_base-inl.h
+++ b/src/stream_base-inl.h
+@@ -163,8 +163,11 @@ inline int StreamBase::Shutdown(v8::Local<v8::Object> req_wrap_obj) {
+     StreamReq::ResetObject(req_wrap_obj);
+   }
+ 
+  BaseObjectPtr<AsyncWrap> req_wrap_ptr;
+   AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(GetAsyncWrap());
+   ShutdownWrap* req_wrap = CreateShutdownWrap(req_wrap_obj);
+  if (req_wrap != nullptr)
+    req_wrap_ptr.reset(req_wrap->GetAsyncWrap());
+   int err = DoShutdown(req_wrap);
+ 
+   if (err != 0 && req_wrap != nullptr) {
+@@ -198,7 +201,7 @@ inline StreamWriteResult StreamBase::Write(
+   if (send_handle == nullptr) {
+     err = DoTryWrite(&bufs, &count);
+     if (err != 0 || count == 0) {
+-      return StreamWriteResult { false, err, nullptr, total_bytes };
+      return StreamWriteResult { false, err, nullptr, total_bytes, {} };
+     }
+   }
+ 
+@@ -208,13 +211,14 @@ inline StreamWriteResult StreamBase::Write(
+     if (!env->write_wrap_template()
+              ->NewInstance(env->context())
+              .ToLocal(&req_wrap_obj)) {
+-      return StreamWriteResult { false, UV_EBUSY, nullptr, 0 };
+      return StreamWriteResult { false, UV_EBUSY, nullptr, 0, {} };
+     }
+     StreamReq::ResetObject(req_wrap_obj);
+   }
+ 
+   AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(GetAsyncWrap());
+   WriteWrap* req_wrap = CreateWriteWrap(req_wrap_obj);
+  BaseObjectPtr<AsyncWrap> req_wrap_ptr(req_wrap->GetAsyncWrap());
+ 
+   err = DoWrite(req_wrap, bufs, count, send_handle);
+   bool async = err == 0;
+@@ -232,7 +236,8 @@ inline StreamWriteResult StreamBase::Write(
+     ClearError();
+   }
+ 
+-  return StreamWriteResult { async, err, req_wrap, total_bytes };
+  return StreamWriteResult {
+      async, err, req_wrap, total_bytes, std::move(req_wrap_ptr) };
+ }
+ 
+ template <typename OtherBase>
+diff --git a/src/stream_base.cc b/src/stream_base.cc
+index 516f57e40bf..06032e2c096 100644
+--- a/src/stream_base.cc
+++ b/src/stream_base.cc
+@@ -259,7 +259,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo<Value>& args) {
+ 
+     // Immediate failure or success
+     if (err != 0 || count == 0) {
+-      SetWriteResult(StreamWriteResult { false, err, nullptr, data_size });
+      SetWriteResult(StreamWriteResult { false, err, nullptr, data_size, {} });
+       return err;
+     }
+ 
+diff --git a/src/stream_base.h b/src/stream_base.h
+index eb75fdc8339..fafd327d75d 100644
+--- a/src/stream_base.h
+++ b/src/stream_base.h
+@@ -24,6 +24,7 @@ struct StreamWriteResult {
+   int err;
+   WriteWrap* wrap;
+   size_t bytes;
+  BaseObjectPtr<AsyncWrap> wrap_obj;
+ };
+ 
+ using JSMethodFunction = void(const v8::FunctionCallbackInfo<v8::Value>& args);
+diff --git a/test/parallel/test-tls-use-after-free-regression.js b/test/parallel/test-tls-use-after-free-regression.js
+new file mode 100644
+index 00000000000..51835fc0339
+--- /dev/null
+++ b/test/parallel/test-tls-use-after-free-regression.js
+@@ -0,0 +1,58 @@
+'use strict';
+
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const https = require('https');
+const tls = require('tls');
+
+const kMessage =
+  'GET / HTTP/1.1\r\nHost: localhost\r\nConnection: Keep-alive\r\n\r\n';
+
+const key = `-----BEGIN EC PARAMETERS-----
+BggqhkjOPQMBBw==
+-----END EC PARAMETERS-----
+-----BEGIN EC PRIVATE KEY-----
+MHcCAQEEIDKfHHbiJMdu2STyHL11fWC7psMY19/gUNpsUpkwgGACoAoGCCqGSM49
+AwEHoUQDQgAEItqm+pYj3Ca8bi5mBs+H8xSMxuW2JNn4I+kw3aREsetLk8pn3o81
+PWBiTdSZrGBGQSy+UAlQvYeE6Z/QXQk8aw==
+-----END EC PRIVATE KEY-----`;
+
+const cert = `-----BEGIN CERTIFICATE-----
+MIIBhjCCASsCFDJU1tCo88NYU//pE+DQKO9hUDsFMAoGCCqGSM49BAMCMEUxCzAJ
+BgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5l
+dCBXaWRnaXRzIFB0eSBMdGQwHhcNMjAwOTIyMDg1NDU5WhcNNDgwMjA3MDg1NDU5
+WjBFMQswCQYDVQQGEwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UECgwY
+SW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcD
+QgAEItqm+pYj3Ca8bi5mBs+H8xSMxuW2JNn4I+kw3aREsetLk8pn3o81PWBiTdSZ
+rGBGQSy+UAlQvYeE6Z/QXQk8azAKBggqhkjOPQQDAgNJADBGAiEA7Bdn4F87KqIe
+Y/ABy/XIXXpFUb2nyv3zV7POQi2lPcECIQC3UWLmfiedpiIKsf9YRIyO0uEood7+
+glj2R1NNr1X68w==
+-----END CERTIFICATE-----`;
+
+const server = https.createServer(
+  { key, cert },
+  common.mustCall((req, res) => {
+    res.writeHead(200);
+    res.end('boom goes the dynamite\n');
+  }, 3));
+
+server.listen(0, common.mustCall(() => {
+  const socket =
+    tls.connect(
+      server.address().port,
+      'localhost',
+      { rejectUnauthorized: false },
+      common.mustCall(() => {
+        socket.write(kMessage);
+        socket.write(kMessage);
+        socket.write(kMessage);
+      }));
+
+  socket.on('data', common.mustCall(() => socket.destroy()));
+  socket.on('close', () => {
+    setImmediate(() => server.close());
+  });
+}));
--- a/CVE-2020-8287-1.patch
+++ b/CVE-2020-8287-1.patch
@ -0,0 +1,79 @@
+From 92d430917a63a567bb528100371263c46e50ee4a Mon Sep 17 00:00:00 2001
+From: Fedor Indutny <fedor@indutny.com>
+Date: Wed, 18 Nov 2020 20:50:21 -0800
+Subject: [PATCH] http: unset `F_CHUNKED` on new `Transfer-Encoding`
+
+Duplicate `Transfer-Encoding` header should be a treated as a single,
+but with original header values concatenated with a comma separator. In
+the light of this, even if the past `Transfer-Encoding` ended with
+`chunked`, we should be not let the `F_CHUNKED` to leak into the next
+header, because mere presence of another header indicates that `chunked`
+is not the last transfer-encoding token.
+
+CVE-ID: CVE-2020-8287
+PR-URL: https://github.com/nodejs-private/node-private/pull/236
+Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
+Reference: https://github.com/nodejs/node/commit/92d430917a63a567bb528100371263c46e50ee4a
+---
+ deps/http_parser/http_parser.c |  7 +++++++
+ deps/http_parser/test.c        | 26 ++++++++++++++++++++++++++
+ 2 files changed, 33 insertions(+)
+
+diff --git a/deps/http_parser/http_parser.c b/deps/http_parser/http_parser.c
+index 0f76b6a..5cc951a 100644
+--- a/deps/http_parser/http_parser.c
+++ b/deps/http_parser/http_parser.c
+@@ -1339,6 +1339,13 @@ reexecute:
+               } else if (parser->index == sizeof(TRANSFER_ENCODING)-2) {
+                 parser->header_state = h_transfer_encoding;
+                 parser->flags |= F_TRANSFER_ENCODING;
+
+		/* Multiple `Transfer-Encoding` headers should be treated as
+                 * one, but with values separate by a comma.
+                 *
+                 * See: https://tools.ietf.org/html/rfc7230#section-3.2.2
+                 */
+                parser->flags &= ~F_CHUNKED;
+               }
+               break;
+ 
+diff --git a/deps/http_parser/test.c b/deps/http_parser/test.c
+index c979467..f185c56 100644
+--- a/deps/http_parser/test.c
+++ b/deps/http_parser/test.c
+@@ -2045,6 +2045,32 @@ const struct message responses[] =
+   ,.body= "2\r\nOK\r\n0\r\n\r\n"
+   ,.num_chunks_complete= 0
+   }
+#define HTTP_200_DUPLICATE_TE_NOT_LAST_CHUNKED 30
+, {.name= "HTTP 200 response with `chunked` and duplicate Transfer-Encoding"
+  ,.type= HTTP_RESPONSE
+  ,.raw= "HTTP/1.1 200 OK\r\n"
+         "Transfer-Encoding: chunked\r\n"
+         "Transfer-Encoding: identity\r\n"
+         "\r\n"
+         "2\r\n"
+         "OK\r\n"
+         "0\r\n"
+         "\r\n"
+  ,.should_keep_alive= FALSE
+  ,.message_complete_on_eof= TRUE
+  ,.http_major= 1
+  ,.http_minor= 1
+  ,.status_code= 200
+  ,.response_status= "OK"
+  ,.content_length= -1
+  ,.num_headers= 2
+  ,.headers=
+    { { "Transfer-Encoding", "chunked" }
+    , { "Transfer-Encoding", "identity" }
+    }
+  ,.body= "2\r\nOK\r\n0\r\n\r\n"
+  ,.num_chunks_complete= 0
+  }
+ };
+ 
+ /* strnlen() is a POSIX.2008 addition. Can't rely on it being available so
+-- 
+2.23.0
+
--- a/CVE-2020-8287-2.patch
+++ b/CVE-2020-8287-2.patch
@ -0,0 +1,151 @@
+From 420244e4d9ca6de2612e7f503f5c87e448fbc14b Mon Sep 17 00:00:00 2001
+From: Matteo Collina <hello@matteocollina.com>
+Date: Thu, 22 Oct 2020 14:10:51 +0200
+Subject: [PATCH] http: unset `F_CHUNKED` on new `Transfer-Encoding`
+
+Duplicate `Transfer-Encoding` header should be a treated as a single,
+but with original header values concatenated with a comma separator. In
+the light of this, even if the past `Transfer-Encoding` ended with
+`chunked`, we should be not let the `F_CHUNKED` to leak into the next
+header, because mere presence of another header indicates that `chunked`
+is not the last transfer-encoding token.
+
+Ref: https://github.com/nodejs-private/llhttp-private/pull/3
+See: https://hackerone.com/bugs?report_id=1002188&subject=nodejs
+
+CVE-ID: CVE-2020-8287
+PR-URL: https://github.com/nodejs-private/node-private/pull/236
+Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
+Reference: https://github.com/nodejs/node/commit/420244e4d9ca6de2612e7f503f5c87e448fbc14b
+---
+ deps/llhttp/src/llhttp.c                      | 36 ++++++++++++++-
+ .../test-http-transfer-encoding-smuggling.js  | 46 +++++++++++++++++++
+ 2 files changed, 80 insertions(+), 2 deletions(-)
+ create mode 100644 test/parallel/test-http-transfer-encoding-smuggling.js
+
+diff --git a/deps/llhttp/src/llhttp.c b/deps/llhttp/src/llhttp.c
+index acc35479f88..3019c410963 100644
+--- a/deps/llhttp/src/llhttp.c
+++ b/deps/llhttp/src/llhttp.c
+@@ -813,6 +813,14 @@ int llhttp__internal__c_or_flags_16(
+   return 0;
+ }
+ 
+int llhttp__internal__c_and_flags(
+    llhttp__internal_t* state,
+    const unsigned char* p,
+    const unsigned char* endp) {
+  state->flags &= -9;
+  return 0;
+}
+
+ int llhttp__internal__c_update_header_state_7(
+     llhttp__internal_t* state,
+     const unsigned char* p,
+@@ -5974,10 +5982,18 @@ static llparse_state_t llhttp__internal__run(
+     /* UNREACHABLE */;
+     abort();
+   }
+  s_n_llhttp__internal__n_invoke_and_flags: {
+    switch (llhttp__internal__c_and_flags(state, p, endp)) {
+      default:
+        goto s_n_llhttp__internal__n_header_value_te_chunked;
+    }
+    /* UNREACHABLE */;
+    abort();
+  }
+   s_n_llhttp__internal__n_invoke_or_flags_16: {
+     switch (llhttp__internal__c_or_flags_16(state, p, endp)) {
+       default:
+-        goto s_n_llhttp__internal__n_header_value_te_chunked;
+        goto s_n_llhttp__internal__n_invoke_and_flags;
+     }
+     /* UNREACHABLE */;
+     abort();
+@@ -7625,6 +7641,14 @@ int llhttp__internal__c_or_flags_16(
+   return 0;
+ }
+ 
+int llhttp__internal__c_and_flags(
+    llhttp__internal_t* state,
+    const unsigned char* p,
+    const unsigned char* endp) {
+  state->flags &= -9;
+  return 0;
+}
+
+ int llhttp__internal__c_update_header_state_7(
+     llhttp__internal_t* state,
+     const unsigned char* p,
+@@ -12522,10 +12546,18 @@ static llparse_state_t llhttp__internal__run(
+     /* UNREACHABLE */;
+     abort();
+   }
+  s_n_llhttp__internal__n_invoke_and_flags: {
+    switch (llhttp__internal__c_and_flags(state, p, endp)) {
+      default:
+        goto s_n_llhttp__internal__n_header_value_te_chunked;
+    }
+    /* UNREACHABLE */;
+    abort();
+  }
+   s_n_llhttp__internal__n_invoke_or_flags_16: {
+     switch (llhttp__internal__c_or_flags_16(state, p, endp)) {
+       default:
+-        goto s_n_llhttp__internal__n_header_value_te_chunked;
+        goto s_n_llhttp__internal__n_invoke_and_flags;
+     }
+     /* UNREACHABLE */;
+     abort();
+diff --git a/test/parallel/test-http-transfer-encoding-smuggling.js b/test/parallel/test-http-transfer-encoding-smuggling.js
+new file mode 100644
+index 00000000000..9d97db4c0a2
+--- /dev/null
+++ b/test/parallel/test-http-transfer-encoding-smuggling.js
+@@ -0,0 +1,46 @@
+'use strict';
+
+const common = require('../common');
+
+const assert = require('assert');
+const http = require('http');
+const net = require('net');
+
+const msg = [
+  'POST / HTTP/1.1',
+  'Host: 127.0.0.1',
+  'Transfer-Encoding: chunked',
+  'Transfer-Encoding: chunked-false',
+  'Connection: upgrade',
+  '',
+  '1',
+  'A',
+  '0',
+  '',
+  'GET /flag HTTP/1.1',
+  'Host: 127.0.0.1',
+  '',
+  '',
+].join('\r\n');
+
+// Verify that the server is called only once even with a smuggled request.
+
+const server = http.createServer(common.mustCall((req, res) => {
+  res.end();
+}, 1));
+
+function send(next) {
+  const client = net.connect(server.address().port, 'localhost');
+  client.setEncoding('utf8');
+  client.on('error', common.mustNotCall());
+  client.on('end', next);
+  client.write(msg);
+  client.resume();
+}
+
+server.listen(0, common.mustCall((err) => {
+  assert.ifError(err);
+  send(common.mustCall(() => {
+    server.close();
+  }));
+}));
--- a/nodejs.spec
+++ b/nodejs.spec
@ -1,5 +1,5 @@
 %bcond_with bootstrap
-%global baserelease 2
+%global baserelease 3
 %{?!_pkgdocdir:%global _pkgdocdir %{_docdir}/%{name}-%{version}}
 %global nodejs_epoch 1
 %global nodejs_major 12
@ -85,6 +85,10 @@ Patch0002: 0002-Install-both-binaries-and-use-libdir.patch
 Patch0003: 0003-Modify-openEuler-aarch64-v8_os_page_size-to-64.patch
 %endif
 Patch0004: 0004-Make-AARCH64-compile-on-64KB-physical-pages.patch
+Patch0005: CVE-2020-8265.patch
+Patch0006: CVE-2020-8287-1.patch
+Patch0007: CVE-2020-8287-2.patch
+
 BuildRequires: python3-devel
 BuildRequires: zlib-devel
 BuildRequires: brotli-devel
@ -486,6 +490,9 @@ end
 %{_pkgdocdir}/npm/docs

 %changelog
+* Tue Feb 09 2021 xinghe <xinghe1@huawei.com> - 1:12.18.4-3
+- fix CVE-2020-8265 CVE-2020-8287
+
 * Mon Jan 04 2020 huanghaitao <huanghaitao8@huawei.com> - 1:12.18.4-2
 - Make AARCH64 compile on 64KB physical pages to fix build error