Look the SSLContext up from the SSL_CTX in the ALPN/NPN callbacks - #1089
Look the SSLContext up from the SSL_CTX in the ALPN/NPN callbacks#1089jeremy wants to merge 1 commit into
Conversation
ossl_sslctx_mark uses rb_gc_mark_movable, so the SSLContext relocates. Its VALUE is stored in four places: the SSL_CTX's ex_data, and the callback argument of the NPN advertise, NPN select and ALPN select callbacks. ossl_sslctx_compact updates the first. Nothing updates the other three, so after a compaction they hold the pre-move address. The three callbacks all receive the SSL, and the SSL_CTX's ex_data copy is already kept current -- so they can look the object up instead of carrying their own copy, which leaves exactly one stored copy and one place to maintain. Registration is one-shot (ossl_sslctx_setup returns early when self is frozen), so the stale address is captured at the first handshake and never refreshed. Fixes ruby#1088.
| } | ||
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| /* | ||
| * 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); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you move this closer to the callers?
Fixes #1088.
ossl_sslctx_markusesrb_gc_mark_movable, so the SSLContext relocates. ItsVALUEis stored in four places:SSL_CTXex_dataossl_sslctx_compactSSL_CTX_set_next_protos_advertised_cbarg (:810)SSL_CTX_set_next_proto_select_cbarg (:814)SSL_CTX_set_alpn_select_cbarg (:830)After a compaction the three callback arguments hold the pre-move address, and each callback does
rb_attr_geton it. Registration is one-shot —ossl_sslctx_setupreturns early whenselfis frozen — so the address is captured at the first handshake and never refreshed.This is a regression from the switch to
rb_gc_mark_movable; every release pins withrb_gc_markand is unaffected.Approach
Rather than adding three fix-ups to
ossl_sslctx_compact, this removes the three extra copies. All three callbacks already receive theSSL, andSSL_get_SSL_CTX(ssl)'s ex_data is the copy that is already maintained — so they can look the object up and passNULLas the callback argument.That leaves exactly one stored copy and one place to keep current, which seemed worth more than the alternative:
ossl_sslctx_compactwould otherwise have to re-register all three callbacks from inside a GC compaction callback, since OpenSSL exposes no getter for these arguments.The helper sits next to the other ex_data accessors and mirrors the idiom already used in the session-remove callback.
Verification
Built HEAD (
9796ee8) and the patched tree in the same step that ran the tests, so the artifact can't drift from the source. Ruby 4.0.6, OpenSSL 3.5.6, in a container.The two NPN sites are isolated by putting the server and client contexts in different processes (fork + pipes), so exactly one of them is subject to the compaction — otherwise the server fails first and masks the client site entirely.
Both ends pin
max_versionto TLS 1.2. That detail is why #1088 recorded the NPN sites as "code reading only": NPN isn't sent at TLS 1.3, so an unconstrained test negotiates 1.3, neither callback fires, and the run looks clean. The scripts now assert the callbacks actually ran before reporting anything.Failure modes observed on master, both from the same defect depending on what lands in the vacated slot:
:810— SEGV inssl_npn_advertise_cb, faulting at0x4, i.e.RSTRING_PTR(Qnil):814—NoMethodError: undefined method 'call' for nilout ofSSLSocket#connect, with the peer loggingsslv3 alert handshake failureReachability
npn_protocols=,npn_select_cb=andalpn_select_cb=are public documented accessors. The trigger is the application's ownGC.compact, not anything an attacker supplies, which is why this is a public issue rather than a private report.One honest caveat: confirmed under
GC.verify_compaction_references. I could not get a plainGC.compactto relocate the SSLContext in a small process, so I'm not claiming a spontaneous production rate — every run where the context did relocate failed (30/30), and every run where it didn't survived (60/60).