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 &index, const QVariant &value, int
role)
{
// ...
const QList &updated_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, &err)) {
// 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
Re: [Wireshark-dev] [Wireshark-users] termshark: a terminal UI for tshark
Hi Peter, Thanks for adding termshark to the wiki. I have to admit, somewhat sheepishly, that I was not aware of sharkd... I will definitely look into that. Just one day in, several people have already requested stream reassembly as a feature! All the best, Graham On Tue, Apr 23, 2019 at 6:46 PM Peter Wu wrote: > (+cc wireshark-dev since some may find this interesting.) > > Hi Graham, > > This looks neat, I have added it to the wiki: > https://wiki.wireshark.org/Tools > > Are you aware of sharkd? For interactive use it might be a more suitable > backend than tshark. sharkd is part of Wireshark and was developed by > Jakub Zawadzki who wrote it for use with Webshark, https://webshark.io/ > > Use of that interface could make things like Follow Stream much easier > since you do not have to manually parse the tshark output and can > instead read JSON. As the "d" in sharkd might suggest, this process > remains up and running until you force it to quit. > > The main logic is implemented in > https://github.com/wireshark/wireshark/blob/master/sharkd_session.c > > with corresponding tests in > https://github.com/wireshark/wireshark/blob/master/test/suite_sharkd.py > > If you encounter any limitations or have suggestions, please let us > know. Thanks :) > > Kind regards, > Peter > > On Mon, Apr 22, 2019 at 10:09:17PM -0400, Graham Clark wrote: > > Hi everyone - I thought you might be interested in this spare-time > project: > > > > https://termshark.io > > > > In my professional life I quite often find myself on a remote machine > > debugging something, and with a need to look at a pcap. I wrote > termshark to > > make it easy to scan the pcap immediately and to avoid having to scp it > > around. Behind the scenes, tshark provides all the intelligence, so > > termshark > > depends on tshark being installed. Termshark runs the input pcap through > > tshark, and uses the PDML and PSML to provide Wireshark-like views of > each > > packet. Currently you can view a pcap, sniff on an interface (if > permissions > > allow), and filter using Wireshark's display filters. There's so much > more > > it > > could do easily through tshark, like stream reassembly, display of > > conversations, statistics, etc, but I wanted to push out v1 so this is > > where I > > drew the line. > > > > Termshark is written in Go and makes heavy use of the excellent tcell > > library > > for control of the terminal. Because Go is so naturally portable, there > are > > versions of termshark on github for Linux (+termux/Android), FreeBSD, > macOS > > and even Windows. > > > > The source code with build instructions is here: > > https://github.com/gcla/termshark > > > > I hope you find it useful, and I'm very interested to hear your feedback. > > > > Graham > ___ > 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 ___ 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
