From: Christos Tsantilas Date: Fri, 24 Apr 2020 22:08:23 +0000 (+0000) Subject: SslBump: Disable OpenSSL TLSv1.3 support for older TLS traffic (#588) X-Git-Tag: 4.15-20210522-snapshot~131 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=cd29a42;p=thirdparty%2Fsquid.git SslBump: Disable OpenSSL TLSv1.3 support for older TLS traffic (#588) This change fixes stalled peeked-at during step2 connections from IE11 and FireFox v56 running on Windows 10 (at least), producing "Handshake with SSL server failed" cache.log errors with this OpenSSL detail: `SSL routines:ssl_choose_client_version:inappropriate fallback` Disabling TLS v1.3 support for older TLS connections is required because, in the affected environments, OpenSSL detects and, for some unknown reason, blocks a "downgrade" when a server claims support for TLS v1.3 but then accepts a TLS v1.2 connection from an older client. This is a Measurement Factory project --- diff --git a/src/anyp/ProtocolVersion.h b/src/anyp/ProtocolVersion.h index 39a326bb1b..e2f5e63986 100644 --- a/src/anyp/ProtocolVersion.h +++ b/src/anyp/ProtocolVersion.h @@ -40,6 +40,9 @@ public: unsigned int major; ///< major version number unsigned int minor; ///< minor version number + /// whether the version is "known" (e.g., has been parsed or explicitly set) + explicit operator bool() const { return protocol != PROTO_NONE; } + bool operator==(const ProtocolVersion& that) const { if (this->protocol != that.protocol) return false; diff --git a/src/client_side.cc b/src/client_side.cc index 94f4e1c3cd..3d566d5652 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -2187,6 +2187,7 @@ ConnStateData::ConnStateData(const MasterXaction::Pointer &xact) : bodyParser(nullptr), #if USE_OPENSSL sslBumpMode(Ssl::bumpEnd), + tlsParser(Security::HandshakeParser::fromClient), #endif needProxyProtocolHeader_(false), #if USE_OPENSSL diff --git a/src/security/Handshake.cc b/src/security/Handshake.cc index 045118b7ed..7b39071ee6 100644 --- a/src/security/Handshake.cc +++ b/src/security/Handshake.cc @@ -106,11 +106,11 @@ static Extensions SupportedExtensions(); } // namespace Security -/// Convenience helper: We parse ProtocolVersion but store "int". +/// parse TLS ProtocolVersion (uint16) and convert it to AnyP::ProtocolVersion static AnyP::ProtocolVersion -ParseProtocolVersion(Parser::BinaryTokenizer &tk) +ParseProtocolVersion(Parser::BinaryTokenizer &tk, const char *contextLabel = ".version") { - Parser::BinaryTokenizerContext context(tk, ".version"); + Parser::BinaryTokenizerContext context(tk, contextLabel); uint8_t vMajor = tk.uint8(".major"); uint8_t vMinor = tk.uint8(".minor"); if (vMajor == 0 && vMinor == 2) @@ -187,10 +187,11 @@ Security::TlsDetails::TlsDetails(): /* Security::HandshakeParser */ -Security::HandshakeParser::HandshakeParser(): +Security::HandshakeParser::HandshakeParser(const MessageSource source): details(new TlsDetails), state(atHelloNone), resumingSession(false), + messageSource(source), currentContentType(0), done(nullptr), expectingModernRecords(false) @@ -285,12 +286,19 @@ Security::HandshakeParser::parseChangeCipherCpecMessage() { Must(currentContentType == ContentType::ctChangeCipherSpec); // We are currently ignoring Change Cipher Spec Protocol messages. - skipMessage("ChangeCipherCpec msg [fragment]"); + skipMessage("ChangeCipherSpec msg [fragment]"); + + // In TLS v1.2 and earlier, ChangeCipherSpec is sent after Hello (when + // tlsSupportedVersion is already known) and indicates session resumption. + // In later TLS versions, ChangeCipherSpec may be sent before and after + // Hello, but it is unused for session resumption and should be ignored. + if (!details->tlsSupportedVersion || Tls1p3orLater(details->tlsSupportedVersion)) + return; - // Everything after the ChangeCipherCpec message may be encrypted. - // Continuing parsing is pointless. Stop here. resumingSession = true; - done = "ChangeCipherCpec"; + + // Everything after the ChangeCipherSpec message may be encrypted. Stop. + done = "ChangeCipherSpec in v1.2-"; } void @@ -316,14 +324,19 @@ Security::HandshakeParser::parseHandshakeMessage() switch (message.msg_type) { case HandshakeType::hskClientHello: Must(state < atHelloReceived); + Must(messageSource == fromClient); Security::HandshakeParser::parseClientHelloHandshakeMessage(message.msg_body); state = atHelloReceived; done = "ClientHello"; return; case HandshakeType::hskServerHello: Must(state < atHelloReceived); + Must(messageSource == fromServer); parseServerHelloHandshakeMessage(message.msg_body); state = atHelloReceived; + // for TLSv1.3 and later, anything after the server Hello is encrypted + if (Tls1p3orLater(details->tlsSupportedVersion)) + done = "ServerHello in v1.3+"; return; case HandshakeType::hskCertificate: Must(state < atCertificatesReceived); @@ -424,6 +437,10 @@ Security::HandshakeParser::parseExtensions(const SBuf &raw) case 35: // SessionTicket TLS Extension; RFC 5077 details->tlsTicketsExtension = true; details->hasTlsTicket = !extension.data.isEmpty(); + break; + case 43: // supported_versions extension; RFC 8446 + parseSupportedVersionsExtension(extension.data); + break; case 13172: // Next Protocol Negotiation Extension (expired draft?) default: break; @@ -504,6 +521,78 @@ Security::HandshakeParser::parseSniExtension(const SBuf &extensionData) const return SBuf(); // SNI extension lacks host_name } +/// RFC 8446 Section 4.2.1: SupportedVersions extension +void +Security::HandshakeParser::parseSupportedVersionsExtension(const SBuf &extensionData) const +{ + // Upon detecting a quoted RFC MUST violation, this parser immediately + // returns, ignoring the entire extension and resulting in Squid relying on + // the legacy_version field value or another (valid) supported_versions + // extension. The alternative would be to reject the whole handshake as + // invalid. Deployment experience will show which alternative is the best. + + // Please note that several of these MUSTs also imply certain likely + // handling of a hypothetical next TLS version (e.g., v1.4). + + // RFC 8446 Section 4.1.2: + // In TLS 1.3, the client indicates its version preferences in the + // "supported_versions" extension (Section 4.2.1) and the legacy_version + // field MUST be set to 0x0303, which is the version number for TLS 1.2. + // + // RFC 8446 Section 4.2.1: + // A server which negotiates TLS 1.3 MUST respond by sending a + // "supported_versions" extension containing the selected version value + // (0x0304). It MUST set the ServerHello.legacy_version field to 0x0303 + // (TLS 1.2). + // + // Ignore supported_versions senders violating legacy_version MUSTs above: + if (details->tlsSupportedVersion != AnyP::ProtocolVersion(AnyP::PROTO_TLS, 1, 2)) + return; + + AnyP::ProtocolVersion supportedVersionMax; + if (messageSource == fromClient) { + Parser::BinaryTokenizer tkList(extensionData); + Parser::BinaryTokenizer tkVersions(tkList.pstring8("SupportedVersions")); + while (!tkVersions.atEnd()) { + const auto version = ParseProtocolVersion(tkVersions, "supported_version"); + if (!supportedVersionMax || TlsVersionEarlierThan(supportedVersionMax, version)) + supportedVersionMax = version; + } + + // ignore empty supported_versions + if (!supportedVersionMax) + return; + + // supportedVersionMax here may be "earlier" than tlsSupportedVersion: A + // TLS v1.3 client may try to negotiate a _legacy_ version X with a TLS + // v1.3 server by sending supported_versions containing just X. + } else { + assert(messageSource == fromServer); + Parser::BinaryTokenizer tkVersion(extensionData, "selected_version"); + const auto version = ParseProtocolVersion(tkVersion); + // RFC 8446 Section 4.2.1: + // A server which negotiates a version of TLS prior to TLS 1.3 [...] + // MUST NOT send the "supported_versions" extension. + if (Tls1p2orEarlier(version)) + return; + supportedVersionMax = version; + } + + // We overwrite Hello-derived legacy_version because the following MUSTs + // indicate that it is ignored in the presence of valid supported_versions + // as far as the negotiated version is concerned. For simplicity sake, we + // may also overwrite previous valid supported_versions extensions (if any). + // + // RFC 8446 Section 4.2.1: + // If this extension is present in the ClientHello, servers MUST NOT use the + // ClientHello.legacy_version value for version negotiation and MUST use + // only the "supported_versions" extension to determine client preferences. + // Servers MUST only select a version of TLS present in that extension + debugs(83, 7, "found " << supportedVersionMax); + assert(supportedVersionMax); + details->tlsSupportedVersion = supportedVersionMax; +} + void Security::HandshakeParser::skipMessage(const char *description) { @@ -647,6 +736,9 @@ Security::SupportedExtensions() #if defined(TLSEXT_TYPE_next_proto_neg) // 13172 extensions.insert(TLSEXT_TYPE_next_proto_neg); #endif +#if defined(TLSEXT_TYPE_supported_versions) // 43 + extensions.insert(TLSEXT_TYPE_supported_versions); +#endif /* * OpenSSL does not support these last extensions by default, but those diff --git a/src/security/Handshake.h b/src/security/Handshake.h index 2bb5fd750a..3e5d7a09b4 100644 --- a/src/security/Handshake.h +++ b/src/security/Handshake.h @@ -29,7 +29,11 @@ public: std::ostream & print(std::ostream &os) const; AnyP::ProtocolVersion tlsVersion; ///< The TLS hello message version - AnyP::ProtocolVersion tlsSupportedVersion; ///< The requested/used TLS version + + /// For most compliant TLS v1.3+ agents, this is supported_versions maximum. + /// For others agents, this is the legacy_version field. + AnyP::ProtocolVersion tlsSupportedVersion; + bool compressionSupported; ///< The requested/used compressed method SBuf serverName; ///< The SNI hostname, if any bool doHeartBeats; @@ -59,7 +63,10 @@ public: /// The parsing states typedef enum {atHelloNone = 0, atHelloStarted, atHelloReceived, atCertificatesReceived, atHelloDoneReceived, atNstReceived, atCcsReceived, atFinishReceived} ParserState; - HandshakeParser(); + /// the originator of the TLS handshake being parsed + typedef enum { fromClient = 0, fromServer } MessageSource; + + explicit HandshakeParser(MessageSource); /// Parses the initial sequence of raw bytes sent by the TLS/SSL agent. /// Returns true upon successful completion (e.g., got HelloDone). @@ -67,7 +74,7 @@ public: /// Throws on errors. bool parseHello(const SBuf &data); - TlsDetails::Pointer details; ///< TLS handshake meta info or nil. + TlsDetails::Pointer details; ///< TLS handshake meta info. Never nil. Security::CertList serverCertificates; ///< parsed certificates chain @@ -75,6 +82,9 @@ public: bool resumingSession; ///< True if this is a resuming session + /// whether we are parsing Server or Client TLS handshake messages + MessageSource messageSource; + private: bool isSslv2Record(const SBuf &raw) const; void parseRecord(); @@ -96,6 +106,7 @@ private: bool parseCompressionMethods(const SBuf &raw); void parseExtensions(const SBuf &raw); SBuf parseSniExtension(const SBuf &extensionData) const; + void parseSupportedVersionsExtension(const SBuf &extensionData) const; void parseCiphers(const SBuf &raw); void parseV23Ciphers(const SBuf &raw); @@ -120,6 +131,40 @@ private: YesNoNone expectingModernRecords; }; +/// whether the given protocol belongs to the TLS/SSL group of protocols +inline bool +TlsFamilyProtocol(const AnyP::ProtocolVersion &version) +{ + return (version.protocol == AnyP::PROTO_TLS || version.protocol == AnyP::PROTO_SSL); +} + +/// whether TLS/SSL protocol `a` precedes TLS/SSL protocol `b` +inline bool +TlsVersionEarlierThan(const AnyP::ProtocolVersion &a, const AnyP::ProtocolVersion &b) +{ + Must(TlsFamilyProtocol(a)); + Must(TlsFamilyProtocol(b)); + + if (a.protocol == b.protocol) + return a < b; + + return a.protocol == AnyP::PROTO_SSL; // implies that b is TLS +} + +/// whether the given TLS/SSL protocol is TLS v1.2 or earlier, including SSL +inline bool +Tls1p2orEarlier(const AnyP::ProtocolVersion &p) +{ + return TlsVersionEarlierThan(p, AnyP::ProtocolVersion(AnyP::PROTO_TLS, 1, 3)); +} + +/// whether the given TLS/SSL protocol is TLS v1.3 or later +inline bool +Tls1p3orLater(const AnyP::ProtocolVersion &p) +{ + return !Tls1p2orEarlier(p); +} + } #endif // SQUID_SECURITY_HANDSHAKE_H diff --git a/src/security/NegotiationHistory.cc b/src/security/NegotiationHistory.cc index 076e2232b3..3a1130faa5 100644 --- a/src/security/NegotiationHistory.cc +++ b/src/security/NegotiationHistory.cc @@ -25,7 +25,7 @@ Security::NegotiationHistory::NegotiationHistory() const char * Security::NegotiationHistory::printTlsVersion(AnyP::ProtocolVersion const &v) const { - if (v.protocol != AnyP::PROTO_SSL && v.protocol != AnyP::PROTO_TLS) + if (!TlsFamilyProtocol(v)) return nullptr; static char buf[512]; diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index d7f764e840..a1e9a2fe16 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -276,18 +276,32 @@ Ssl::PeekingPeerConnector::noteNegotiationError(const int result, const int ssl_ BIO *b = SSL_get_rbio(session.get()); Ssl::ServerBio *srvBio = static_cast(BIO_get_data(b)); - // In Peek mode, the ClientHello message sent to the server. If the - // server resuming a previous (spliced) SSL session with the client, - // then probably we are here because local SSL object does not know - // anything about the session being resumed. - // - if (srvBio->bumpMode() == Ssl::bumpPeek && (resumingSession = srvBio->resumingSession())) { - // we currently splice all resumed sessions unconditionally - // if (const bool spliceResumed = true) { - bypassCertValidator(); - checkForPeekAndSpliceMatched(Ssl::bumpSplice); - return; - // } // else fall through to find a matching ssl_bump action (with limited info) + if (srvBio->bumpMode() == Ssl::bumpPeek) { + auto bypassValidator = false; + if (srvBio->encryptedCertificates()) { + // it is pointless to peek at encrypted certificates + // + // we currently splice all sessions with encrypted certificates + // if (const auto spliceEncryptedCertificates = true) { + bypassValidator = true; + // } // else fall through to find a matching ssl_bump action (with limited info) + } else if (srvBio->resumingSession()) { + // In peek mode, the ClientHello message is forwarded to the server. + // If the server is resuming a previous (spliced) SSL session with + // the client, then probably we are here because our local SSL + // object does not know anything about the session being resumed. + // + // we currently splice all resumed sessions + // if (const auto spliceResumed = true) { + bypassValidator = true; + // } // else fall through to find a matching ssl_bump action (with limited info) + } + + if (bypassValidator) { + bypassCertValidator(); + checkForPeekAndSpliceMatched(Ssl::bumpSplice); + return; + } } // If we are in peek-and-splice mode and still we did not write to diff --git a/src/ssl/PeekingPeerConnector.h b/src/ssl/PeekingPeerConnector.h index 2af8c2c816..d69ff7826c 100644 --- a/src/ssl/PeekingPeerConnector.h +++ b/src/ssl/PeekingPeerConnector.h @@ -30,7 +30,6 @@ public: Security::PeerConnector(aServerConn, aCallback, alp, timeout), clientConn(aClientConn), splice(false), - resumingSession(false), serverCertificateHandled(false) { request = aRequest; @@ -75,7 +74,6 @@ private: Comm::ConnectionPointer clientConn; ///< TCP connection to the client AsyncCall::Pointer closeHandler; ///< we call this when the connection closed bool splice; ///< whether we are going to splice or not - bool resumingSession; ///< whether it is an SSL resuming session connection bool serverCertificateHandled; ///< whether handleServerCertificate() succeeded }; diff --git a/src/ssl/bio.cc b/src/ssl/bio.cc index 5f08a03001..15cc9eecd0 100644 --- a/src/ssl/bio.cc +++ b/src/ssl/bio.cc @@ -250,7 +250,8 @@ Ssl::ServerBio::ServerBio(const int anFd): parsedHandshake(false), parseError(false), bumpMode_(bumpNone), - rbufConsumePos(0) + rbufConsumePos(0), + parser_(Security::HandshakeParser::fromServer) { } @@ -554,6 +555,13 @@ Ssl::ServerBio::resumingSession() return parser_.resumingSession; } +bool +Ssl::ServerBio::encryptedCertificates() const +{ + return parser_.details->tlsSupportedVersion && + Security::Tls1p3orLater(parser_.details->tlsSupportedVersion); +} + /// initializes BIO table after allocation static int squid_bio_create(BIO *bi) @@ -717,6 +725,12 @@ applyTlsDetailsToSSL(SSL *ssl, Security::TlsDetails::Pointer const &details, Ssl SSL_set_options(ssl, SSL_OP_NO_COMPRESSION); #endif +#if defined(SSL_OP_NO_TLSv1_3) + // avoid "inappropriate fallback" OpenSSL error messages + if (details->tlsSupportedVersion && Security::Tls1p2orEarlier(details->tlsSupportedVersion)) + SSL_set_options(ssl, SSL_OP_NO_TLSv1_3); +#endif + #if defined(TLSEXT_STATUSTYPE_ocsp) if (details->tlsStatusRequest) SSL_set_tlsext_status_type(ssl, TLSEXT_STATUSTYPE_ocsp); diff --git a/src/ssl/bio.h b/src/ssl/bio.h index ed930afb42..c27801e752 100644 --- a/src/ssl/bio.h +++ b/src/ssl/bio.h @@ -143,6 +143,10 @@ public: bool resumingSession(); + /// whether the server encrypts its certificate (e.g., TLS v1.3) + /// \retval false the server uses plain certs or its intent is unknown + bool encryptedCertificates() const; + /// The write hold state bool holdWrite() const {return holdWrite_;} /// Enables or disables the write hold state diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index 8861dd8203..b801e1a797 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -28,7 +28,7 @@ Security::EncryptorAnswer::~EncryptorAnswer() {} std::ostream &Security::operator <<(std::ostream &os, const Security::EncryptorAnswer &) STUB_RETVAL(os) #include "security/Handshake.h" -Security::HandshakeParser::HandshakeParser() STUB +Security::HandshakeParser::HandshakeParser(MessageSource) STUB bool Security::HandshakeParser::parseHello(const SBuf &) STUB_RETVAL(false) #include "security/KeyData.h"