Bug 5266: Do not parse the same TLS record fragments twice

src/parser/BinaryTokenizer.h

@@ -60,6 +60,9 @@ class BinaryTokenizer
     /// this method avoids append overheads during incremental parsing
     void reinput(const SBuf &data, const bool expectMore) { data_ = data; expectMore_ = expectMore; }
 
+    /// adds more data bytes to parse
+    void append(const SBuf &data) { data_.append(data); }
+
     /// make progress: future parsing failures will not rollback beyond this point
     void commit();
 
@@ -110,6 +113,11 @@ class BinaryTokenizer
     /// debugging helper for parsed multi-field structures
     void got(uint64_t size, const char *description) const;
 
+    /// whether more data bytes may arrive in the future
+    auto expectingMore() const { return expectMore_; }
+    /// allow or prohibit arriving more data bytes in the future
+    void expectingMore(bool val) { expectMore_ = val; }
+
     const BinaryTokenizerContext *context; ///< debugging: thing being parsed
 
 protected:

src/security/Handshake.cc

@@ -10,6 +10,7 @@
 
 #include "squid.h"
 #include "base/IoManip.h"
+#include "parser/forward.h"
 #include "sbuf/Stream.h"
 #include "security/Handshake.h"
 #if USE_OPENSSL
@@ -274,24 +275,26 @@ 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;
         currentContentType = record.type;
-    } else {
-        fragments.append(record.fragment);
     }
 
-    if (tkRecords.atEnd() && !done)
-        parseMessages();
+    const auto haveUnparsedRecordBytes = !tkRecords.atEnd();
+    const auto expectMoreRecordLayerBytes = tkRecords.expectingMore();
+    // TODO: consider adding BinaryTokenizer::exhausted() instead
+    const auto expectMoreMessageLayerBytes = haveUnparsedRecordBytes || expectMoreRecordLayerBytes;
+
+    tkMessages.expectingMore(expectMoreMessageLayerBytes);
+    tkMessages.append(record.fragment);
+
+    parseMessages();
 }
 
 /// parses one or more "higher-level protocol" frames of currentContentType
 void
 Security::HandshakeParser::parseMessages()
 {
-    tkMessages.reset(fragments, false);
-    for (; !tkMessages.atEnd(); tkMessages.commit()) {
+    for (tkMessages.rollback(); !done; tkMessages.commit()) {
         switch (currentContentType) {
         case ContentType::ctChangeCipherSpec:
             parseChangeCipherCpecMessage();
@@ -631,33 +634,37 @@ Security::HandshakeParser::parseSupportedVersionsExtension(const SBuf &extension
 void
 Security::HandshakeParser::skipMessage(const char *description)
 {
-    // tkMessages/fragments can only contain messages of the same ContentType.
+    // skip at least one byte
+    if (tkMessages.atEnd())
+        throw Parser::InsufficientInput();
+    // tkMessages can only contain messages of the same ContentType.
     // To skip a message, we can and should skip everything we have [left]. If
-    // we have partial messages, debugging will mislead about their boundaries.
+    // we buffered a partial message, we will need to read/skip multiple times.
     tkMessages.skip(tkMessages.leftovers().length(), description);
 }
 
 bool
 Security::HandshakeParser::parseHello(const SBuf &data)
 {
-    try {
-        if (!expectingModernRecords.configured())
-            expectingModernRecords.configure(!isSslv2Record(data));
+    if (!expectingModernRecords.configured())
+        expectingModernRecords.configure(!isSslv2Record(data));
+
+    // data contains everything read so far, but we may read more later
+    tkRecords.reinput(data, true);
+    tkRecords.rollback();
+    while (!done) {
+        try {
+            if (tkRecords.atEnd())
+                return false;
 
-        // data contains everything read so far, but we may read more later
-        tkRecords.reinput(data, true);
-        tkRecords.rollback();
-        while (!done)
             parseRecord();
-        debugs(83, 7, "success; got: " << done);
-        // we are done; tkRecords may have leftovers we are not interested in
-        return true;
-    }
-    catch (const Parser::BinaryTokenizer::InsufficientInput &) {
-        debugs(83, 5, "need more data");
-        return false;
+        }
+        catch (const Parser::BinaryTokenizer::InsufficientInput &) {
+            debugs(83, 5, "need more data");
+        }
     }
-    return false; // unreached
+    // we are done; tkRecords may have leftovers we are not interested in
+    return true;
 }
 
 /// A helper function to create a set of all supported TLS extensions

src/security/Handshake.h

@@ -115,9 +115,6 @@ class HandshakeParser
 
     const char *done; ///< not nil if we got what we were looking for
 
-    /// concatenated TLSPlaintext.fragments of TLSPlaintext.type
-    SBuf fragments;
-
     /// TLS record layer (parsing uninterpreted data)
     Parser::BinaryTokenizer tkRecords;