Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
350f3a6
Bug 5266: Do not parse the same TLS record fragments twice
rousskov Apr 18, 2023
75f04a1
Bug 5266: Do not parse the same TLS record fragments twice
eduard-bagdasaryan Dec 4, 2023
6ef59b6
Adjusted the solution after testing and removed stale code
eduard-bagdasaryan Dec 6, 2023
f3650cf
Undid an out-of-scope change
eduard-bagdasaryan Dec 6, 2023
095601c
Undone an out-of-scope change
eduard-bagdasaryan Dec 7, 2023
328b704
Simplified, removing try/catch from HandshakeParser::parseMessages()
eduard-bagdasaryan Dec 18, 2023
6932606
Moved tkMessages.commit() into individual parsers
eduard-bagdasaryan Dec 19, 2023
2180c95
Do not skip 'empty' message fragments
eduard-bagdasaryan Dec 21, 2023
32c24e5
Moved tkMessages.commit() back into parseMessages()
eduard-bagdasaryan Dec 21, 2023
c5f70c1
Reworked cycles over tkRecords and tkMessages
eduard-bagdasaryan Dec 26, 2023
9a96bc7
Polished method names and a description.
eduard-bagdasaryan Dec 26, 2023
eba1b8e
Polished with 'auto'.
eduard-bagdasaryan Dec 26, 2023
5d4b9b5
Removed a typo
eduard-bagdasaryan Dec 26, 2023
e304647
A couple of fixes
eduard-bagdasaryan Dec 28, 2023
3a88001
Added a debugs() and simplified parseModernRecord()
eduard-bagdasaryan Dec 29, 2023
71802a6
Call tkRecords.commit() calls from parsers to parseHello()
eduard-bagdasaryan Dec 29, 2023
dbf2481
Refactored HandshakeParser::parseMessages()
eduard-bagdasaryan Jan 4, 2024
6b12750
Simplified, removing ChangeCipherSpec class
eduard-bagdasaryan Jan 5, 2024
47eb627
Do not call tkMessages.atEnd() before rollback()
eduard-bagdasaryan Jan 5, 2024
4b525e4
Do not silently ignore empty messages in parseNonEmptyMessages()
eduard-bagdasaryan Jan 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/parser/BinaryTokenizer.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,24 @@ 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();

/// the number of bytes that were parsed but not committed yet
auto uncommitted() const { assert(parsed_ >= syncPoint_); return parsed_ - syncPoint_; }

/// resume [incremental] parsing from the last commit point
void rollback();

/// no more bytes to parse or skip
bool atEnd() const;

/// whether all available bytes have been parsed and no more bytes are expected
bool exhausted() const { return atEnd() && !expectingMore(); }

/// parse a single-byte unsigned integer
uint8_t uint8(const char *description);

Expand Down Expand Up @@ -110,6 +119,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
bool expectingMore() const { return expectMore_; }
/// allow or prohibit arriving more data bytes in the future
void expectMore(const bool em) { expectMore_ = em; }

const BinaryTokenizerContext *context; ///< debugging: thing being parsed

protected:
Expand Down
94 changes: 52 additions & 42 deletions src/security/Handshake.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -231,7 +232,6 @@ void
Security::HandshakeParser::parseVersion2Record()
{
const Sslv2Record record(tkRecords);
tkRecords.commit();
details->tlsVersion = AnyP::ProtocolVersion(AnyP::PROTO_SSL, 2, 0);
parseVersion2HandshakeMessage(record.fragment);
state = atHelloReceived;
Expand Down Expand Up @@ -264,7 +264,6 @@ void
Security::HandshakeParser::parseModernRecord()
{
const TLSPlaintext record(tkRecords);
tkRecords.commit();

details->tlsVersion = record.version;

Expand All @@ -273,49 +272,64 @@ Security::HandshakeParser::parseModernRecord()
// RFC 5246: MUST NOT send zero-length [non-application] fragments
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 (currentContentType != record.type && tkMessages.uncommitted() > 0) {
// We could not parse these leftovers before. Since every serialized TLS construct has a
// known size (constant or sent-in-advance), parsing these bytes now cannot succeed either.
throw TextException(ToSBuf("truncated TLS message;",
" leftovers size=", tkMessages.uncommitted(),
" type=", currentContentType), Here());
}

if (tkRecords.atEnd() && !done)
rousskov marked this conversation as resolved.
Show resolved Hide resolved
currentContentType = record.type;

tkMessages.expectMore(!tkRecords.exhausted());
tkMessages.append(record.fragment);

try {
parseMessages();
} catch (const Parser::BinaryTokenizer::InsufficientInput &) {
debugs(83, 3, "need more records");
}
}

/// parses one or more "higher-level protocol" frames of currentContentType
/// Incrementally parses all sequential currentContentType TLS fragments.
/// Successfully stops parsing earlier if `done` becomes set.
void
Security::HandshakeParser::parseMessages()
{
tkMessages.reset(fragments, false);
for (; !tkMessages.atEnd(); tkMessages.commit()) {
rousskov marked this conversation as resolved.
Show resolved Hide resolved
switch (currentContentType) {
case ContentType::ctChangeCipherSpec:
parseChangeCipherCpecMessage();
continue;
case ContentType::ctAlert:
parseAlertMessage();
continue;
case ContentType::ctHandshake:
parseHandshakeMessage();
continue;
case ContentType::ctApplicationData:
parseApplicationDataMessage();
continue;
}
skipMessage("unknown ContentType msg [fragment]");
switch (currentContentType) {
case ContentType::ctChangeCipherSpec:
return parseNonEmptyMessages(&HandshakeParser::parseChangeCipherSpecMessage);
case ContentType::ctAlert:
return parseNonEmptyMessages(&HandshakeParser::parseAlertMessage);
case ContentType::ctHandshake:
return parseNonEmptyMessages(&HandshakeParser::parseHandshakeMessage);
case ContentType::ctApplicationData:
return skipPossiblyEmptyMessages("app data [fragment]");
}
return skipPossiblyEmptyMessages("unknown ContentType msg [fragment]");
}

/// Incrementally parses all sequential currentContentType messages using the given TLS message parser.
/// Each message is assumed to be serialized using at least one byte.
/// At least one message is expected per sequence.
/// Successfully stops parsing earlier if `done` becomes set.
void
Security::HandshakeParser::parseChangeCipherCpecMessage()
Security::HandshakeParser::parseNonEmptyMessages(const ParseMethod messageParser)
{
tkMessages.rollback();
do {
(this->*messageParser)();
tkMessages.commit();
} while (!done && !tkMessages.exhausted());
}

void
Security::HandshakeParser::parseChangeCipherSpecMessage()
{
Must(currentContentType == ContentType::ctChangeCipherSpec);
// We are currently ignoring Change Cipher Spec Protocol messages.
skipMessage("ChangeCipherSpec msg [fragment]");
tkMessages.skip(1, "ChangeCipherSpec msg [fragment]");

// In TLS v1.2 and earlier, ChangeCipherSpec is sent after Hello (when
// tlsSupportedVersion is already known) and indicates session resumption.
Expand Down Expand Up @@ -378,13 +392,6 @@ Security::HandshakeParser::parseHandshakeMessage()
static_cast<unsigned int>(message.msg_type) << " handshake message");
}

void
Security::HandshakeParser::parseApplicationDataMessage()
{
Must(currentContentType == ContentType::ctApplicationData);
skipMessage("app data [fragment]");
}

void
Security::HandshakeParser::parseVersion2HandshakeMessage(const SBuf &raw)
{
Expand Down Expand Up @@ -629,12 +636,16 @@ Security::HandshakeParser::parseSupportedVersionsExtension(const SBuf &extension
}

void
Security::HandshakeParser::skipMessage(const char *description)
Security::HandshakeParser::skipPossiblyEmptyMessages(const char *description)
{
// tkMessages/fragments can only contain messages of the same ContentType.
Assure(tkMessages.uncommitted() == 0);
// 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);
tkMessages.commit();
if (tkMessages.expectingMore())
throw Parser::InsufficientInput();
}

bool
Expand All @@ -646,8 +657,7 @@ Security::HandshakeParser::parseHello(const SBuf &data)

// data contains everything read so far, but we may read more later
tkRecords.reinput(data, true);
tkRecords.rollback();
while (!done)
for (tkRecords.rollback(); !done; tkRecords.commit())
parseRecord();
debugs(83, 7, "success; got: " << done);
// we are done; tkRecords may have leftovers we are not interested in
Expand Down
11 changes: 5 additions & 6 deletions src/security/Handshake.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,17 +84,19 @@ class HandshakeParser
MessageSource messageSource;

private:
using ParseMethod = void (HandshakeParser::*)();

bool isSslv2Record(const SBuf &raw) const;
void parseRecord();
void parseModernRecord();
void parseVersion2Record();
void parseMessages();
void parseNonEmptyMessages(ParseMethod);

void parseChangeCipherCpecMessage();
void parseChangeCipherSpecMessage();
void parseAlertMessage();
void parseHandshakeMessage();
void parseApplicationDataMessage();
void skipMessage(const char *msgType);
void skipPossiblyEmptyMessages(const char *msgType);

bool parseRecordVersion2Try();
void parseVersion2HandshakeMessage(const SBuf &raw);
Expand All @@ -115,9 +117,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;

Expand Down
Loading