src: retain pointers to WriteWrap/ShutdownWrap

jasnell · richardlau · commit 5b00de7d67a1 · 2020-12-24T11:27:08.000Z
Avoids potential use-after-free when wrap req's are synchronously destroyed. CVE-ID: CVE-2020-8265 Fixes: nodejs-private/node-private#227 PR-URL: nodejs-private/node-private#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>
diff --git 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
@@ -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
@@ -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
@@ -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());
+  });
+}));

Original file line number	Diff line number	Diff line change
`@@ -163,8 +163,11 @@ inline int StreamBase::Shutdown(v8::Local<v8::Object> req_wrap_obj) {`
`163`	`163`	`StreamReq::ResetObject(req_wrap_obj);`
`164`	`164`	`}`
`165`	`165`
	`166`	`+ BaseObjectPtr<AsyncWrap> req_wrap_ptr;`
`166`	`167`	`AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(GetAsyncWrap());`
`167`	`168`	`ShutdownWrap* req_wrap = CreateShutdownWrap(req_wrap_obj);`
	`169`	`+ if (req_wrap != nullptr)`
	`170`	`+ req_wrap_ptr.reset(req_wrap->GetAsyncWrap());`
`168`	`171`	`int err = DoShutdown(req_wrap);`
`169`	`172`
`170`	`173`	`if (err != 0 && req_wrap != nullptr) {`
`@@ -198,7 +201,7 @@ inline StreamWriteResult StreamBase::Write(`
`198`	`201`	`if (send_handle == nullptr) {`
`199`	`202`	`err = DoTryWrite(&bufs, &count);`
`200`	`203`	`if (err != 0 \|\| count == 0) {`
`201`		`- return StreamWriteResult { false, err, nullptr, total_bytes };`
	`204`	`+ return StreamWriteResult { false, err, nullptr, total_bytes, {} };`
`202`	`205`	`}`
`203`	`206`	`}`
`204`	`207`
`@@ -208,13 +211,14 @@ inline StreamWriteResult StreamBase::Write(`
`208`	`211`	`if (!env->write_wrap_template()`
`209`	`212`	`->NewInstance(env->context())`
`210`	`213`	`.ToLocal(&req_wrap_obj)) {`
`211`		`- return StreamWriteResult { false, UV_EBUSY, nullptr, 0 };`
	`214`	`+ return StreamWriteResult { false, UV_EBUSY, nullptr, 0, {} };`
`212`	`215`	`}`
`213`	`216`	`StreamReq::ResetObject(req_wrap_obj);`
`214`	`217`	`}`
`215`	`218`
`216`	`219`	`AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(GetAsyncWrap());`
`217`	`220`	`WriteWrap* req_wrap = CreateWriteWrap(req_wrap_obj);`
	`221`	`+ BaseObjectPtr<AsyncWrap> req_wrap_ptr(req_wrap->GetAsyncWrap());`
`218`	`222`
`219`	`223`	`err = DoWrite(req_wrap, bufs, count, send_handle);`
`220`	`224`	`bool async = err == 0;`
`@@ -232,7 +236,8 @@ inline StreamWriteResult StreamBase::Write(`
`232`	`236`	`ClearError();`
`233`	`237`	`}`
`234`	`238`
`235`		`- return StreamWriteResult { async, err, req_wrap, total_bytes };`
	`239`	`+ return StreamWriteResult {`
	`240`	`+ async, err, req_wrap, total_bytes, std::move(req_wrap_ptr) };`
`236`	`241`	`}`
`237`	`242`
`238`	`243`	`template <typename OtherBase>`
Original file line number	Diff line number	Diff line change
`@@ -259,7 +259,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo<Value>& args) {`
`259`	`259`
`260`	`260`	`// Immediate failure or success`
`261`	`261`	`if (err != 0 \|\| count == 0) {`
`262`		`- SetWriteResult(StreamWriteResult { false, err, nullptr, data_size });`
	`262`	`+ SetWriteResult(StreamWriteResult { false, err, nullptr, data_size, {} });`
`263`	`263`	`return err;`
`264`	`264`	`}`
`265`	`265`