Request for feedback: Updating SSL to align with stdlib #957
Conversation
|
n.b. there have been a few CVEs and other security reports filed against downstream users of eventmachine: I'd wager most EM-using applications were vulnerable to this and probably a significant percentage still are. I discovered this issue in a couple of my apps and fixed it myself (several times) many years before those CVEs. But I'm ashamed I never upstreamed my fixes back then (to be clear, my fixes were in pure ruby and almost identical to what em-http-request has done). IMO this is something that needs to work securely by default, and those CVEs belong to this project. |
|
FWIW, concerning my third proposed implementation approach, see ruby/ruby@5cdf99f and ruby/openssl#475. If it can be done safely, IMO that approach is preferable over the approach I took in this PR. |
25cd75b to
17c0023
Compare
First: this branch isn't ready to be merged. It's still a bit sloppy and incomplete. I'm looking for feedback on the basic approach, API design, etc. If the basic approach for the API is okay (whether or not the actual implementation is good), I'd like to split it into a series of much smaller simpler PRs (re-implementing as necessary). I'm hopeful that someone can tell me there's a simpler, easier way to accomplish what I wrote in this draft PR. ;)
Issues
EventMachine is full of old SSL code, which isn't following best practices. It is much easier to write insecure code than to write secure code: there are no secure defaults. Debugging errors can be a pain. EventMachine's interface with OpenSSL feels very different from stdlib's, which can lead to confusion (and bugs and security holes).
Additionally, the internal API for setting the context:
EventMachine.set_tls_paramsis a big mess, and it's only going to get messier if we keep using the same approach.In particular, I wanted to add a default
ca_store, the ability to configureca_pathandca_file, and averify_hostnameoption which uses theopensslgem's algorithm. I want to delegate all certificate and identity verification to the openssl library, and have the ability to easily diagnose specific errors from ruby.Proposal:
My proposal is to move as close as possible to stdlib's API and implementation , while maintaining a reasonable backward compatibility with the original EM API.
API changes
The API for internal methods, such as
EM.set_tls_params,EM.start_tls, and even e.g.EM.get_cipher_*do not to be protected from change. Applications and other gems should not rely on those. However it might be worth doing a quick survey of reverse dependencies on rubygems.org to see if anyone is using them.Where the default
EM.start_tlswith no parameters defaults are insecure, we should break compatibility. The new defaults should mimic stdlib'sOpenSSL::SSL::SSLContext::DEFAULT_PARAMSas much as it is feasible to do so. And we should run a RFC6125 hostname verification procedure by default just like stdlib (seeOpenSSL::SSL::SSLSocket#post_connection_checkandOpenSSL::SSL.verify_certificate_identity).Instead of passing a dozen or more parameters from
EM::Connection#start_tlstoEM.set_tls_parmstot_set_tls_parmstoConnectionDescriptor::SetTlsParmsto a bunch of instance variables onConnectionDescriptorto instance variables onSslContext_tandSslBox_tto the actual underlyingSSL_CTXandSSLobjects, we should create a ruby class that wraps all of theSSL_CTXparams. This current PR unloads that ruby object into into a C struct which is delivered into aSslContext_tobject. But ideally we could make our ruby object just aTypedData_Wrap_Structaround theSSL_CTXand replaceSslContext_taltogether. Both the OpenSSL project and theopensslgem encapsulatedSSL_CTXinto a data type. So should EM! We can entirely dispose ofEM.set_tls_parmsand simply changestart_tlsto take ourSSL_CTXwrapper and whatever other options remain for theSSLobject, e.g. SNI hostname.Adding a ruby class that is analogous to
OpenSSL::SSL::SSLSocketseems less necessary: the EventMachine approach has always been query methods and callback methods on theEM::Connectionmodule/class. However, EM's C++ code has already found it useful to split offSslBox_t, and keeping a small separation allows us to change our API inside ourSSLandSSL_CTXwrappers, while maintaining backwards compatibility viaEM:::Connection. E.g. this could be an opportunity to have a singleciphermethod instead of threeget_cipher_*methods. Andpeer_certcan returnOpenSSL::X509::Certificateobjects (from stdlib), instead of simple PEM strings. AndEM::SSL::X509StoreContextcan be sent to theEM::SSL::Context#verify_callback. Etc.The direction I'm going is that the ruby
EM::Connectionclass doesn't need to know about SSL at all. Instead of callingEM::Connection#start_tls(**params)and using the two connection callback methods, the new recommended API could mimic stdlib:backwards compatibility
EM::Connection#start_tlsas just a simple implementation of the above. It can support both the old API and the new API. If a context is sent in, great! Use it. Otherwise, take the arguments Hash and use it to build a new context like above.😜
EM::SSL::BackwardsCompatiblemixin that could be used to bring back the existing behavior. It could be included in the next release but deprecated and available but not included inEM::Connectionin the next release. That could allow the basicEM::Connectioncode to be a little bit simpler.Implementation changes
So... this PR is sloppy. Sorry. Consider it a first-draft proof of concept?
Moving the implementation as closely as possible to stdlib isn't a nice-to-have. It's critically important: stdlib has far more active maintainers and resources devoted to it. In my branch, I copied and pasted large chunks of code (C and Ruby) from the
opensslgem. This was just a quick exploration. Ideally, we should be able to "inherit" bugfixes and improvements from stdlib. This could be accomplished in a few ways:opensslupdates.SSL_CTX,SSL, etc fromVALUE-- Another alternative might be to hack create actual stdlib objects, hack the wrappedT_DATAobjects to extract the underlyingSSL_CTX,SSL, etc, and callSSL_dupto have something that's been configured by ruby but is completely under EM control. I'm not 100% certain that's feasible forSSLbut I think it could be doable with `SSL_CTX. One very big concern of mine is that I don't know C linking and ABIs well enough to know if it's actually safe to do this.opensslinclude some of its header files intoproject.h, and link directly against wherever possible. Again, I'm not really sure how "safe" this is wrt openssl ABIs. But it might enable us to use the C functions defined in the gem, without ABI incompatibility issues.OpenSSL::SSL::SSLSocketto the underlying IO, and then proxy reads and writes through thatSSLSocket. Essentially, replaceSslBox_t *SslBoxwithVALUE SSLSocket, and replace everyif (SslBox) {...}inConnectionDescripterwith a similarif (SslSocket) { ... }. Although calling it from C/C++, we'd only use the ruby API. In this case, EventMachine wouldn't even need to include the openssl headers.I did some research, and there do appear to be occasional minor ABI incompatibilities between different versions of openssl?
https://abi-laboratory.pro/?view=timeline&l=openssl So I don't know if that can be made safe. I'm only a C/C++ tourist. ;)
But pushing as much of our SSL code into stdlib's
opensslgem as possible seems like a wise approach. Perhaps there's a set of patches we could submit to theopensslgem which might let us use it more directly without sacrificing performance?