Bug 15709: Segfault on MacOS; help wanted

[Uli Heilmeier <zeugs () heilmeier eu>]

Hi list,

I'm stumbled over a segfault [1] in Wireshark and I'm currently trying to solve it. However I'm totally lost and need
some hints to go on.

The crash is a EXC_BAD_ACCESS when typing or pasting in a malformed string (with non-hex characters) as an Initiator
Cookie in the ISAKMP IKEv1 Decryption Table.

Debugging this with lldb shows that ikev1_uat_data_update_cb() of uat_new() is called, sets err to "Length of
Initiator's COOKIE must be %d octets (%d hex characters)" and returns FALSE.
However the error message is not displayed and WS crashes in the isakmp_init_protocol() function when calling
'memcpy(ic_key, ikev1_uat_data[i].icookie, COOKIE_SIZE);'. As far as I can see this is because ikev1_uat_data[i].icookie
is NULL.

A simple workaround would be to check icookie_len before calling memcpy (see patch below). But I think the root cause is
in the handling of the UAT.
lldb shows here most of the time assembler code (the qt libs stuff), and I'm lost.

Would it make sense to have a post_update_cb() function and check here for the input?
What's the difference between update_cb() and post_update_cb()?
Should I look anywhere else?

Any hints welcome how I can debug this.

Thanks & Cheers
Uli

[1]: https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=15709


---- patch:

diff --git a/epan/dissectors/packet-isakmp.c b/epan/dissectors/packet-isakmp.c
index c434c23ccc..5f4a09e776 100644
--- a/epan/dissectors/packet-isakmp.c
+++ b/epan/dissectors/packet-isakmp.c
@@ -5861,14 +5861,16 @@ isakmp_init_protocol(void) {
       free_cookie_key, free_cookie_value);

   for (i = 0; i < num_ikev1_uat_data; i++) {
-    ic_key = (guint8 *)g_slice_alloc(COOKIE_SIZE);
-    memcpy(ic_key, ikev1_uat_data[i].icookie, COOKIE_SIZE);
+    if (ikev1_uat_data[i].icookie_len == COOKIE_SIZE) {
+      ic_key = (guint8 *)g_slice_alloc(COOKIE_SIZE);
+      memcpy(ic_key, ikev1_uat_data[i].icookie, COOKIE_SIZE);

-    decr = create_decrypt_data();
-    memcpy(decr->secret, ikev1_uat_data[i].key, ikev1_uat_data[i].key_len);
-    decr->secret_len = ikev1_uat_data[i].key_len;
+      decr = create_decrypt_data();
+      memcpy(decr->secret, ikev1_uat_data[i].key, ikev1_uat_data[i].key_len);
+      decr->secret_len = ikev1_uat_data[i].key_len;

-    g_hash_table_insert(isakmp_hash, ic_key, decr);
+      g_hash_table_insert(isakmp_hash, ic_key, decr);
+    }
   }
   ikev2_key_hash = g_hash_table_new(ikev2_key_hash_func, ikev2_key_equal_func);
   for (i = 0; i < num_ikev2_uat_data; i++) {
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe