Fixed parsing of TLS messages that span multiple records (#457)

Squid fed the TLS message parser with one TLS record fragment
at a time but allowed InsufficientInput exceptions to bubble up
beyond the TLS message parsing code. If a server handshake
message spans multiple TLS records, and Squid reads all those
records together with the end of the TLS server handshake, then
the higher-level code interprets InsufficientInput as the need
for more TLS records for the record parser (rather than more
fragments for the TLS message parser). The affected transaction
would then time out or otherwise fail while waiting for those
non-existent TLS records to come from the server.

We now parse TLS messages only after accumulating all same-type
TLS records. For truncated handshakes, this may reduce the
level of information extracted by Squid in some cases, but
this approach keeps the code simple. The handshake is still
available for logging if that partial info is needed for triage.

Test case: 1000-sans.badssl.com which sends a huge server certificate.

This is a Measurement Factory project.

diff --git a/src/security/Handshake.cc b/src/security/Handshake.cc
index f1ba9c9bc252ee17f36036a532ea24315d87692e..ef3e9861512aa1a69e39cd81bdb2715c91a846ff 100644 (file)
--- a/src/security/Handshake.cc
+++ b/src/security/Handshake.cc
@@ -244,22 +244,23 @@ Security::HandshakeParser::parseModernRecord()
     Must(record.fragment.length() || record.type == ContentType::ctApplicationData);
 
     if (currentContentType != record.type) {
+        parseMessages();
         Must(tkMessages.atEnd()); // no currentContentType leftovers
         fragments = record.fragment;
-        tkMessages.reset(fragments, true); // true because more fragments may come
         currentContentType = record.type;
     } else {
         fragments.append(record.fragment);
-        tkMessages.reinput(fragments, true); // true because more fragments may come
-        tkMessages.rollback();
     }
-    parseMessages();
+
+    if (tkRecords.atEnd() && !done)
+        parseMessages();
 }
 
 /// parses one or more "higher-level protocol" frames of currentContentType
 void
 Security::HandshakeParser::parseMessages()
 {
+    tkMessages.reset(fragments, false);
     for (; !tkMessages.atEnd(); tkMessages.commit()) {
         switch (currentContentType) {
         case ContentType::ctChangeCipherSpec: