Segfault in check_session_buf_not_used

[patcable]

Likely related to the use-after-free fix (#400, #417)

Describe the bug
I'm getting an occasional segfault with my OpenVPN server. There's nothing obvious in the logs that's setting this off yet but I'm now running the OpenVPN server with --verb 4 to see if I get any extra information before it happens.

To Reproduce
Not clear yet.

Expected behavior
The OpenVPN process doesn't segfault.

Version information (please complete the following information):

• OS: Ubuntu 20.04
• OpenVPN version: 2.6.7

Additional context

#0  0x0000aaaad95de984 in check_session_buf_not_used (to_link=0xaaab10aa4c40, to_link=0xaaab10aa4c40, session=<optimized out>) at ssl.c:3195
#1  tls_multi_process (multi=0xaaab10989c80, to_link=to_link@entry=0xaaab10aa4c40, to_link_addr=to_link_addr@entry=0xaaab10aa4958, to_link_socket_info=0xaaab10a87dd8, wakeup=wakeup@entry=0xffffd3a6c340) at ssl.c:3312
#2  0x0000aaaad957f3b4 in check_tls (c=0xaaab10aa3be0) at forward.h:310
#3  pre_select (c=c@entry=0xaaab10aa3be0) at forward.c:2001
#4  0x0000aaaad959f50c in multi_process_post (m=m@entry=0xffffd3a6c4e8, mi=0xaaab10aa3a20, flags=flags@entry=5) at multi.c:3049
#5  0x0000aaaad95a04bc in multi_process_timeout (m=m@entry=0xffffd3a6c4e8, mpp_flags=mpp_flags@entry=5) at multi.c:3685
#6  0x0000aaaad959aaa4 in tunnel_server_udp (top=0xffffd3a6d868) at mudp.c:512
#7  0x0000aaaad95a7c00 in openvpn_main (argc=14, argv=0xffffd3a6eb98) at openvpn.c:317
#8  0x0000ffff8192fe10 in __libc_start_main (main=0xaaaad956b930 <main>, argc=14, argv=0xffffd3a6eb98, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=<optimized out>) at ../csu/libc-start.c:308
#9  0x0000aaaad956b968 in _start () at openvpn.c:394

[cron2]

Thanks a lot for the crash dump. Someone else reported crashes on the openvpn-devel mailing list, but he had no crashdump yet (but a log file, hinting at "this happens if a TLS (re)negotiation expires, possibly because the other side is not answering").

check_session_buf_not_used() is brand new, introduced in commit cd4d819 (in branch release/2.6), and it's purpose in life is "ensure that bad things are not happening", but it seems "commit suicide" is not the right approach here... @schwabe has been pinged.

[schwabe]

I am not entirely sure what happens but it might be that we get called with already freed key states. Are you able to patch OpenVPN with this patch?

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 210a70165..9c88b9e84 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -3258,6 +3258,11 @@ check_session_buf_not_used(struct buffer *to_link, struct tls_session *session)
     for (int i = 0; i < KS_SIZE; i++)
     {
         struct key_state *ks = &session->key[i];
+        if (ks->state == S_UNDEF)
+        {
+            continue;
+        }
+
         for (int j = 0; j < ks->send_reliable->size; j++)
         {
             if (ks->send_reliable->array[i].buf.data == dataptr)
             
             

[patcable]

Just deployed a build with that patch in it, will let you know!

[patcable]

So far so good -- but there hasn't been a ton of people connected over the weekend. I think the real test will come Monday.

[patcable]

Yeah, had another couple core dumps this morning, but frustratingly apport didn't capture them because i hadnt moved the old one out of /var/crash. I'll wait for one more to happen, but then i've got to downgrade because our dev vpn is also where our development happens.

[patcable]

Alright, here's the latest one. Wish i knew what caused it so i could reproduce it, alas i've downgraded.

Reading symbols from ../openvpn-2.6.7/usr/sbin/openvpn...
[New LWP 747988]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1".
Core was generated by `/usr/sbin/openvpn --daemon ovpn-server --status /run/openvpn/server.status 10 -'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000aaaabec3e984 in check_session_buf_not_used (to_link=0xaaaae68209f0, to_link=0xaaaae68209f0, session=<optimized out>)
    at ssl.c:3195
3195	ssl.c: No such file or directory.

[cron2]

Is this unmodified 2.6.7 or with the S_UNDEF patch applied? It really shouldn't crash anymore, then, according to the debug infos we've received so far... scratch head

[patcable]

thats with 2.6.7 with S_UNDEF patch applied yeah.

[cron2]

Could you re-test with 2.6.8 please? We're reasonably sure that we've fixed the issue - at least the way we could reproduce is fully gone.

[braindead-bf]

Is this the real issue? The for loop here is using j as an iterator but checking the array at index i (then reporting j as the array index when it matches dataptr):

for (int j = 0; j < ks->send_reliable->size; j++)
        {
            if (ks->send_reliable->array[i].buf.data == dataptr)
            {
                msg(M_INFO, "Warning buffer of freed TLS session is still in"
                    " use (session->key[%d].send_reliable->array[%d])",
                    i, j);
...

[schwabe]

That is a good catch. Fortunatily i is always between 0 and 2 and ks->send_reliable->size is (when it is defined) always 6 (TLS_RELIABLE_N_SEND_BUFFERS). So while the check is stupid with j, it at least is not crashing or anything similar.

[cron2]

Is this the real issue?

No, that's just broken code... the crash-causing issue (that we could track down) was that ks->send_reliable can be NULL, which wasn't checked in 2.6.7.

But the one you found need fixing as well as it renders the whole check a bit useless (it should never trigger, but if the check is not checking correctly in the first place, this ensures it never will...)

[braindead-bf]

Thanks, I submitted PR #463 for this fix.

[patcable]

Just to check back in -- i've been running 2.6.8 for a while without issue which is a good sign.

[cron2]

So thanks for all the testing and feedback. I have now committed & pushed the fix for the i/j loop variable confusion, and I think this can now be closed for good. Such a haunted function.