Skip to content

Look the SSLContext up from the SSL_CTX in the ALPN/NPN callbacks #1089

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jeremy wants to merge 1 commit into
base: master
Choose a base branch
from
+20 −6
Open

Look the SSLContext up from the SSL_CTX in the ALPN/NPN callbacks #1089

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions ext/openssl/ossl_ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,19 @@ ossl_sslctx_mark(void *ptr)
rb_gc_mark_movable((VALUE)SSL_CTX_get_ex_data(ctx, ossl_sslctx_ex_ptr_idx));
}

/*
* The SSLContext's VALUE is stored in exactly one place -- the SSL_CTX's ex_data,
* which ossl_sslctx_compact keeps up to date. Callbacks must go through here rather
* than capture their own copy, which nothing would relocate.
*/
static VALUE
ossl_sslctx_obj_from_ssl(const SSL *ssl)
{
SSL_CTX *ctx = SSL_get_SSL_CTX(ssl);

return (VALUE)SSL_CTX_get_ex_data(ctx, ossl_sslctx_ex_ptr_idx);
}

Comment on lines +62 to +74

@rhenium rhenium Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this closer to the callers?

static void
ossl_sslctx_free(void *ptr)
{
Expand Down Expand Up @@ -584,7 +597,8 @@ static int
ssl_npn_advertise_cb(SSL *ssl, const unsigned char **out, unsigned int *outlen,
void *arg)
{
VALUE protocols = rb_attr_get((VALUE)arg, id_npn_protocols_encoded);
VALUE protocols = rb_attr_get(ossl_sslctx_obj_from_ssl(ssl),
id_npn_protocols_encoded);

*out = (const unsigned char *) RSTRING_PTR(protocols);
*outlen = RSTRING_LENINT(protocols);
Expand All @@ -598,7 +612,7 @@ ssl_npn_select_cb(SSL *ssl, unsigned char **out, unsigned char *outlen,
{
VALUE sslctx_obj, cb;

sslctx_obj = (VALUE) arg;
sslctx_obj = ossl_sslctx_obj_from_ssl(ssl);
cb = rb_attr_get(sslctx_obj, id_i_npn_select_cb);

return ssl_npn_select_cb_common(ssl, cb, (const unsigned char **)out,
Expand All @@ -612,7 +626,7 @@ ssl_alpn_select_cb(SSL *ssl, const unsigned char **out, unsigned char *outlen,
{
VALUE sslctx_obj, cb;

sslctx_obj = (VALUE) arg;
sslctx_obj = ossl_sslctx_obj_from_ssl(ssl);
cb = rb_attr_get(sslctx_obj, id_i_alpn_select_cb);

return ssl_npn_select_cb_common(ssl, cb, out, outlen, in, inlen);
Expand Down Expand Up @@ -807,11 +821,11 @@ ossl_sslctx_setup(VALUE self)
if (!NIL_P(val)) {
VALUE encoded = ssl_encode_npn_protocols(val);
rb_ivar_set(self, id_npn_protocols_encoded, encoded);
SSL_CTX_set_next_protos_advertised_cb(ctx, ssl_npn_advertise_cb, (void *)self);
SSL_CTX_set_next_protos_advertised_cb(ctx, ssl_npn_advertise_cb, NULL);
OSSL_Debug("SSL NPN advertise callback added");
}
if (RTEST(rb_attr_get(self, id_i_npn_select_cb))) {
SSL_CTX_set_next_proto_select_cb(ctx, ssl_npn_select_cb, (void *) self);
SSL_CTX_set_next_proto_select_cb(ctx, ssl_npn_select_cb, NULL);
OSSL_Debug("SSL NPN select callback added");
}
#endif
Expand All @@ -827,7 +841,7 @@ ossl_sslctx_setup(VALUE self)
OSSL_Debug("SSL ALPN values added");
}
if (RTEST(rb_attr_get(self, id_i_alpn_select_cb))) {
SSL_CTX_set_alpn_select_cb(ctx, ssl_alpn_select_cb, (void *) self);
SSL_CTX_set_alpn_select_cb(ctx, ssl_alpn_select_cb, NULL);

@rhenium rhenium Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSL_get_SSL_CTX() is not reliable in server-side callbacks (where the SSL_CTX might have been replaced by the SNI callback). Please pass the original ctx explicitly to the callback instead.

@jeremy jeremy Aug 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a second look at the SNI case: I think the lookup tracks it correctly. OpenSSL fetches the callback and its arg from the same SSL_CTX. tls_handle_alpn does sctx = SSL_CONNECTION_GET_CTX(s) and then sctx->ext.alpn_select_cb(..., sctx->ext.alpn_select_cb_arg), and SSL_CONNECTION_GET_CTX(sc) is (sc)->ssl.ctx, which is exactly what SSL_get_SSL_CTX() returns. Inside the callback the two can't disagree.

And after an SNI swap the new context is the one we want. SSL_set_SSL_CTX() doesn't copy ext.alpn_select_cb or its arg, and ossl_call_servername_cb calls ossl_sslctx_setup(ret_obj) before the swap, so on master, post-SNI, arg is already the new SSLContext. Passing the original ctx would be the behaviour change.

If you'd rather not lean on SSL_get_SSL_CTX() here, rb_attr_get(SSL_get_ex_data(ssl, ossl_ssl_ex_ptr_idx), id_i_context) tracks the swap too, and I'm happy to switch to that. I'd just want to avoid going back to a captured arg, since that's the stale VALUE compaction leaves behind.

@rhenium rhenium Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I had assumed the ALPN/NPN callbacks were part of the configuration copied to the SSL object at SSL_new() time. Thanks for verifying the implementation.

Passing the original ctx here wouldn't break anything, but it looks like using SSL_get_SSL_CTX() is fine in this case.

OSSL_Debug("SSL ALPN select callback added");
}

Expand Down