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<int> &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