Skip to content

Commit

Permalink
tls: prevent use-after-free
Browse files Browse the repository at this point in the history
* Destroy `SSL*` and friends on a next tick to make sure that we are not
  doing it in one of the OpenSSL callbacks
* Add more checks to the C++ methods that might be invoked during
  destructor's pending queue cleanup

Fix: nodejs/node-v0.x-archive#8780
Fix: #1696
PR-URL: #1702
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  • Loading branch information
indutny committed Jun 4, 2015
1 parent 5795e83 commit 75930bb
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 1 deletion.
7 changes: 6 additions & 1 deletion lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -305,13 +305,18 @@ TLSSocket.prototype._wrapHandle = function(handle) {
});

this.on('close', function() {
this._destroySSL();
// Make sure we are not doing it on OpenSSL's stack
setImmediate(destroySSL, this);
res = null;
});

return res;
};

function destroySSL(self) {
self._destroySSL();
}

TLSSocket.prototype._destroySSL = function _destroySSL() {
if (!this.ssl) return;
this.ssl.destroySSL();
Expand Down
13 changes: 13 additions & 0 deletions src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,9 @@ void TLSWrap::EncOutCb(WriteWrap* req_wrap, int status) {
return;
}

if (wrap->ssl_ == nullptr)
return;

// Commit
NodeBIO::FromBIO(wrap->enc_out_)->Read(nullptr, wrap->write_size_);

Expand Down Expand Up @@ -554,6 +557,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
size_t count,
uv_stream_t* send_handle) {
CHECK_EQ(send_handle, nullptr);
CHECK_NE(ssl_, nullptr);

bool empty = true;

Expand Down Expand Up @@ -627,6 +631,11 @@ void TLSWrap::OnAfterWriteImpl(WriteWrap* w, void* ctx) {
void TLSWrap::OnAllocImpl(size_t suggested_size, uv_buf_t* buf, void* ctx) {
TLSWrap* wrap = static_cast<TLSWrap*>(ctx);

if (wrap->ssl_ == nullptr) {
*buf = uv_buf_init(nullptr, 0);
return;
}

size_t size = 0;
buf->base = NodeBIO::FromBIO(wrap->enc_in_)->PeekWritable(&size);
buf->len = size;
Expand Down Expand Up @@ -747,6 +756,10 @@ void TLSWrap::SetVerifyMode(const FunctionCallbackInfo<Value>& args) {
void TLSWrap::EnableSessionCallbacks(
const FunctionCallbackInfo<Value>& args) {
TLSWrap* wrap = Unwrap<TLSWrap>(args.Holder());
if (wrap->ssl_ == nullptr) {
return wrap->env()->ThrowTypeError(
"EnableSessionCallbacks after destroySSL");
}
wrap->enable_session_callbacks();
NodeBIO::FromBIO(wrap->enc_in_)->set_initial(kMaxHelloLength);
wrap->hello_parser_.Start(SSLWrap<TLSWrap>::OnClientHello,
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-tls-js-stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ var server = tls.createServer({

socket.end('hello');
socket.resume();
socket.destroy();
});

socket.once('close', function() {
Expand Down

0 comments on commit 75930bb

Please sign in to comment.