src: retain pointers to WriteWrap/ShutdownWrap

Avoids potential use-after-free when wrap req's are synchronously destroyed. CVE-ID: CVE-2020-8265 Fixes: nodejs-private/node-private#227 Refs: https://hackerone.com/bugs?subject=nodejs&report_id=988103 PR-URL: nodejs-private/node-private#23 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]>
nodejs · Dec 24, 2020 · 9834ef8 · 9834ef8
1 parent c5dbe83
commit 9834ef8
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 4 deletions.
diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h
@@ -137,8 +137,11 @@ 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) {
@@ -172,7 +175,7 @@ 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, {} };
     }
   }
 
@@ -182,13 +185,14 @@ 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;
@@ -206,7 +210,8 @@ 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
@@ -265,7 +265,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
@@ -25,6 +25,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
@@ -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());
+  });
+}));