From 1d623fd54116fbe234300712a9cbf04560802e9f Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 6 Apr 2021 20:52:17 +0200 Subject: [PATCH] packet-ldap: fix regression for SASL handling commit 19b3376a24192319a008399e8c44f1655f6dcebd ("LDAP bogus malformed errors: decoding encrypted data") introduced 2 problems: - guint decr_len = tvb_reported_length(decr_tvb); was always called with decr_tvb==NULL - dissect_ldap_payload() was not called if sasl_tree is NULL, it needs to be called even if the tree pointer are NULL in order to have the COL_INFO setup correctly. I guess this should also be backported to stable branches (together with 2e6d3b571b50f6a0df31787cb95ba0f66c4fa85f "LDAP: SASL Buffer doesn't include Length field") https://gitlab.com/wireshark/wireshark/-/issues/17347 Signed-off-by: Stefan Metzmacher --- epan/dissectors/asn1/ldap/packet-ldap-template.c | 29 ++++++++++---------- epan/dissectors/packet-ldap.c | 35 ++++++++++++------------ 2 files changed, 31 insertions(+), 33 deletions(-) diff --git a/epan/dissectors/asn1/ldap/packet-ldap-template.c b/epan/dissectors/asn1/ldap/packet-ldap-template.c index 8e370cc08b..30739bebfe 100644 --- a/epan/dissectors/asn1/ldap/packet-ldap-template.c +++ b/epan/dissectors/asn1/ldap/packet-ldap-template.c @@ -1194,7 +1194,7 @@ static void ((strcmp(ldap_info->auth_mech, "GSS-SPNEGO") == 0) || /* auth_mech may have been set from the bind */ (strcmp(ldap_info->auth_mech, "GSSAPI") == 0))) { - tvbuff_t *gssapi_tvb, *plain_tvb = NULL, *decr_tvb= NULL; + tvbuff_t *gssapi_tvb = NULL; int ver_len; int tmp_length; gssapi_encrypt_info_t gssapi_encrypt; @@ -1224,6 +1224,9 @@ static void return; } if (gssapi_encrypt.gssapi_decrypted_tvb) { + tvbuff_t *decr_tvb = gssapi_encrypt.gssapi_decrypted_tvb; + proto_tree *enc_tree = NULL; + /* * The LDAP payload (blob) was encrypted and we were able to decrypt it. * The data was signed via a MIC token, sealed (encrypted), and "wrapped" @@ -1231,20 +1234,17 @@ static void * one or more LDAPMessages such as searchRequest messages within this * payload. */ - col_set_str(pinfo->cinfo, COL_INFO, "SASL GSS-API Decrypted payload: "); + col_set_str(pinfo->cinfo, COL_INFO, "SASL GSS-API Privacy (decrypted): "); if (sasl_tree) { - proto_tree *enc_tree; guint decr_len = tvb_reported_length(decr_tvb); - decr_tvb = gssapi_encrypt.gssapi_decrypted_tvb; - enc_tree = proto_tree_add_subtree_format(sasl_tree, decr_tvb, 0, -1, - ett_ldap_payload, NULL, "GSS-API Decrypted payload (%d byte%s)", + ett_ldap_payload, NULL, "GSS-API Encrypted payload (%d byte%s)", decr_len, plurality(decr_len, "", "s")); - - dissect_ldap_payload(decr_tvb, pinfo, enc_tree, ldap_info, is_mscldap); } + + dissect_ldap_payload(decr_tvb, pinfo, enc_tree, ldap_info, is_mscldap); } else if (gssapi_encrypt.gssapi_data_encrypted) { /* @@ -1257,6 +1257,9 @@ static void proto_tree_add_item(sasl_tree, hf_ldap_gssapi_encrypted_payload, gssapi_tvb, ver_len, -1, ENC_NA); } else { + tvbuff_t *plain_tvb = tvb_new_subset_remaining(gssapi_tvb, ver_len); + proto_tree *plain_tree = NULL; + /* * The payload was not encrypted (sealed) but was signed via a MIC token. * If krb5_tok_id == KRB_TOKEN_CFX_WRAP, the payload was wrapped within @@ -1266,18 +1269,14 @@ static void col_set_str(pinfo->cinfo, COL_INFO, "SASL GSS-API Integrity: "); if (sasl_tree) { - guint plain_len; - proto_tree *plain_tree; - - plain_tvb = tvb_new_subset_remaining(gssapi_tvb, ver_len); - plain_len = tvb_reported_length(plain_tvb); + guint plain_len = tvb_reported_length(plain_tvb); plain_tree = proto_tree_add_subtree_format(sasl_tree, plain_tvb, 0, -1, ett_ldap_payload, NULL, "GSS-API payload (%d byte%s)", plain_len, plurality(plain_len, "", "s")); - - dissect_ldap_payload(plain_tvb, pinfo, plain_tree, ldap_info, is_mscldap); } + + dissect_ldap_payload(plain_tvb, pinfo, plain_tree, ldap_info, is_mscldap); } } } else { diff --git a/epan/dissectors/packet-ldap.c b/epan/dissectors/packet-ldap.c index 8bf1b513ff..0e774275db 100644 --- a/epan/dissectors/packet-ldap.c +++ b/epan/dissectors/packet-ldap.c @@ -4104,7 +4104,7 @@ static void ((strcmp(ldap_info->auth_mech, "GSS-SPNEGO") == 0) || /* auth_mech may have been set from the bind */ (strcmp(ldap_info->auth_mech, "GSSAPI") == 0))) { - tvbuff_t *gssapi_tvb, *plain_tvb = NULL, *decr_tvb= NULL; + tvbuff_t *gssapi_tvb = NULL; int ver_len; int tmp_length; gssapi_encrypt_info_t gssapi_encrypt; @@ -4134,6 +4134,9 @@ static void return; } if (gssapi_encrypt.gssapi_decrypted_tvb) { + tvbuff_t *decr_tvb = gssapi_encrypt.gssapi_decrypted_tvb; + proto_tree *enc_tree = NULL; + /* * The LDAP payload (blob) was encrypted and we were able to decrypt it. * The data was signed via a MIC token, sealed (encrypted), and "wrapped" @@ -4141,20 +4144,17 @@ static void * one or more LDAPMessages such as searchRequest messages within this * payload. */ - col_set_str(pinfo->cinfo, COL_INFO, "SASL GSS-API Decrypted payload: "); + col_set_str(pinfo->cinfo, COL_INFO, "SASL GSS-API Privacy (decrypted): "); if (sasl_tree) { - proto_tree *enc_tree; guint decr_len = tvb_reported_length(decr_tvb); - decr_tvb = gssapi_encrypt.gssapi_decrypted_tvb; - enc_tree = proto_tree_add_subtree_format(sasl_tree, decr_tvb, 0, -1, - ett_ldap_payload, NULL, "GSS-API Decrypted payload (%d byte%s)", + ett_ldap_payload, NULL, "GSS-API Encrypted payload (%d byte%s)", decr_len, plurality(decr_len, "", "s")); - - dissect_ldap_payload(decr_tvb, pinfo, enc_tree, ldap_info, is_mscldap); } + + dissect_ldap_payload(decr_tvb, pinfo, enc_tree, ldap_info, is_mscldap); } else if (gssapi_encrypt.gssapi_data_encrypted) { /* @@ -4167,6 +4167,9 @@ static void proto_tree_add_item(sasl_tree, hf_ldap_gssapi_encrypted_payload, gssapi_tvb, ver_len, -1, ENC_NA); } else { + tvbuff_t *plain_tvb = tvb_new_subset_remaining(gssapi_tvb, ver_len); + proto_tree *plain_tree = NULL; + /* * The payload was not encrypted (sealed) but was signed via a MIC token. * If krb5_tok_id == KRB_TOKEN_CFX_WRAP, the payload was wrapped within @@ -4176,18 +4179,14 @@ static void col_set_str(pinfo->cinfo, COL_INFO, "SASL GSS-API Integrity: "); if (sasl_tree) { - guint plain_len; - proto_tree *plain_tree; - - plain_tvb = tvb_new_subset_remaining(gssapi_tvb, ver_len); - plain_len = tvb_reported_length(plain_tvb); + guint plain_len = tvb_reported_length(plain_tvb); plain_tree = proto_tree_add_subtree_format(sasl_tree, plain_tvb, 0, -1, ett_ldap_payload, NULL, "GSS-API payload (%d byte%s)", plain_len, plurality(plain_len, "", "s")); - - dissect_ldap_payload(plain_tvb, pinfo, plain_tree, ldap_info, is_mscldap); } + + dissect_ldap_payload(plain_tvb, pinfo, plain_tree, ldap_info, is_mscldap); } } } else { @@ -5633,7 +5632,7 @@ void proto_register_ldap(void) { NULL, HFILL }}, /*--- End of included file: packet-ldap-hfarr.c ---*/ -#line 2158 "./asn1/ldap/packet-ldap-template.c" +#line 2157 "./asn1/ldap/packet-ldap-template.c" }; /* List of subtrees */ @@ -5707,7 +5706,7 @@ void proto_register_ldap(void) { &ett_ldap_T_warning, /*--- End of included file: packet-ldap-ettarr.c ---*/ -#line 2172 "./asn1/ldap/packet-ldap-template.c" +#line 2171 "./asn1/ldap/packet-ldap-template.c" }; /* UAT for header fields */ static uat_field_t custom_attribute_types_uat_fields[] = { @@ -5896,7 +5895,7 @@ proto_reg_handoff_ldap(void) /*--- End of included file: packet-ldap-dis-tab.c ---*/ -#line 2344 "./asn1/ldap/packet-ldap-template.c" +#line 2343 "./asn1/ldap/packet-ldap-template.c" dissector_add_uint_range_with_preference("tcp.port", TCP_PORT_RANGE_LDAP, ldap_handle); -- 2.11.4.GIT