Skip to content

Commit

Permalink
src: fix TLSWrap lifetime bug in ALPN callback
Browse files Browse the repository at this point in the history
Retrieve the TLSWrap from the SSL object, not SSL_CTX.

A SSL_CTX object is the parent of zero or more SSL objects. TLSWrap is a
wrapper around SSL, SecureContext around SSL_CTX.

Node.js normally uses a SecureContext per TLSWrap but it is possible to
use a SecureContext object more than once.

It was therefore possible for an ALPN callback to use the wrong
(possibly already freed) TLSWrap object.

Having said that, while the bug is clear once you see it, I'm not able
to trigger it (and hence no test, not for lack of trying.)

None of the bug reporters were able to reliably reproduce it either so
the stars probably need to align just right in order to hit it.

Fixes: #47207
PR-URL: #49635
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
  • Loading branch information
bnoordhuis committed Oct 4, 2023
1 parent 66776d8 commit 1643adf
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions src/crypto/crypto_tls.cc
Expand Up @@ -223,7 +223,7 @@ int SelectALPNCallback(
const unsigned char* in,
unsigned int inlen,
void* arg) {
TLSWrap* w = static_cast<TLSWrap*>(arg);
TLSWrap* w = static_cast<TLSWrap*>(SSL_get_app_data(s));
if (w->alpn_callback_enabled_) {
Environment* env = w->env();
HandleScope handle_scope(env->isolate());
Expand Down Expand Up @@ -1293,7 +1293,8 @@ void TLSWrap::EnableALPNCb(const FunctionCallbackInfo<Value>& args) {
wrap->alpn_callback_enabled_ = true;

SSL* ssl = wrap->ssl_.get();
SSL_CTX_set_alpn_select_cb(SSL_get_SSL_CTX(ssl), SelectALPNCallback, wrap);
SSL_CTX* ssl_ctx = SSL_get_SSL_CTX(ssl);
SSL_CTX_set_alpn_select_cb(ssl_ctx, SelectALPNCallback, nullptr);
}

void TLSWrap::GetServername(const FunctionCallbackInfo<Value>& args) {
Expand Down Expand Up @@ -1589,7 +1590,8 @@ void TLSWrap::SetALPNProtocols(const FunctionCallbackInfo<Value>& args) {
} else {
w->alpn_protos_ = std::vector<unsigned char>(
protos.data(), protos.data() + protos.length());
SSL_CTX_set_alpn_select_cb(SSL_get_SSL_CTX(ssl), SelectALPNCallback, w);
SSL_CTX* ssl_ctx = SSL_get_SSL_CTX(ssl);
SSL_CTX_set_alpn_select_cb(ssl_ctx, SelectALPNCallback, nullptr);
}
}

Expand Down

0 comments on commit 1643adf

Please sign in to comment.