Openconnect crashes on auth dialog

[Konstantin]

The authentication dialog of openconnect crashes.

Steps to reproduce:

Add net vpn connection in Gui:
* Settings -> Network -> VPN -> +
* Selecct "Multi-protocol vpn client (openconnect)"
* Select "Cisco AnyConnect or openconnect"
* Enter as gateway "vpn.sydney.edu.au"
* Click "Add"
* Activate the newly added VPN
* Click "Connect"

Now the login window briefly shows up before it disappears without error message or anything.

The syslog contains the following lines indicating that the authentication dialog segfaulted.

Sep 27 10:44:06 durian kernel: [214841.848187] nm-openconnect-[150897]: segfault at 0 ip 00005602eb992570 sp 00007ffc8f6abd40 error 4 in nm-openconnect-auth-dialog[5602eb98e000+6000]
Sep 27 10:44:06 durian kernel: [214841.848205] Code: 66 2e 0f 1f 84 00 00 00 00 00 49 8b 3e 49 83 c6 08 e8 f4 d1 ff ff 4c 39 f5 75 ef 48 8b 7c 24 10 31 ed e8 e3 d1 ff ff 0f 1f 00 <49> 8b 3c ec 48 83 c5 01 e8 d3 d1 ff ff 39 eb 7f ef 4c 89 e7 e8 c7

The system is Ubuntu 22.04, recently updated from 20.04. I attached the output of
ubuntu-bug network-manager-openconnect --save=filename
To this report.

[Konstantin (list-kseiler)]

I did some debugging and found the reason for the crash: an array is attempted to be freed even though it could be NULL.

Below is the code fragment with the missing if-statenment inserted.

In auth-dialog/main.c around line 720:

function static void cookie_cb (GObject *source_obj, GAsyncResult *res, gpointer data):
[...]
    for (i = 0; i < 2 * (num_cookies + 1); i++) {
        free(cookie_array[i]);
    }
    free(cookie_array);

    if (headers_array) { // <=========== THIS LINE IS MISSING AND MUST BE ADDED
        for (i = 0; i < 2 * (num_headers + 1); i++) {
            free(headers_array[i]);
        }
        free(headers_array);
    } // <=========== THIS LINE IS MISSING AND MUST BE ADDED
    if (headers)
        soup_message_headers_free (headers);

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "Patch to avoid freeing pointer if NULL." seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Andrew McNamara (andrewm-object-craft)]

Proposed patch fixes the issue for me.

[dwmw2 (dwmw2)]

Hm, that's odd. It should only actually be freeing anything there if the `num_headers` count is greater than zero. In which case it *should* have allocated `headers_array`. Unless the calloc() failed, but that is unlikely.

Then again, I'm looking at the upstream code. And that has had the check for `headers_array` being NULL since it was first committed in https://gitlab.gnome.org/GNOME/NetworkManager-openconnect/-/commit/9089be31#5122bcd6270f3f91d7aa6174306705bad477d30f_699_721

I'd suggest reconciling with the upstream version of the code.

[Andrew McNamara (andrewm-object-craft)]

It looks to me like the bug is still in the upstream code. `headers_array` is only allocated if `headers` is not null, and that will be null if `resource` or `response` is null.

[dwmw2 (dwmw2)]

Yes, but num_headers never changes from zero in that case, so no attempt is made to free anything.

And also, the loop which does the freeing has the check for headers_array being NULL anyway, as part of the termination condition:

 for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
  free(headers_array[i]);
 }

I still don't quite see *how* it can happen when num_headers is non-zero though, unless the calloc() fails. Or unless openconnect_webview_load_changed() frees them, which surely it shouldn't?

[Konstantin (list-kseiler)]

This check for headers_array in the loop’s termination condition is only in the upstream code, not in the one that was in Ubuntu at the time I reported the bug and submitted the patch.

When I made the patch I decided to put a proper if-clause there because it’s easier to understand what is going on (the check in the for loop seems like obfuscated programming to me) and technically it’s even faster to check only once instead of at every loop iteration (even though this minimal performance gain is hardly significant here).

[dwmw2 (dwmw2)]

> This check for headers_array in the loop’s termination condition is only in the upstream code, not in the one that was in Ubuntu at the time I reported the bug and submitted the patch.

Yes. That's why I'm pointing out the difference and suggesting that we reconcile the code.

> When I made the patch I decided to put a proper if-clause there because it’s easier to understand what is going on (the check in the for loop seems like obfuscated programming to me)

Agreed. Happy to accept a patch which cleans that up in the upstream tree. Although...

> and technically it’s even faster to check only once instead of at every loop iteration (even though this minimal performance gain is hardly significant here).

In practice any compiler worth its salt should optimise that away because it'll know that calling free() won't affect the value of headers_array. See in 'objdump -S' output how the loop only jump back from 0x8d98 to 0x9d88, adding 8 to the pointer every time until it reaches the precalculated end value.

        free(cookie_array);
    8d6a: 48 8b 3c 24 mov (%rsp),%rdi
    8d6e: e8 ed da ff ff call 6860 <free@plt>
        for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
    8d73: 4d 85 ff test %r15,%r15
    8d76: 74 22 je 8d9a <cookie_cb+0x25a>
    8d78: 43 8d 44 24 01 lea 0x1(%r12,%r12,1),%eax
    8d7d: 4c 89 fd mov %r15,%rbp
    8d80: 4d 8d 64 c7 08 lea 0x8(%r15,%rax,8),%r12
    8d85: 0f 1f 00 nopl (%rax)
                free(headers_array[i]);
    8d88: 48 8b 7d 00 mov 0x0(%rbp),%rdi
        for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
    8d8c: 48 83 c5 08 add $0x8,%rbp
                free(headers_array[i]);
    8d90: e8 cb da ff ff call 6860 <free@plt>
        for (i = 0; headers_array && i < 2 * (num_headers + 1); i++) {
    8d95: 49 39 ec cmp %rbp,%r12
    8d98: 75 ee jne 8d88 <cookie_cb+0x248>
        free(headers_array);
    8d9a: 4c 89 ff mov %r15,%rdi
    8d9d: e8 be da ff ff call 6860 <free@plt>