From 1701a6b14c0fef6a374368a31c8a0d753b574b9c Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Wed, 21 Sep 2022 19:56:45 +0200 Subject: [PATCH] tls: handle incomplete header sooner Make sure to exit the parser early on incomplete header data. Additionally, make sure to not create duplicated tls frames in this case. Add a debug validation check for the header parser parsing too much data, which should never happen. --- src/app-layer-ssl.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/src/app-layer-ssl.c b/src/app-layer-ssl.c index 45dd9c2776..9a1a30c3b1 100644 --- a/src/app-layer-ssl.c +++ b/src/app-layer-ssl.c @@ -2218,6 +2218,7 @@ static struct SSLDecoderResult SSLv3Decode(uint8_t direction, SSLState *ssl_stat { uint32_t parsed = 0; uint32_t record_len; /* slice of input_len for the current record */ + const bool first_call = (ssl_state->curr_connp->bytes_processed == 0); if (ssl_state->curr_connp->bytes_processed < SSLV3_RECORD_HDR_LEN) { const uint16_t prev_version = ssl_state->curr_connp->version; @@ -2229,6 +2230,33 @@ static struct SSLDecoderResult SSLv3Decode(uint8_t direction, SSLState *ssl_stat SSLSetEvent(ssl_state, TLS_DECODER_EVENT_INVALID_TLS_HEADER); return SSL_DECODER_ERROR(-1); } + parsed = retval; + + SCLogDebug("%s input %p record_length %u", (direction == 0) ? "toserver" : "toclient", + input, ssl_state->curr_connp->record_length); + + /* first the hdr frame at our first chance */ + if (first_call) { + AppLayerFrameNewByPointer(ssl_state->f, &stream_slice, input, SSLV3_RECORD_HDR_LEN, + direction, TLS_FRAME_HDR); + } + + /* parser is streaming for the initial header, then switches to incomplete + * API: so if we don't have the hdr yet, return consumed bytes and wait + * until we are called again with new data. */ + if (ssl_state->curr_connp->bytes_processed < SSLV3_RECORD_HDR_LEN) { + SCLogDebug( + "incomplete header, return %u bytes consumed and wait for more data", parsed); + return SSL_DECODER_OK(parsed); + } + + /* pdu frame needs record length, so only create it when hdr fully parsed. */ + AppLayerFrameNewByPointer(ssl_state->f, &stream_slice, input, + ssl_state->curr_connp->record_length + retval, direction, TLS_FRAME_PDU); + record_len = MIN(input_len - parsed, ssl_state->curr_connp->record_length); + SCLogDebug( + "record_len %u (input_len %u, parsed %u, ssl_state->curr_connp->record_length %u)", + record_len, input_len, parsed, ssl_state->curr_connp->record_length); bool unknown_record = false; switch (ssl_state->curr_connp->content_type) { @@ -2243,17 +2271,6 @@ static struct SSLDecoderResult SSLv3Decode(uint8_t direction, SSLState *ssl_stat break; } - SCLogDebug("%s input %p record_length %u", (direction == 0) ? "toserver" : "toclient", - input, ssl_state->curr_connp->record_length); - AppLayerFrameNewByPointer(ssl_state->f, &stream_slice, input, - ssl_state->curr_connp->record_length + retval, direction, TLS_FRAME_PDU); - AppLayerFrameNewByPointer( - ssl_state->f, &stream_slice, input, SSLV3_RECORD_HDR_LEN, direction, TLS_FRAME_HDR); - parsed = retval; - record_len = MIN(input_len - parsed, ssl_state->curr_connp->record_length); - SCLogDebug("record_len %u (input_len %u, parsed %u, ssl_state->curr_connp->record_length %u)", - record_len, input_len, parsed, ssl_state->curr_connp->record_length); - /* unknown record type. For TLS 1.0, 1.1 and 1.2 this is ok. For the rest it is fatal. Based * on Wireshark logic. */ if (prev_version == TLS_VERSION_10 || prev_version == TLS_VERSION_11) { @@ -2294,6 +2311,7 @@ static struct SSLDecoderResult SSLv3Decode(uint8_t direction, SSLState *ssl_stat SSLSetEvent(ssl_state, TLS_DECODER_EVENT_INVALID_RECORD_LENGTH); return SSL_DECODER_ERROR(-1); } + DEBUG_VALIDATE_BUG_ON(ssl_state->curr_connp->bytes_processed > SSLV3_RECORD_HDR_LEN); } else { ValidateRecordState(ssl_state->curr_connp); -- 2.47.2