Re: [Wireshark-dev] Bug 15709: Segfault on MacOS; help wanted
On Apr 24, 2019, at 4:14 PM, Peter Wu wrote: > It should be displayed in the UAT dialog and prevent you from saving it. > However on macOS I have seen that events are not always processed in the > expected order: > https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=15709#c5 Or maybe not processed at all. In that example, I've found that the only way to add a new Decode As entry on macOS is to tab out of the combo box for the "decode it as..." protocol before clicking a button to add the entry. (All that in addition to the combo box looking ugly as *hell* on macOS when it's active.) ___ Sent via:Wireshark-dev mailing list Archives:https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] Bug 15709: Segfault on MacOS; help wanted
Thanks a lot Peter for your help! > Also if you have not already, build with cmake -DENABLE_ASAN=1. I > suspect that it might blow up with a use-after-free warning before the > NULL pointer dereference. Yes, you're right. After compiling it with -DENABLE_ASAN=1 and -DCMAKE_BUILD_TYPE=Debug it fails at: * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) frame #0: 0x7fff588c9d89 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 169 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell: -> 0x7fff588c9d89 <+169>: movq (%rsi), %rcx 0x7fff588c9d8c <+172>: movq (%rsi,%rdx), %r8 0x7fff588c9d90 <+176>: movq %rcx, (%rdi) 0x7fff588c9d93 <+179>: movq %r8, (%rdi,%rdx) Target 0: (Wireshark) stopped. = ==18967==ERROR: AddressSanitizer: SEGV on unknown address 0x (pc 0x7fff588c9d89 bp 0x7ffee4d3a9c0 sp 0x7ffee4d3a9c0 T0) ==18967==The signal is caused by a READ memory access. ==18967==Hint: address points to the zero page. #0 0x7fff588c9d88 in _platform_memmove$VARIANT$Haswell (libsystem_platform.dylib:x86_64+0x1d88) #1 0x10e624854 in __asan_memcpy (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x59854) #2 0x116c36936 in isakmp_init_protocol packet-isakmp.c:5866 #3 0x118e76e4a in call_routine packet.c:247 #4 0x10d8637ec in g_slist_foreach (libglib-2.0.0.dylib:x86_64+0x5c7ec) #5 0x118e76f59 in init_dissection packet.c:328 So I will have a look at the UAT part. Cheers Uli ___ Sent via:Wireshark-dev mailing list Archives:https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] Bug 15709: Segfault on MacOS; help wanted
On Wed, Apr 24, 2019 at 10:03:24PM +0200, Uli Heilmeier wrote:
> 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
It should be displayed in the UAT dialog and prevent you from saving it.
However on macOS I have seen that events are not always processed in the
expected order:
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=15709#c5
> 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.
This should never happen. The UAT core (see epan/uat-int.h) has two
separate arrays that store the data:
struct epan_uat {
// ...
GArray* user_data; /**< An array of valid records that will be exposed
to the dissector. */
GArray* raw_data; /**< An array of records containing possibly
invalid data. For internal use only. */
GArray* valid_data; /**< An array of booleans describing whether the
records in 'raw_data' are valid or not. */
The "user_data" array is the one that is exposed to the IKEv2 dissector.
The "raw_data" array might contain invalid entries, but in that case
"valid_data" should be FALSE.
Your observation seems to suggest that the Qt UI has a bug with
validation and updating the UAT. The relevant code is in
ui/qt/models/uat_model.cpp
bool UatModel::setData(const QModelIndex , const QVariant , int
role)
{
// ...
const QList _cols = checkRow(row);
// ...
if (record_errors[row].isEmpty()) {
// If all fields are valid, invoke the update callback
if (uat_->update_cb) {
char *err = NULL;
if (!uat_->update_cb(rec, )) {
// TODO the error is not exactly on the first column, but
we need a way to show the error.
record_errors[row].insert(0, err);
g_free(err);
}
}
uat_update_record(uat_, rec, TRUE);
} else {
uat_update_record(uat_, rec, FALSE);
}
Calling checkRow should have updated the "record_errors[row]" list. If
any error has occured, it should call uat_update_record(..., FALSE) to
mark a record as bad. This seems rather solid, so I don't think it is
the cause.
> 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?
Yes you could try that. See also the large comment on top of epan/uat.h.
In particular:
* The records contents are only guaranteed to be valid in the post_update_cb
* function.
> What's the difference between update_cb() and post_update_cb()?
update_cb is called after updating an individual record. post_update_cb
is called once a UAT has fully loaded. Use update_cb for validation of
individual entries, and possibly converting data to a better
representation. For example, packet-wireguard.c converts a base64 string
to bytes.
Use post_update_cb to apply the changes from the "user_data" parameter
that was given to "uat_new". In the case of IKEv1, that would be
ikev1_uat_data (with num_ikev1_uat_data items). Again packet-wireguard.c
has an example where it clears the previous set of keys and loads the
new keys (see wg_key_uat_apply).
Also if you have not already, build with cmake -DENABLE_ASAN=1. I
suspect that it might blow up with a use-after-free warning before the
NULL pointer dereference.
--
Kind regards,
Peter Wu
https://lekensteyn.nl
___
Sent via:Wireshark-dev mailing list
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Bug 15709: Segfault on MacOS; help wanted
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
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
