Hi Gert and devs,

During my experimentation with this dual mode, I've had to go through a lot
of the code in the ssl.c library file. While I was tracing and testing my
modifications, I think I found another potential crash path but I'm also
not sure if it was my modifications or if this is indeed a bug potentially.

This was the GDB backtrace I was able to capture during my own modified
testing:



Thread 8 "openvpn" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffdffff6c0 (LWP 638500)]
0x00007ffff74add55 in __GI___libc_free (mem=0x2) at ./malloc/malloc.c:3375
warning: 3375   ./malloc/malloc.c: No such file or directory
(gdb) Quit
(gdb) bt
#0  0x00007ffff74add55 in __GI___libc_free (mem=0x2) at
./malloc/malloc.c:3375
#1  0x0000555555564be1 in free_buf (buf=buf@entry=0x7fffd40068b0) at
buffer.c:186
#2  0x00005555555e1d04 in key_state_free (ks=ks@entry=0x7fffd40065d0,
clear=clear@entry=false) at ssl.c:928
#3  0x00005555555e1eca in tls_session_free (session=0x7fffd4005ac8,
clear=false) at ssl.c:1076
#4  0x00005555555e4c29 in tls_multi_free (multi=0x7fffd4003700,
clear=clear@entry=true) at ssl.c:1286
#5  0x0000555555586c79 in do_close_tls (c=0x7fffd4000d40) at init.c:3986
#6  close_instance (c=c@entry=0x7fffd4000d40) at init.c:4868
#7  0x00005555555872f8 in close_context (c=c@entry=0x7fffd4000d40,
sig=sig@entry=15, flags=flags@entry=1) at init.c:5074
#8  0x000055555559be11 in multi_close_instance (m=m@entry=0x7ffff73fd7c0,
mi=0x7fffd4000b70, shutdown=shutdown@entry=false) at multi.c:661
#9  0x000055555559cadb in multi_close_instance_on_signal
(m=m@entry=0x7ffff73fd7c0,
mi=<optimized out>) at multi.c:3413
#10 0x00005555555a37e4 in multi_io_action (m=m@entry=0x7ffff73fd7c0,
mi=0x0, action=<optimized out>, action@entry=1, poll=<optimized out>,
    poll@entry=false, flags=flags@entry=0, z=z@entry=1) at multi_io.c:640
#11 0x00005555555a40f4 in multi_io_process_io (b=0x7fffffff7500, f=f@entry=0,
z=1) at multi_io.c:466
#12 0x00005555555a4388 in threaded_multi_io_process_io (a=0x7ffff73fd6e0)
at multi_io.c:587
#13 0x00007ffff749caa4 in start_thread (arg=<optimized out>) at
./nptl/pthread_create.c:447
#14 0x00007ffff7529c6c in clone3 () at
../sysdeps/unix/sysv/linux/x86_64/clone3.S:78
(gdb)



I noticed in this ssl.c library file that there is a tls_session_init()
which calls upon key_state_init() for the KS_PRIMARY key ONLY, not
including the LAME_DUCK key.

https://github.com/OpenVPN/openvpn/blob/master/src/openvpn/ssl.c#L1032

key_state_init(session, &session->key[KS_PRIMARY]);

However, this init/malloc call chain does not line up with the associated
and related free method calls. The tls_session_free() method will then
call key_state_free() all ALL keys there after.

https://github.com/OpenVPN/openvpn/blob/master/src/openvpn/ssl.c#L1061

for (size_t i = 0; i < KS_SIZE; ++i)
{
        key_state_free(&session->key[i], false);
}

Now I saw that the lame duck key is eventually initialized during
renegotiation, however, I believe that if a client were to maybe connect
and then reconnect right away that the lame duck key won't be initialized
and potentially cause the crash that I observed above? It may be good for
one of the devs who knows this code base better to explore this mis-match
in init/free calls. :)

For now, as a work around in my dual modification PR, I set a bool variable
in the key state struct object that gets set to true when initialized and
then the free call can later detect that to determine when to properly free
it later!

https://github.com/stoops/openvpn-fork/blob/dual/src/openvpn/ssl.c#L861

ks->keys_init = true;

https://github.com/stoops/openvpn-fork/blob/dual/src/openvpn/ssl.c#L921

if (!ks->keys_init) { return 1; }



Thanks again for your time,
Jon C
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Reply via email to