Re: Bug 15709: Segfault on MacOS; help wanted

[Peter Wu <peter () lekensteyn nl>]

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