src: use unique_ptr for WriteWrap

This commit attempts to avoid a use-after-free error by using unqiue_ptr
and passing a reference to it.

CVE-ID: CVE-2020-8265
Fixes: nodejs-private/node-private#227
PR-URL: nodejs-private/node-private#238
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>

Author: danbev

src/js_stream.cc

@@ -105,7 +105,7 @@ int JSStream::DoShutdown(ShutdownWrap* req_wrap) {
 }
 
 
-int JSStream::DoWrite(WriteWrap* w,
+int JSStream::DoWrite(std::unique_ptr<WriteWrap>& w,
                       uv_buf_t* bufs,
                       size_t count,
                       uv_stream_t* send_handle) {
@@ -122,7 +122,7 @@ int JSStream::DoWrite(WriteWrap* w,
   }
 
   Local<Value> argv[] = {
-    w->object(),
+    w.get()->object(),
     bufs_arr
   };
 

src/js_stream.h

@@ -22,7 +22,7 @@ class JSStream : public AsyncWrap, public StreamBase {
   int ReadStop() override;
 
   int DoShutdown(ShutdownWrap* req_wrap) override;
-  int DoWrite(WriteWrap* w,
+  int DoWrite(std::unique_ptr<WriteWrap>& w,
               uv_buf_t* bufs,
               size_t count,
               uv_stream_t* send_handle) override;

src/node_file.h

@@ -287,7 +287,7 @@ class FileHandle : public AsyncWrap, public StreamBase {
   ShutdownWrap* CreateShutdownWrap(v8::Local<v8::Object> object) override;
   int DoShutdown(ShutdownWrap* req_wrap) override;
 
-  int DoWrite(WriteWrap* w,
+  int DoWrite(std::unique_ptr<WriteWrap>& w,
               uv_buf_t* bufs,
               size_t count,
               uv_stream_t* send_handle) override {

src/node_http2.cc

@@ -2315,7 +2315,7 @@ int Http2Stream::ReadStop() {
 // chunks of data have been flushed to the underlying nghttp2_session.
 // Note that this does *not* mean that the data has been flushed
 // to the socket yet.
-int Http2Stream::DoWrite(WriteWrap* req_wrap,
+int Http2Stream::DoWrite(std::unique_ptr<WriteWrap>& req_wrap,
                          uv_buf_t* bufs,
                          size_t nbufs,
                          uv_stream_t* send_handle) {
@@ -2330,7 +2330,7 @@ int Http2Stream::DoWrite(WriteWrap* req_wrap,
     // Store the req_wrap on the last write info in the queue, so that it is
     // only marked as finished once all buffers associated with it are finished.
     queue_.emplace(nghttp2_stream_write {
-      i == nbufs - 1 ? req_wrap : nullptr,
+      i == nbufs - 1 ? req_wrap.get() : nullptr,
       bufs[i]
     });
     IncrementAvailableOutboundLength(bufs[i].len);

src/node_http2.h

@@ -568,7 +568,7 @@ class Http2Stream : public AsyncWrap,
 
   AsyncWrap* GetAsyncWrap() override { return this; }
 
-  int DoWrite(WriteWrap* w, uv_buf_t* bufs, size_t count,
+  int DoWrite(std::unique_ptr<WriteWrap>& w, uv_buf_t* bufs, size_t count,
               uv_stream_t* send_handle) override;
 
   void MemoryInfo(MemoryTracker* tracker) const override {

src/stream_base-inl.h

@@ -216,14 +216,14 @@ inline StreamWriteResult StreamBase::Write(
   }
 
   AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(GetAsyncWrap());
-  WriteWrap* req_wrap = CreateWriteWrap(req_wrap_obj);
+  std::unique_ptr<WriteWrap> req_wrap{CreateWriteWrap(req_wrap_obj)};
 
   err = DoWrite(req_wrap, bufs, count, send_handle);
   bool async = err == 0;
 
-  if (!async) {
+  if (!async && req_wrap != nullptr) {
     req_wrap->Dispose();
-    req_wrap = nullptr;
+    req_wrap.release();
   }
 
   const char* msg = Error();
@@ -232,7 +232,7 @@ inline StreamWriteResult StreamBase::Write(
     ClearError();
   }
 
-  return StreamWriteResult { async, err, req_wrap, total_bytes };
+  return StreamWriteResult { async, err, req_wrap.release(), total_bytes };
 }
 
 template <typename OtherBase>

src/stream_base.h

@@ -215,10 +215,11 @@ class StreamResource {
   virtual int DoTryWrite(uv_buf_t** bufs, size_t* count);
   // Perform a write of data, and either call req_wrap->Done() when finished
   // and return 0, or return a libuv error code for synchronous failures.
-  virtual int DoWrite(WriteWrap* w,
-                      uv_buf_t* bufs,
-                      size_t count,
-                      uv_stream_t* send_handle) = 0;
+  virtual int DoWrite(
+      /* NOLINT (runtime/references) */ std::unique_ptr<WriteWrap>& w,
+      uv_buf_t* bufs,
+      size_t count,
+      uv_stream_t* send_handle) = 0;
 
   // Returns true if the stream supports the `OnStreamWantsWrite()` interface.
   virtual bool HasWantsWrite() const { return false; }

src/stream_wrap.cc

@@ -351,11 +351,11 @@ int LibuvStreamWrap::DoTryWrite(uv_buf_t** bufs, size_t* count) {
 }
 
 
-int LibuvStreamWrap::DoWrite(WriteWrap* req_wrap,
+int LibuvStreamWrap::DoWrite(std::unique_ptr<WriteWrap>& req_wrap,
                              uv_buf_t* bufs,
                              size_t count,
                              uv_stream_t* send_handle) {
-  LibuvWriteWrap* w = static_cast<LibuvWriteWrap*>(req_wrap);
+  LibuvWriteWrap* w = static_cast<LibuvWriteWrap*>(req_wrap.get());
   int r;
   if (send_handle == nullptr) {
     r = w->Dispatch(uv_write, stream(), bufs, count, AfterUvWrite);

src/stream_wrap.h

@@ -51,7 +51,7 @@ class LibuvStreamWrap : public HandleWrap, public StreamBase {
   // Resource implementation
   int DoShutdown(ShutdownWrap* req_wrap) override;
   int DoTryWrite(uv_buf_t** bufs, size_t* count) override;
-  int DoWrite(WriteWrap* w,
+  int DoWrite(std::unique_ptr<WriteWrap>& w,
               uv_buf_t* bufs,
               size_t count,
               uv_stream_t* send_handle) override;

src/tls_wrap.cc

@@ -91,8 +91,7 @@ bool TLSWrap::InvokeQueued(int status, const char* error_str) {
     return false;
 
   if (current_write_ != nullptr) {
-    WriteWrap* w = current_write_;
-    current_write_ = nullptr;
+    WriteWrap* w = current_write_.release();
     w->Done(status, error_str);
   }
 
@@ -617,7 +616,7 @@ void TLSWrap::ClearError() {
 
 
 // Called by StreamBase::Write() to request async write of clear text into SSL.
-int TLSWrap::DoWrite(WriteWrap* w,
+int TLSWrap::DoWrite(std::unique_ptr<WriteWrap>& w,
                      uv_buf_t* bufs,
                      size_t count,
                      uv_stream_t* send_handle) {
@@ -651,7 +650,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
     if (BIO_pending(enc_out_) == 0) {
       Debug(this, "No pending encrypted output, writing to underlying stream");
       CHECK_NULL(current_empty_write_);
-      current_empty_write_ = w;
+      current_empty_write_ = w.get();
       StreamWriteResult res =
           underlying_stream()->Write(bufs, count, send_handle);
       if (!res.async) {
@@ -666,7 +665,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
 
   // Store the current write wrap
   CHECK_NULL(current_write_);
-  current_write_ = w;
+  current_write_ = std::move(w);
 
   // Write encrypted data to underlying stream and call Done().
   if (length == 0) {
@@ -705,7 +704,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
     // If we stopped writing because of an error, it's fatal, discard the data.
     if (!arg.IsEmpty()) {
       Debug(this, "Got SSL error (%d), returning UV_EPROTO", err);
-      current_write_ = nullptr;
+      current_write_.release();
       return UV_EPROTO;
     }
 
@@ -718,6 +717,8 @@ int TLSWrap::DoWrite(WriteWrap* w,
   // Write any encrypted/handshake output that may be ready.
   EncOut();
 
+  w.reset(current_write_.get());
+
   return 0;
 }
 

src/tls_wrap.h

@@ -67,7 +67,7 @@ class TLSWrap : public AsyncWrap,
   ShutdownWrap* CreateShutdownWrap(
       v8::Local<v8::Object> req_wrap_object) override;
   int DoShutdown(ShutdownWrap* req_wrap) override;
-  int DoWrite(WriteWrap* w,
+  int DoWrite(std::unique_ptr<WriteWrap>& w,
               uv_buf_t* bufs,
               size_t count,
               uv_stream_t* send_handle) override;
@@ -170,7 +170,7 @@ class TLSWrap : public AsyncWrap,
   // Waiting for ClearIn() to pass to SSL_write().
   std::vector<char> pending_cleartext_input_;
   size_t write_size_ = 0;
-  WriteWrap* current_write_ = nullptr;
+  std::unique_ptr<WriteWrap> current_write_ = nullptr;
   WriteWrap* current_empty_write_ = nullptr;
   bool write_callback_scheduled_ = false;
   bool started_ = false;