diff options
author | Daniel Bevenius <daniel.bevenius@gmail.com> | 2020-12-02 18:21:41 +0100 |
---|---|---|
committer | Richard Lau <rlau@redhat.com> | 2020-12-23 14:14:29 +0000 |
commit | 7f178663ebffc82c9f8a5a1b6bf2da0c263a30ed (patch) | |
tree | ba090b41447a57e0b6a7b3136bee9a039b05c5fc | |
parent | f08d0fef64f71090ae2fab93ce6b19c392410863 (diff) | |
download | node-new-7f178663ebffc82c9f8a5a1b6bf2da0c263a30ed.tar.gz |
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: https://github.com/nodejs-private/node-private/issues/227
PR-URL: https://github.com/nodejs-private/node-private/pull/238
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
-rw-r--r-- | src/js_stream.cc | 4 | ||||
-rw-r--r-- | src/js_stream.h | 2 | ||||
-rw-r--r-- | src/node_file.h | 2 | ||||
-rw-r--r-- | src/node_http2.cc | 4 | ||||
-rw-r--r-- | src/node_http2.h | 2 | ||||
-rw-r--r-- | src/stream_base-inl.h | 8 | ||||
-rw-r--r-- | src/stream_base.h | 9 | ||||
-rw-r--r-- | src/stream_wrap.cc | 4 | ||||
-rw-r--r-- | src/stream_wrap.h | 2 | ||||
-rw-r--r-- | src/tls_wrap.cc | 13 | ||||
-rw-r--r-- | 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 e3d734c015..4054e90b2e 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<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 }; diff --git a/src/js_stream.h b/src/js_stream.h index 6612e558ae..bf0d15d462 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<WriteWrap>& 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 cbbb8b037d..b440c143b9 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<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 { diff --git a/src/node_http2.cc b/src/node_http2.cc index f1f6e1397a..c81e202943 100644 --- a/src/node_http2.cc +++ b/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); diff --git a/src/node_http2.h b/src/node_http2.h index 1526e0b47e..d1d523edcb 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<WriteWrap>& 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 027b938d30..dca02ac76a 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<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> diff --git a/src/stream_base.h b/src/stream_base.h index 65abd4dcf4..3e922a4ac4 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<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; } diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 10444fea4a..bd512e3349 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<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); diff --git a/src/stream_wrap.h b/src/stream_wrap.h index 98f0ca4ac4..3c00d339af 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<WriteWrap>& 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 ce46e2163e..65ea8841b7 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<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; } diff --git a/src/tls_wrap.h b/src/tls_wrap.h index bfcf07bbc8..e2e748b859 100644 --- a/src/tls_wrap.h +++ b/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; |