282 lines
10 KiB
Diff
282 lines
10 KiB
Diff
|
|
From 7f178663ebffc82c9f8a5a1b6bf2da0c263a30ed Mon Sep 17 00:00:00 2001
|
||
|
|
From: Daniel Bevenius <daniel.bevenius@gmail.com>
|
||
|
|
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 <midawson@redhat.com>
|
||
|
|
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
|
||
|
|
Reviewed-By: Richard Lau <rlau@redhat.com>
|
||
|
|
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<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 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<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 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<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 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<WriteWrap>& 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<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 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<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 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<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 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<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 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<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 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<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 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<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;
|
||
|
|
--
|
||
|
|
2.23.0
|
||
|
|
|