From: Christos Tsantilas Date: Wed, 6 May 2020 07:09:50 +0000 (+0300) Subject: SslBump: Disable OpenSSL TLSv1.3 support for older TLS traffic (#620) X-Git-Tag: SQUID_5_0_3~19 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4d714a37630b0ce8164188d5e25733d3359a0132;p=thirdparty%2Fsquid.git SslBump: Disable OpenSSL TLSv1.3 support for older TLS traffic (#620) * 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 * Fixed TLS selected_version parsing and debugging The description of the expected input was given to the wrong parsing function. This typo may have affected parsing because it told the TLS version tokenizer that more data may be expected for the already fully extracted extension. I believe that the lie could affect error diagnostic when parsing malformed input, but had no effect on handling well-formed TLS handshakes (other than less-specific debugging). Detected by Coverity. CID 1462621: Incorrect expression (NO_EFFECT) Broken by master commit cd29a42. Co-authored-by: Alex Rousskov --- 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 445417a960..58468491f8 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..3db9cfec73 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); + const auto version = ParseProtocolVersion(tkVersion, "selected_version"); + // 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 22335b8a3b..a50f1401b4 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"