From e287364cf8403222ec87111f5243b45ea9d2499e Mon Sep 17 00:00:00 2001 From: Christos Tsantilas Date: Tue, 13 Aug 2019 14:44:22 +0000 Subject: [PATCH] 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. --- src/security/Handshake.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/security/Handshake.cc b/src/security/Handshake.cc index f1ba9c9bc2..ef3e986151 100644 --- 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: -- 2.47.2