From 7f178663ebffc82c9f8a5a1b6bf2da0c263a30ed Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Wed, 2 Dec 2020 18:21:41 +0100 Subject: [PATCH] src: use unique_ptr for WriteWrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs-private/node-private/issues/227 PR-URL: https://github.com/nodejs-private/node-private/pull/238 Reviewed-By: Michael Dawson Reviewed-By: Tobias Nießen Reviewed-By: Richard Lau Reference: https://github.com/nodejs/node/commit/7f178663ebffc82c9f8a5a1b6bf2da0c263a30ed --- src/js_stream.cc | 4 ++-- src/js_stream.h | 2 +- src/node_file.h | 2 +- src/node_http2.cc | 4 ++-- src/node_http2.h | 2 +- src/stream_base-inl.h | 8 ++++---- src/stream_base.h | 9 +++++---- src/stream_wrap.cc | 4 ++-- src/stream_wrap.h | 2 +- src/tls_wrap.cc | 13 +++++++------ src/tls_wrap.h | 4 ++-- 11 files changed, 28 insertions(+), 26 deletions(-) diff --git a/src/js_stream.cc b/src/js_stream.cc index e3d734c..4054e90 100644 --- a/src/js_stream.cc +++ b/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& w, uv_buf_t* bufs, size_t count, uv_stream_t* send_handle) { @@ -122,7 +122,7 @@ int JSStream::DoWrite(WriteWrap* w, } Local argv[] = { - w->object(), + w.get()->object(), bufs_arr }; diff --git a/src/js_stream.h b/src/js_stream.h index 6612e55..bf0d15d 100644 --- a/src/js_stream.h +++ b/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& w, uv_buf_t* bufs, size_t count, uv_stream_t* send_handle) override; diff --git a/src/node_file.h b/src/node_file.h index cbbb8b0..b440c14 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -287,7 +287,7 @@ class FileHandle : public AsyncWrap, public StreamBase { ShutdownWrap* CreateShutdownWrap(v8::Local object) override; int DoShutdown(ShutdownWrap* req_wrap) override; - int DoWrite(WriteWrap* w, + int DoWrite(std::unique_ptr& w, uv_buf_t* bufs, size_t count, uv_stream_t* send_handle) override { diff --git a/src/node_http2.cc b/src/node_http2.cc index 9bde444..2a523a4 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2314,7 +2314,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& req_wrap, uv_buf_t* bufs, size_t nbufs, uv_stream_t* send_handle) { @@ -2329,7 +2329,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); diff --git a/src/node_http2.h b/src/node_http2.h index 1526e0b..d1d523e 100644 --- a/src/node_http2.h +++ b/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& w, uv_buf_t* bufs, size_t count, uv_stream_t* send_handle) override; void MemoryInfo(MemoryTracker* tracker) const override { diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 027b938..dca02ac 100644 --- a/src/stream_base-inl.h +++ b/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 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 diff --git a/src/stream_base.h b/src/stream_base.h index 65abd4d..3e922a4 100644 --- a/src/stream_base.h +++ b/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& 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; } diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 10444fe..bd512e3 100644 --- a/src/stream_wrap.cc +++ b/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& req_wrap, uv_buf_t* bufs, size_t count, uv_stream_t* send_handle) { - LibuvWriteWrap* w = static_cast(req_wrap); + LibuvWriteWrap* w = static_cast(req_wrap.get()); int r; if (send_handle == nullptr) { r = w->Dispatch(uv_write, stream(), bufs, count, AfterUvWrite); diff --git a/src/stream_wrap.h b/src/stream_wrap.h index 98f0ca4..3c00d33 100644 --- a/src/stream_wrap.h +++ b/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& w, uv_buf_t* bufs, size_t count, uv_stream_t* send_handle) override; diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index ce46e21..65ea884 100644 --- a/src/tls_wrap.cc +++ b/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& 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; } diff --git a/src/tls_wrap.h b/src/tls_wrap.h index bfcf07b..e2e748b 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -67,7 +67,7 @@ class TLSWrap : public AsyncWrap, ShutdownWrap* CreateShutdownWrap( v8::Local req_wrap_object) override; int DoShutdown(ShutdownWrap* req_wrap) override; - int DoWrite(WriteWrap* w, + int DoWrite(std::unique_ptr& 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 pending_cleartext_input_; size_t write_size_ = 0; - WriteWrap* current_write_ = nullptr; + std::unique_ptr current_write_ = nullptr; WriteWrap* current_empty_write_ = nullptr; bool write_callback_scheduled_ = false; bool started_ = false; -- 2.23.0