From ab1a7cc4a5dc5435df50cc480d1f0ae8ad06ee65 Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Wed, 15 May 2019 01:30:29 +0100 Subject: [PATCH] TLS: fix DISSECTOR_ASSERT for zero-length records fragments When decrypt_ssl3_record is called with a record length of zero, it will pass NULL to ssl_data_set because tvb_get_ptr(..., 0) yields NULL. That triggers a DISSECTOR_ASSERT. Fix this and add expert info while at it. Bug: 15780 Change-Id: I727b511aa48b6e1aeb20a441d1eb9d3627a74413 Reviewed-on: https://code.wireshark.org/review/33203 Petri-Dish: Peter Wu Tested-by: Petri Dish Buildbot Reviewed-by: Alexis La Goutte --- epan/dissectors/packet-dtls.c | 2 +- epan/dissectors/packet-tls-utils.c | 17 +++++++++++++++++ epan/dissectors/packet-tls-utils.h | 3 ++- epan/dissectors/packet-tls.c | 9 ++++++--- 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/epan/dissectors/packet-dtls.c b/epan/dissectors/packet-dtls.c index e7f6c117a6..c6f5184e7d 100644 --- a/epan/dissectors/packet-dtls.c +++ b/epan/dissectors/packet-dtls.c @@ -775,7 +775,7 @@ dissect_dtls_record(tvbuff_t *tvb, packet_info *pinfo, if (decrypted) { add_new_data_source(pinfo, decrypted, "Decrypted DTLS"); } - ssl_check_record_length(&dissect_dtls_hf, pinfo, record_length, length_pi, session->version, decrypted); + ssl_check_record_length(&dissect_dtls_hf, pinfo, (ContentType)content_type, record_length, length_pi, session->version, decrypted); switch ((ContentType) content_type) { diff --git a/epan/dissectors/packet-tls-utils.c b/epan/dissectors/packet-tls-utils.c index f019130b06..58be84cb59 100644 --- a/epan/dissectors/packet-tls-utils.c +++ b/epan/dissectors/packet-tls-utils.c @@ -7546,6 +7546,7 @@ ssl_try_set_version(SslSession *session, SslDecryptSession *ssl, void ssl_check_record_length(ssl_common_dissect_t *hf, packet_info *pinfo, + ContentType content_type, guint record_length, proto_item *length_pi, guint16 version, tvbuff_t *decrypted_tvb) { @@ -7557,6 +7558,22 @@ ssl_check_record_length(ssl_common_dissect_t *hf, packet_info *pinfo, /* RFC 5246, Section 6.2.3: TLSCiphertext.fragment length MUST NOT exceed 2^14 + 2048 */ max_expansion = 2048; } + /* + * RFC 5246 (TLS 1.2), Section 6.2.1 forbids zero-length Handshake, Alert + * and ChangeCipherSpec. + * RFC 6520 (Heartbeats) does not mention zero-length Heartbeat fragments, + * so assume it is permitted. + * RFC 6347 (DTLS 1.2) does not mention zero-length fragments either, so + * assume TLS 1.2 requirements. + */ + if (record_length == 0 && + (content_type == SSL_ID_CHG_CIPHER_SPEC || + content_type == SSL_ID_ALERT || + content_type == SSL_ID_HANDSHAKE)) { + expert_add_info_format(pinfo, length_pi, &hf->ei.record_length_invalid, + "Zero-length %s fragments are not allowed", + val_to_str_const(content_type, ssl_31_content_type, "unknown")); + } if (record_length > TLS_MAX_RECORD_LENGTH + max_expansion) { expert_add_info_format(pinfo, length_pi, &hf->ei.record_length_invalid, "TLSCiphertext length MUST NOT exceed 2^14 + %u", max_expansion); diff --git a/epan/dissectors/packet-tls-utils.h b/epan/dissectors/packet-tls-utils.h index 346ac68ce7..235a30c6c1 100644 --- a/epan/dissectors/packet-tls-utils.h +++ b/epan/dissectors/packet-tls-utils.h @@ -1022,6 +1022,7 @@ ssl_end_vector(ssl_common_dissect_t *hf, tvbuff_t *tvb, packet_info *pinfo, prot extern void ssl_check_record_length(ssl_common_dissect_t *hf, packet_info *pinfo, + ContentType content_type, guint record_length, proto_item *length_pi, guint16 version, tvbuff_t *decrypted_tvb); @@ -2128,7 +2129,7 @@ ssl_common_dissect_t name = { \ }, \ { & name .ei.record_length_invalid, \ { prefix ".record.length.invalid", PI_PROTOCOL, PI_ERROR, \ - "Record fragment length is too large", EXPFILL } \ + "Record fragment length is too small or too large", EXPFILL } \ } /* }}} */ diff --git a/epan/dissectors/packet-tls.c b/epan/dissectors/packet-tls.c index d7e79fb4cc..16074158fc 100644 --- a/epan/dissectors/packet-tls.c +++ b/epan/dissectors/packet-tls.c @@ -1016,7 +1016,10 @@ tls_save_decrypted_record(packet_info *pinfo, gint record_id, SslDecryptSession } content_type = data[--datalen]; if (datalen == 0) { - /* XXX should we remember that the decrypted contents was zero-length? */ + /* + * XXX zero-length Handshake fragments are forbidden by RFC 8446, + * Section 5.1. Empty Application Data fragments are allowed though. + */ return; } } @@ -1916,7 +1919,7 @@ dissect_ssl3_record(tvbuff_t *tvb, packet_info *pinfo, * handshake records in the same frame). * In TLS 1.3, only "Application Data" records are encrypted. */ - if (ssl && (session->version != TLSV1DOT3_VERSION || content_type == SSL_ID_APP_DATA)) { + if (ssl && record_length && (session->version != TLSV1DOT3_VERSION || content_type == SSL_ID_APP_DATA)) { gboolean decrypt_ok = FALSE; /* Try to decrypt TLS 1.3 early data first */ @@ -1954,7 +1957,7 @@ dissect_ssl3_record(tvbuff_t *tvb, packet_info *pinfo, proto_item_set_generated(ti); } } - ssl_check_record_length(&dissect_ssl3_hf, pinfo, record_length, length_pi, version, decrypted); + ssl_check_record_length(&dissect_ssl3_hf, pinfo, (ContentType)content_type, record_length, length_pi, version, decrypted); switch ((ContentType) content_type) { case SSL_ID_CHG_CIPHER_SPEC: