]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
SslBump: Disable OpenSSL TLSv1.3 support for older TLS traffic (#620)
authorChristos Tsantilas <christos@chtsanti.net>
Wed, 6 May 2020 07:09:50 +0000 (10:09 +0300)
committerGitHub <noreply@github.com>
Wed, 6 May 2020 07:09:50 +0000 (19:09 +1200)
* 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 <rousskov@measurement-factory.com>
src/anyp/ProtocolVersion.h
src/client_side.cc
src/security/Handshake.cc
src/security/Handshake.h
src/security/NegotiationHistory.cc
src/ssl/PeekingPeerConnector.cc
src/ssl/PeekingPeerConnector.h
src/ssl/bio.cc
src/ssl/bio.h
src/tests/stub_libsecurity.cc

index 39a326bb1bf9627243701899f19c70de3b0d534b..e2f5e6398633899e34dd1d4e716f3e411dc3a5b1 100644 (file)
@@ -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;
index 445417a960df3658700de85a251ec2ef31a2b009..58468491f85ed372be3899138720a3ebd0d9564b 100644 (file)
@@ -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
index 045118b7edf966d491b548ae5effc7081e340332..3db9cfec73d13fe82236c4295be6bb13c026099d 100644 (file)
@@ -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
index 2bb5fd750abaef2b9236e3885131b3300ba9d021..3e5d7a09b40a318ae6b405c849d2f1adcc044cfc 100644 (file)
@@ -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
index 076e2232b36f5fa75b4ca18e76303c0ffc347ce5..3a1130faa5649c1e61f972c42ffe38f4f8998e53 100644 (file)
@@ -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];
index d7f764e840110ad42ec8711726296df93bf837b4..a1e9a2fe168057a7bc4d2066a547243a4ba5249d 100644 (file)
@@ -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<Ssl::ServerBio *>(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
index 2af8c2c816731af31c36f49e5307fdfdf207729c..d69ff7826ca711fa004bf70943a8211ebf93c740 100644 (file)
@@ -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
 };
 
index 5f08a030014062ab427ee2ea029aa14a9090f927..15cc9eecd0f7e4e49965c847be9a28e74e7db4af 100644 (file)
@@ -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);
index 22335b8a3bbe4159f624b3374f9512082866c6cf..a50f1401b45e96608bd94d1b49963e06e6e50a1f 100644 (file)
@@ -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
index 8861dd8203b86f0491de39c24c33009cbc8683df..b801e1a79722c57f0a037f3be96f02f2410a2722 100644 (file)
@@ -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"