From 9996b7b8db3a8c8f57c56854730a085955a652f0 Mon Sep 17 00:00:00 2001 From: Lawrence Forooghian Date: Thu, 25 Apr 2024 12:00:53 -0300 Subject: [PATCH] Fix server certificate verification For ably/ably-ruby#396. --- lib/em-http/http_connection.rb | 122 +++++++++++++++++++++++++++------ 1 file changed, 101 insertions(+), 21 deletions(-) diff --git a/lib/em-http/http_connection.rb b/lib/em-http/http_connection.rb index 3627a28..db003e9 100644 --- a/lib/em-http/http_connection.rb +++ b/lib/em-http/http_connection.rb @@ -40,36 +40,118 @@ def unbind(reason=nil) # TLS verification support, original implementation by Mislav Marohnić # https://github.com/lostisland/faraday/blob/63cf47c95b573539f047c729bd9ad67560bc83ff/lib/faraday/adapter/em_http_ssl_patch.rb + # + # Updated by Ably, here’s why: + # + # We noticed that the existing verification mechanism is failing in the + # case where the certificate chain presented by the server contains a + # certificate that’s signed by an expired trust anchor. At the time of + # writing, this is the case with some Let’s Encrypt certificate chains, + # which contain a cross-sign by the expired DST Root X3 CA. + # + # This isn’t meant to be an issue; the certificate chain presented by the + # server still contains a certificate that’s a trust anchor in most + # modern systems. So in the case where this trust anchor exists, OpenSSL + # would instead construct a certification path that goes straight to that + # anchor, bypassing the expired certificate. + # + # Unfortunately, as described in + # https://github.com/eventmachine/eventmachine/issues/954#issue-1014842247, + # EventMachine misuses OpenSSL in a variety of ways. One of them is that + # it does not configure a list of trust anchors, meaning that OpenSSL is + # unable to construct the correct certification path in the manner + # described above. + # + # This means that we end up in a degenerate situation where + # ssl_verify_peer just receives the certificates in the chain provided by + # the peer. In the scenario described above, one of these certificates is + # expired and hence the existing verification mechanism, which "verifies" + # each certificate provided to ssl_verify_peer, fails. + # + # So, instead we remove the existing ad-hoc mechanism for verification + # (which did things I’m not sure it should have done, like putting + # non-trust-anchors into an OpenSSL::X509::Store) and instead employ + # OpenSSL (configured to use the system trust store, and hence able to + # construct the correct certification path) to do all the hard work of + # constructing the certification path and then verifying the peer + # certificate. (This is what, in my opinion, EventMachine ideally would + # be allowing OpenSSL to do in the first place. Instead, as far as I can + # tell, it pushes all of this responsibility onto its users, and then + # provides them with an insufficient API for meeting this + # responsibility.) def ssl_verify_peer(cert_string) - cert = nil + # We use ssl_verify_peer simply as a mechanism for gathering the + # certificate chain presented by the peer. In ssl_handshake_completed, + # we’ll make use of this information in order to verify the peer. + @peer_certificate_chain ||= [] begin cert = OpenSSL::X509::Certificate.new(cert_string) + @peer_certificate_chain << cert + true rescue OpenSSL::X509::CertificateError return false end + end - @last_seen_cert = cert + def ssl_handshake_completed + # It’s not great to have to perform the server certificate verification + # after the handshake has completed, because it means: + # + # - We have to be sure that we don’t send any data over the TLS + # connection until we’ve verified the certificate. Created + # https://github.com/ably/ably-ruby/issues/400 to understand whether + # there’s anything we need to change to be sure of this. + # + # - If verification does fail, we have no way of failing the handshake + # with a bad_certificate error. + # + # Unfortunately there doesn’t seem to be a better alternative within + # the TLS-related APIs provided to us by EventMachine. (Admittedly I am + # not familiar with EventMachine.) + # + # (Performing the verification post-handshake is not new to the Ably + # implementation of certificate verification; the previous + # implementation performed hostname verification after the handshake + # was complete.) + + # I was quite worried by the description in the aforementioned issue + # eventmachine/eventmachine#954 of how EventMachine "ignores all errors + # from the chain construction" and hence I don’t know if there is some + # weird scenario where, somehow, the calls to ssl_verify_peer terminate + # with some intermediate certificate instead of with the certificate of + # the server we’re communicating with. (It's quite possible that this + # can’t occur and I’m just being paranoid, but I think a bit of + # paranoia when it comes to security isn't a bad thing.) + # + # That's why, instead of the previous code which passed + # certificate_store.verify the final certificate received by + # ssl_verify_peer, I explicitly use the result of get_peer_cert, to be + # sure that the certificate that we’re verifying is the one that the + # server has demonstrated that they hold the private key for. + server_certificate = OpenSSL::X509::Certificate.new(get_peer_cert) + + # A sense check to confirm my understanding of what’s in @peer_certificate_chain. + # + # (As mentioned above, unless something has gone very wrong, these two + # certificates should be identical.) + unless server_certificate == @peer_certificate_chain.last + raise OpenSSL::SSL::SSLError.new(%(Peer certificate sense check failed for "#{host}")); + end - if certificate_store.verify(@last_seen_cert) - begin - certificate_store.add_cert(@last_seen_cert) - rescue OpenSSL::X509::StoreError => e - raise e unless e.message == 'cert already in hash table' - end - true - else + # Verify the server’s certificate against the default trust anchors, + # aided by the intermediate certificates provided by the server. + unless create_certificate_store.verify(server_certificate, @peer_certificate_chain[0...-1]) raise OpenSSL::SSL::SSLError.new(%(unable to verify the server certificate for "#{host}")) end - end - def ssl_handshake_completed unless verify_peer? warn "[WARNING; ably-em-http-request] TLS hostname validation is disabled (use 'tls: {verify_peer: true}'), see" + " CVE-2020-13482 and https://github.com/igrigorik/em-http-request/issues/339 for details" unless parent.connopts.tls.has_key?(:verify_peer) return true end - unless OpenSSL::SSL.verify_certificate_identity(@last_seen_cert, host) + # Verify that the peer’s certificate matches the hostname. + unless OpenSSL::SSL.verify_certificate_identity(server_certificate, host) raise OpenSSL::SSL::SSLError.new(%(host "#{host}" does not match the server certificate)) else true @@ -84,14 +166,12 @@ def host parent.connopts.host end - def certificate_store - @certificate_store ||= begin - store = OpenSSL::X509::Store.new - store.set_default_paths - ca_file = parent.connopts.tls[:cert_chain_file] - store.add_file(ca_file) if ca_file - store - end + def create_certificate_store + store = OpenSSL::X509::Store.new + store.set_default_paths + ca_file = parent.connopts.tls[:cert_chain_file] + store.add_file(ca_file) if ca_file + store end end