]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Bug 4309: Fix the presence of extensions detection in SSL Hello messages
authorChristos Tsantilas <chtsanti@users.sourceforge.net>
Thu, 17 Sep 2015 05:58:53 +0000 (22:58 -0700)
committerAmos Jeffries <squid3@treenet.co.nz>
Thu, 17 Sep 2015 05:58:53 +0000 (22:58 -0700)
RFC5246 section 7.4.1.3 (Server Hello) says:

   The presence of extensions can be detected by determining whether
   there are bytes following the compression_method field at the end
   of the ServerHello.

Current parsing Hello code checks whether there are bytes in the whole
SSL message. It does not account for the fact that the message may
contain more than just ServerHello.

This patch fixes this issue and tries to improve the related code to
avoid related problems in the future.

This is a Measurement Factory project

src/ssl/bio.cc

index ac3d11059fa031518562bc43d92c2868bc953f3a..2c951773274c5d3581c787753aa87b19e0f770c9 100644 (file)
@@ -873,15 +873,17 @@ Ssl::Bio::sslFeatures::get(const MemBuf &buf, bool record)
 }
 
 bool
-Ssl::Bio::sslFeatures::parseV3ServerHello(const unsigned char *hello, size_t size)
+Ssl::Bio::sslFeatures::parseV3ServerHello(const unsigned char *messageContainer, size_t messageContainerSize)
 {
     // Parse a ServerHello Handshake message
     // RFC5246 section 7.4, 7.4.1.3
-    // The ServerHello starts at hello+5
-    const size_t helloSize = (hello[6] << 16) | (hello[7] << 8) | hello[8];
+    // The ServerHello starts at messageContainer + 5
+    const unsigned char *serverHello = messageContainer + 5;
+
+    // The Length field (bytes 1-3) plus 4 bytes of the serverHello message header (1 handshake type + 3 hello length)
+    const size_t helloSize = ((serverHello[1] << 16) | (serverHello[2] << 8) | serverHello[3]) + 4;
     debugs(83, 7, "ServerHello message size: " << helloSize);
-    // helloSize should be msgSize + hello Header (4 bytes)
-    if (helloSize + 4 > size) {
+    if (helloSize > messageContainerSize) {
         debugs(83, 2, "ServerHello parse error");
         return false;
     }
@@ -895,32 +897,32 @@ Ssl::Bio::sslFeatures::parseV3ServerHello(const unsigned char *hello, size_t siz
 
     debugs(83, 7, "Get fake features from v3 ServerHello message.");
     // Get the correct version of the sub-hello message
-    sslVersion = (hello[9] << 8) | hello[10];
-    // At the position 43 (MsgHeader(5 bytes) + HelloHeader (6bytes) + SSL3_RANDOM_SIZE (32bytes))
-    const size_t sessIdLen = (size_t)hello[43];
+    sslVersion = (serverHello[4] << 8) | serverHello[5];
+    // At the position 38 (HelloHeader (6bytes) + SSL3_RANDOM_SIZE (32bytes))
+    const size_t sessIdLen = static_cast<size_t>(serverHello[38]);
     debugs(83, 7, "Session ID Length: " <<  sessIdLen);
 
     // The size should be enough to hold at least the following
-    // 5 MsgHelloHeader + 4 (hello header)
+    // 4 (hello header)
     // + 2 (SSL Version) + 32 (random) + 1 (sessionId length)
     // + sessIdLength + 2 (cipher suite) + 1 (compression method)
-    // = 47 + sessIdLength
-    if (47 + sessIdLen > size) {
+    // = 42 + sessIdLength
+    if (42 + sessIdLen > helloSize) {
         debugs(83, 2, "ciphers length parse error");
         return false;
     }
 
-    // The sessionID stored at 44 position, after sessionID length field
-    sessionId.assign((const char *)(hello + 44), sessIdLen);
+    // The sessionID stored at 39 position, after sessionID length field
+    sessionId.assign(reinterpret_cast<const char *>(serverHello + 39), sessIdLen);
 
     // Check if there are extensions in hello message
     // RFC5246 section 7.4.1.4
-    if (size > 47 + sessIdLen + 2) {
-        // 47 + sessIdLen
-        const unsigned char *pToExtensions = hello + 47 + sessIdLen;
+    if (helloSize > 42 + sessIdLen + 2) {
+        // 42 + sessIdLen
+        const unsigned char *pToExtensions = serverHello + 42 + sessIdLen;
         const size_t extensionsLen = (pToExtensions[0] << 8) | pToExtensions[1];
         // Check if the hello size can hold extensions
-        if (47 + 2 + sessIdLen + extensionsLen > size ) {
+        if (42 + 2 + sessIdLen + extensionsLen > helloSize ) {
             debugs(83, 2, "Extensions length parse error");
             return false;
         }
@@ -944,17 +946,18 @@ Ssl::Bio::sslFeatures::parseV3ServerHello(const unsigned char *hello, size_t siz
 }
 
 bool
-Ssl::Bio::sslFeatures::parseV3Hello(const unsigned char *hello, size_t size)
+Ssl::Bio::sslFeatures::parseV3Hello(const unsigned char *messageContainer, size_t messageContainerSize)
 {
     // Parse a ClientHello Handshake message
     // RFC5246 section 7.4, 7.4.1.2
-    // The ClientHello starts at hello+5
+    // The ClientHello starts at messageContainer + 5
+    const unsigned char * clientHello = messageContainer + 5;
 
     debugs(83, 7, "Get fake features from v3 ClientHello message.");
-    const size_t helloSize = (hello[6] << 16) | (hello[7] << 8) | hello[8];
+    // The Length field (bytes 1-3) plus 4 bytes of the clientHello message header (1 handshake type + 3 hello length)
+    const size_t helloSize = ((clientHello[1] << 16) | (clientHello[2] << 8) | clientHello[3]) + 4;
     debugs(83, 7, "ClientHello message size: " << helloSize);
-    // helloSize should be size + hello Header (4 bytes)
-    if (helloSize + 4 > size) {
+    if (helloSize > messageContainerSize) {
         debugs(83, 2, "ClientHello parse error");
         return false;
     }
@@ -967,119 +970,124 @@ Ssl::Bio::sslFeatures::parseV3Hello(const unsigned char *hello, size_t size)
     }
 
     //For SSLv3 or TLSv1.* protocols we can get some more informations
-    if (hello[1] == 0x3 && hello[5] == 0x1 /*HELLO A message*/) {
-        // Get the correct version of the sub-hello message
-        sslVersion = (hello[9] << 8) | hello[10];
-        //Get Client Random number. It starts on the position 11 of hello message
-        memcpy(client_random, hello + 11, SSL3_RANDOM_SIZE);
-        debugs(83, 7, "Client random: " <<  objToString(client_random, SSL3_RANDOM_SIZE));
-
-        // At the position 43 (11+SSL3_RANDOM_SIZE)
-        const size_t sessIDLen = (size_t)hello[43];
-        debugs(83, 7, "Session ID Length: " <<  sessIDLen);
-
-        // The size should be enough to hold at least the following
-        // 5 MsgHelloHeader + 4 (hello header)
-        // + 2 (SSL Version) + 32 (random) + 1 (sessionId length)
-        // + sessIdLength + 2 (cipher suite length) + 1 (compression method length)
-        // = 47 + sessIdLength
-        if (47 + sessIDLen > size)
-            return false;
+    if (messageContainer[1] != 0x3 || clientHello[0] != 0x1 /*HELLO A message*/) {
+        debugs(83, 2, "Not an SSLv3/TLSv1.x client hello message, stop parsing here");
+        return true;
+    }
 
-        // The sessionID stored art 44 position, after sessionID length field
-        sessionId.assign((const char *)(hello + 44), sessIDLen);
+    // Get the correct version of the sub-hello message
+    sslVersion = (clientHello[4] << 8) | clientHello[5];
+    //Get Client Random number. It starts on the position 6 of clientHello message
+    memcpy(client_random, clientHello + 6, SSL3_RANDOM_SIZE);
+    debugs(83, 7, "Client random: " <<  objToString(client_random, SSL3_RANDOM_SIZE));
 
-        //Ciphers list. It is stored after the Session ID.
-        // It is a variable-length vector(RFC5246 section 4.3)
-        const unsigned char *ciphers = hello + 44 + sessIDLen;
-        const size_t ciphersLen = (ciphers[0] << 8) | ciphers[1];
-        if (47 + sessIDLen + ciphersLen > size) {
-            debugs(83, 2, "ciphers length parse error");
-            return false;
+    // At the position 38 (6+SSL3_RANDOM_SIZE)
+    const size_t sessIDLen = static_cast<size_t>(clientHello[38]);
+    debugs(83, 7, "Session ID Length: " <<  sessIDLen);
+
+    // The helloSize should be enough to hold at least the following
+    // 1 handshake type + 3 hello Length
+    // + 2 (SSL Version) + 32 (random) + 1 (sessionId length)
+    // + sessIdLength + 2 (cipher suite length) + 1 (compression method length)
+    // = 42 + sessIdLength
+    if (42 + sessIDLen > helloSize) {
+        debugs(83, 2, "Session ID length parse error");
+        return false;
+    }
+
+    // The sessionID stored art 39 position, after sessionID length field
+    sessionId.assign(reinterpret_cast<const char *>(clientHello + 39), sessIDLen);
+
+    //Ciphers list. It is stored after the Session ID.
+    // It is a variable-length vector(RFC5246 section 4.3)
+    const unsigned char *ciphers = clientHello + 39 + sessIDLen;
+    const size_t ciphersLen = (ciphers[0] << 8) | ciphers[1];
+    if (42 + sessIDLen + ciphersLen > helloSize) {
+        debugs(83, 2, "ciphers length parse error");
+        return false;
+    }
+
+    ciphers += 2;
+    if (ciphersLen) {
+        const SSL_METHOD *method = SSLv3_method();
+        const int cs = method->put_cipher_by_char(NULL, NULL);
+        assert(cs > 0);
+        for (size_t i = 0; i < ciphersLen; i += cs) {
+            const SSL_CIPHER *c = method->get_cipher_by_char((ciphers + i));
+            if (c != NULL) {
+                if (!clientRequestedCiphers.empty())
+                    clientRequestedCiphers.append(":");
+                clientRequestedCiphers.append(c->name);
+            } else
+                unknownCiphers = true;
         }
+    }
+    debugs(83, 7, "Ciphers requested by client: " << clientRequestedCiphers);
 
-        ciphers += 2;
-        if (ciphersLen) {
-            const SSL_METHOD *method = SSLv3_method();
-            const int cs = method->put_cipher_by_char(NULL, NULL);
-            assert(cs > 0);
-            for (size_t i = 0; i < ciphersLen; i += cs) {
-                const SSL_CIPHER *c = method->get_cipher_by_char((ciphers + i));
-                if (c != NULL) {
-                    if (!clientRequestedCiphers.empty())
-                        clientRequestedCiphers.append(":");
-                    clientRequestedCiphers.append(c->name);
-                } else
-                    unknownCiphers = true;
-            }
+    // Compression field: 1 bytes the number of compression methods and
+    // 1 byte for each compression method
+    const unsigned char *compression = ciphers + ciphersLen;
+    if (compression[0] > 1)
+        compressMethod = 1;
+    else
+        compressMethod = 0;
+    debugs(83, 7, "SSL compression methods number: " << static_cast<int>(compression[0]));
+
+    // Parse Extensions, RFC5246 section 7.4.1.4
+    const unsigned char *pToExtensions = compression + 1 + static_cast<int>(compression[0]);
+    if ((size_t)((pToExtensions - clientHello) + 2) < helloSize) {
+        const size_t extensionsLen = (pToExtensions[0] << 8) | pToExtensions[1];
+        if ((pToExtensions - clientHello) + 2 + extensionsLen > helloSize) {
+            debugs(83, 2, "Extensions length parse error");
+            return false;
         }
-        debugs(83, 7, "Ciphers requested by client: " << clientRequestedCiphers);
 
-        // Compression field: 1 bytes the number of compression methods and
-        // 1 byte for each compression method
-        const unsigned char *compression = ciphers + ciphersLen;
-        if (compression[0] > 1)
-            compressMethod = 1;
-        else
-            compressMethod = 0;
-        debugs(83, 7, "SSL compression methods number: " << (int)compression[0]);
-
-        // Parse Extensions, RFC5246 section 7.4.1.4
-        const unsigned char *pToExtensions = compression + 1 + (int)compression[0];
-        if ((size_t)((pToExtensions - hello) + 2) < size) {
-            const size_t extensionsLen = (pToExtensions[0] << 8) | pToExtensions[1];
-            if ((pToExtensions - hello) + 2 + extensionsLen > size) {
-                debugs(83, 2, "Extensions length parse error");
+        pToExtensions += 2;
+        const unsigned char *ext = pToExtensions;
+        while (ext + 4 <= pToExtensions + extensionsLen) {
+            const size_t extType = (ext[0] << 8) | ext[1];
+            ext += 2;
+            const size_t extLen = (ext[0] << 8) | ext[1];
+            ext += 2;
+            debugs(83, 7, "TLS Extension: " << std::hex << extType << " of size:" << extLen);
+
+            if (ext + extLen > pToExtensions + extensionsLen) {
+                debugs(83, 2, "Extension " << std::hex << extType << " length parser error");
                 return false;
             }
 
-            pToExtensions += 2;
-            const unsigned char *ext = pToExtensions;
-            while (ext + 4 <= pToExtensions + extensionsLen) {
-                const size_t extType = (ext[0] << 8) | ext[1];
-                ext += 2;
-                const size_t extLen = (ext[0] << 8) | ext[1];
-                ext += 2;
-                debugs(83, 7, "TLS Extension: " << std::hex << extType << " of size:" << extLen);
-
-                if (ext + extLen > pToExtensions + extensionsLen) {
-                    debugs(83, 2, "Extension " << std::hex << extType << " length parser error");
-                    return false;
-                }
+            //The SNI extension has the type 0 (extType == 0)
+            // RFC6066 sections 3, 10.2
+            // The two first bytes indicates the length of the SNI data (should be extLen-2)
+            // The next byte is the hostname type, it should be '0' for normal hostname (ext[2] == 0)
+            // The 3rd and 4th bytes are the length of the hostname
+            if (extType == 0 && ext[2] == 0) {
+                const size_t hostLen = (ext[3] << 8) | ext[4];
+                if (hostLen < extLen)
+                    serverName.assign(reinterpret_cast<const char *>(ext+5), hostLen);
+                debugs(83, 7, "Found server name: " << serverName);
+            } else if (extType == 15 && ext[0] != 0) {
+                // The heartBeats are the type 15, RFC6520
+                doHeartBeats = true;
+            } else if (extType == 0x23) {
+                //SessionTicket TLS Extension RFC5077
+                tlsTicketsExtension = true;
+                if (extLen != 0)
+                    hasTlsTicket = true;
+            } else if (extType == 0x05) {
+                // RFC6066 sections 8, 10.2
+                tlsStatusRequest = true;
+            } else if (extType == 0x3374) {
+                // detected TLS next protocol negotiate extension
+            } else if (extType == 0x10) {
+                // Application-Layer Protocol Negotiation Extension, RFC7301
+                const size_t listLen = (ext[0] << 8) | ext[1];
+                if (listLen < extLen)
+                    tlsAppLayerProtoNeg.assign(reinterpret_cast<const char *>(ext+5), listLen);
+            } else
+                extensions.push_back(extType);
 
-                //The SNI extension has the type 0 (extType == 0)
-                // RFC6066 sections 3, 10.2
-                // The two first bytes indicates the length of the SNI data (should be extLen-2)
-                // The next byte is the hostname type, it should be '0' for normal hostname (ext[2] == 0)
-                // The 3rd and 4th bytes are the length of the hostname
-                if (extType == 0 && ext[2] == 0) {
-                    const size_t hostLen = (ext[3] << 8) | ext[4];
-                    if (hostLen < extLen)
-                        serverName.assign((const char *)(ext+5), hostLen);
-                    debugs(83, 7, "Found server name: " << serverName);
-                } else if (extType == 15 && ext[0] != 0) {
-                    // The heartBeats are the type 15, RFC6520
-                    doHeartBeats = true;
-                } else if (extType == 0x23) {
-                    //SessionTicket TLS Extension RFC5077
-                    tlsTicketsExtension = true;
-                    if (extLen != 0)
-                        hasTlsTicket = true;
-                } else if (extType == 0x05) {
-                    // RFC6066 sections 8, 10.2
-                    tlsStatusRequest = true;
-                } else if (extType == 0x3374) {
-                    // detected TLS next protocol negotiate extension
-                } else if (extType == 0x10) {
-                    // Application-Layer Protocol Negotiation Extension, RFC7301
-                    const size_t listLen = (ext[0] << 8) | ext[1];
-                    if (listLen < extLen)
-                        tlsAppLayerProtoNeg.assign((const char *)(ext+5), listLen);
-                } else
-                    extensions.push_back(extType);
-
-                ext += extLen;
-            }
+            ext += extLen;
         }
     }
     return true;