Deadlock when using SSLContext._msg_callback and SSLContext.sni_callback #87743
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
assignee = 'https://github.com/tiran' closed_at = <Date 2021-03-21.16:03:49.009> created_at = <Date 2021-03-21.05:00:42.206> labels = ['expert-SSL', 'type-bug', '3.8', '3.9', '3.10'] title = 'Deadlock when using SSLContext._msg_callback and SSLContext.sni_callback' updated_at = <Date 2021-03-21.16:03:49.008> user = 'https://github.com/theandrew168'
activity = <Date 2021-03-21.16:03:49.008> actor = 'christian.heimes' assignee = 'christian.heimes' closed = True closed_date = <Date 2021-03-21.16:03:49.009> closer = 'christian.heimes' components = ['SSL'] creation = <Date 2021-03-21.05:00:42.206> creator = 'theandrew168' dependencies =  files = ['49897'] hgrepos =  issue_num = 43577 keywords = ['patch'] message_count = 8.0 messages = ['389216', '389222', '389229', '389232', '389234', '389235', '389238', '389240'] nosy_count = 3.0 nosy_names = ['christian.heimes', 'miss-islington', 'theandrew168'] pr_nums = ['24957', '24958', '24959'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue43577' versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']
The text was updated successfully, but these errors were encountered:
I think I might've stumbled onto an oversight with how an SSLSocket handles overwriting its SSLContext within an sni_callback. If both "_msg_callback" and "sni_callback" are defined on an SSLContext object and the sni_callback replaces the context with new one, the interpreter locks up indefinitely. It fails to respond to keyboard interrupts and must be forcefully killed.
This seems to be a common use case of the sni_callback: create a new context with a different cert chain and attach it to the current socket (which replaces the existing one). If _msg_callback never gets defined on the original context then this deadlock never occurs. Curiously, if you assign the same _msg_callback to the new context before replacement, this also avoids the deadlock.
I've attached as minimal of a reproduction as I could come up with. I think the code within will probably do a better job explaining this problem than I've done here in prose. I've only tested it on a couple Linux distros (Ubuntu Server and Void Linux) but the lock occurs 100% of the time in my experience.
In the brief time I've spent digging into the CPython source, I've come to understand that replacing the SSLContext on an SSLSocket isn't "just" a simple replacement but actually involves some OpenSSL mechanics (specifically, SSL_set_SSL_CTX) . I'm wondering if maybe this context update routine isn't properly cleaning up whatever resources / references were being used by the msg_callback? Maybe this is even closer to an OpenSSL bug (or a least a gotcha)?
I also feel the need to explain why I'd even be using an undocumented property (SSLContext._msg_callback) in the first place. I'm trying to implement a program that automatically manages TLS certs on a socket via Let's Encrypt and the ACME protocol (RFC8555). Part of this process involves serving up a specific cert when a connection requests the acme-tls/1 ALPN protocol. Given the existing Python SSL API, I don't believe there is any way for me to do this "correctly".
The documentation for SSLContext.sni_callback  mentions that the selected_alpn_protocol function should be usable within the callback but I don't that is quite true. According to the OpenSSL docs :
If there is a better way for me to identify a specific ALPN protocol _before_ the sni_callback, I could definitely use the guidance. That would avoid this deadlock altogether (even though it'd still be waiting to catch someone else...).
This is my first Python issue so I hope what I've supplied makes sense. If there is anything more I can do to help or provide more info, please let me know.
Thanks for the excellent bug report and reproducer! I have identified the issue and submitted a fix for review. OpenSSL copies the internal msg_callback to SSL struct, but SSL_set_SSL_CTX() does not update the msg_callback with value from new context.
Could you please open a new bug regarding the issue with SNI and ALPN order? This is unrelated. It looks like OpenSSL processes the ALPN extension after the SNI extension, https://github.com/openssl/openssl/blob/abded2ced44b94d96f08ea5cf01df6519b80f5d3/ssl/ssl_local.h#L740-L769 . I can see that the state machines fires "final_server_name" first (which triggers the SNI callback), then "tls_handle_alpn". This makes sense. This allows the new context to select ALPNs.
#0 final_server_name (s=0x8a4080, context=128, sent=1) at ssl/statem/extensions.c:925
#0 tls_handle_alpn (s=0x8a4080) at ssl/statem/statem_srvr.c:2167
I'm glad that the info I provided was helpful! I'll go ahead and create another issue for the misleading docs surrounding SSLContext.sni_callback. Thanks for looking into this and coming up with a fix so quickly.
I do have one more question: does python provide a "safe" way to test for deadlocks like this? I noticed that you added a test case to verify that this lockup doesn't happen but what would happen if someone ran that test on an earlier version? Would the test runner also freeze or are there facilities in-place to catch such behavior? Maybe something nutty like:
with should_deadlock(): my_buggy_test()
No, there is no check for that. This kind of deadlock should never occur. The problem was an implementation bug in low-level C code that had bad interaction with the global interpreter lock. Python releases the GIL around OpenSSL calls. Callbacks have to re-acquire the GIL correctly and release it again at the end of a callback.
By the way the _msg_cb attribute is deliberately undocumented and marked as an internal property. I implemented the callback to debug some issues with TLS 1.3 and OpenSSL 1.1.1. It's neither well tested nor stable.