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 ___________________________________________________________________________ Sent via: Wireshark-dev mailing list <[email protected]> Archives: https://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev mailto:[email protected]?subject=unsubscribe
