diff options
author | Peter Wu <peter@lekensteyn.nl> | 2017-02-14 11:18:24 +0100 |
---|---|---|
committer | Peter Wu <peter@lekensteyn.nl> | 2017-02-15 12:19:40 +0000 |
commit | bb1450b017226b0da892c7c3ebba0fe1621e38d9 (patch) | |
tree | 2b87694692e7ae756873aad0dbe5a447b4b9f9f5 /epan | |
parent | efcb5c07f04210ee89e57347c867a64d3486ebc4 (diff) | |
download | wireshark-bb1450b017226b0da892c7c3ebba0fe1621e38d9.tar.gz |
ssl,dtls: fix wrong expert info for overly large records
The plaintext length is limited to 2^14, but the actual record length
(TLSCiphertext) may be larger due to expansion from compression and the
cipher (like AEAD auth tags). The wrong check led to false expert infos.
Change-Id: I3a56f1b0af05ecc1d97c4f1f0bcf35ff4d0fad42
Fixes: v2.3.0rc0-1584-gff0371e898 ("ssl,dtls: add expert info for overly large record lengths")
Reviewed-on: https://code.wireshark.org/review/20099
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Jaap Keuter <jaap.keuter@xs4all.nl>
Reviewed-by: Peter Wu <peter@lekensteyn.nl>
Diffstat (limited to 'epan')
-rw-r--r-- | epan/dissectors/packet-dtls.c | 8 | ||||
-rw-r--r-- | epan/dissectors/packet-ssl-utils.c | 23 | ||||
-rw-r--r-- | epan/dissectors/packet-ssl-utils.h | 7 | ||||
-rw-r--r-- | epan/dissectors/packet-ssl.c | 8 |
4 files changed, 35 insertions, 11 deletions
diff --git a/epan/dissectors/packet-dtls.c b/epan/dissectors/packet-dtls.c index ce56201c6f..d92f47fd9c 100644 --- a/epan/dissectors/packet-dtls.c +++ b/epan/dissectors/packet-dtls.c @@ -693,7 +693,7 @@ dissect_dtls_record(tvbuff_t *tvb, packet_info *pinfo, guint8 next_byte; proto_tree *ti; proto_tree *dtls_record_tree; - proto_item *pi; + proto_item *length_pi; tvbuff_t *decrypted; SslRecordInfo *record = NULL; heur_dtbl_entry_t *hdtbl_entry; @@ -761,11 +761,8 @@ dissect_dtls_record(tvbuff_t *tvb, packet_info *pinfo, offset += 6; /* add the length */ - pi = proto_tree_add_uint(dtls_record_tree, hf_dtls_record_length, tvb, + length_pi = proto_tree_add_uint(dtls_record_tree, hf_dtls_record_length, tvb, offset, 2, record_length); - if (record_length > TLS_MAX_RECORD_LENGTH) { - expert_add_info(pinfo, pi, &dissect_dtls_hf.ei.record_length_invalid); - } offset += 2; /* move past length field itself */ /* @@ -794,6 +791,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); switch ((ContentType) content_type) { diff --git a/epan/dissectors/packet-ssl-utils.c b/epan/dissectors/packet-ssl-utils.c index 28b49aaa61..23a348af0b 100644 --- a/epan/dissectors/packet-ssl-utils.c +++ b/epan/dissectors/packet-ssl-utils.c @@ -6669,6 +6669,29 @@ ssl_try_set_version(SslSession *session, SslDecryptSession *ssl, } } +void +ssl_check_record_length(ssl_common_dissect_t *hf, packet_info *pinfo, + guint record_length, proto_item *length_pi, + guint16 version, tvbuff_t *decrypted_tvb) +{ + guint max_expansion; + if (version == TLSV1DOT3_VERSION) { + /* TLS 1.3: Max length is 2^14 + 256 */ + max_expansion = 256; + } else { + /* RFC 5246, Section 6.2.3: TLSCiphertext.fragment length MUST NOT exceed 2^14 + 2048 */ + max_expansion = 2048; + } + 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); + } + if (decrypted_tvb && tvb_captured_length(decrypted_tvb) > TLS_MAX_RECORD_LENGTH) { + expert_add_info_format(pinfo, length_pi, &hf->ei.record_length_invalid, + "TLSPlaintext length MUST NOT exceed 2^14"); + } +} + static void ssl_set_cipher(SslDecryptSession *ssl, guint16 cipher) { diff --git a/epan/dissectors/packet-ssl-utils.h b/epan/dissectors/packet-ssl-utils.h index 9462f93f87..5eb66ba084 100644 --- a/epan/dissectors/packet-ssl-utils.h +++ b/epan/dissectors/packet-ssl-utils.h @@ -870,6 +870,11 @@ 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, + guint record_length, proto_item *length_pi, + guint16 version, tvbuff_t *decrypted_tvb); + void ssl_dissect_change_cipher_spec(ssl_common_dissect_t *hf, tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, @@ -1641,7 +1646,7 @@ ssl_common_dissect_t name = { \ }, \ { & name .ei.record_length_invalid, \ { prefix ".record.length.invalid", PI_PROTOCOL, PI_ERROR, \ - "Record fragment length must not exceed 2^14", EXPFILL } \ + "Record fragment length is too large", EXPFILL } \ } /* }}} */ diff --git a/epan/dissectors/packet-ssl.c b/epan/dissectors/packet-ssl.c index 679462908e..ccb821728d 100644 --- a/epan/dissectors/packet-ssl.c +++ b/epan/dissectors/packet-ssl.c @@ -1546,7 +1546,7 @@ dissect_ssl3_record(tvbuff_t *tvb, packet_info *pinfo, guint8 next_byte; proto_tree *ti; proto_tree *ssl_record_tree; - proto_item *pi, *ct_pi; + proto_item *length_pi, *ct_pi; guint content_type_offset; guint32 available_bytes; tvbuff_t *decrypted; @@ -1679,11 +1679,8 @@ dissect_ssl3_record(tvbuff_t *tvb, packet_info *pinfo, offset += 2; /* add the length */ - pi = proto_tree_add_uint(ssl_record_tree, hf_ssl_record_length, tvb, + length_pi = proto_tree_add_uint(ssl_record_tree, hf_ssl_record_length, tvb, offset, 2, record_length); - if (record_length > TLS_MAX_RECORD_LENGTH) { - expert_add_info(pinfo, pi, &dissect_ssl3_hf.ei.record_length_invalid); - } offset += 2; /* move past length field itself */ /* @@ -1739,6 +1736,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); switch ((ContentType) content_type) { case SSL_ID_CHG_CIPHER_SPEC: |