Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#812 closed Bug / Defect (fixed)

SIGUSR1 restart does not re-open tun even if the pushed ip address changes

Reported by: Selva Nair Owned by:
Priority: critical Milestone: release 2.4.1
Component: Generic / unclassified Version: OpenVPN 2.4.0 (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:
Cc: Steffan Karger

Description

After sitting on this for a day it still looks like a bug so here goes:

An easy way to reproduce:

Have --persist-tun in client config so tun is preserved unless route, ifconfig etc. changes on restart. Successfully connect to a server and arrange the server to push a new ip address next time (say using ifconfig-push in a ccd file). Restart the client using SIGUSR1 and watch the connection to complete without any tun re-open. That means the old ip and routes remain and the tunnel is broken.

The slightly edited log below is from Linux but looks like this is a cross-platform problem..

First connection:

Tue Jan  3 01:13:59 2017 us=913576 PUSH: Received control message: 'PUSH_REPLY,tun-ipv6,route-gateway 10.9.0.1,topology subnet,ping 10,ping-restart 120,ifconfig 10.9.0.10 255.255.255.0,peer-id 1,cipher AES-256-GCM'
Tue Jan  3 01:13:59 2017 us=913893 OPTIONS IMPORT: timers and/or timeouts modified
Tue Jan  3 01:13:59 2017 us=913919 OPTIONS IMPORT: --ifconfig/up options modified
Tue Jan  3 01:13:59 2017 us=913941 OPTIONS IMPORT: route-related options modified
Tue Jan  3 01:13:59 2017 us=913968 OPTIONS IMPORT: peer-id set
Tue Jan  3 01:13:59 2017 us=914019 OPTIONS IMPORT: data channel crypto options modified
Tue Jan  3 01:13:59 2017 us=914191 Data Channel Encrypt: Cipher 'AES-256-GCM' initialized with 256 bit key
Tue Jan  3 01:13:59 2017 us=914222 Data Channel Decrypt: Cipher 'AES-256-GCM' initialized with 256 bit key
Tue Jan  3 01:13:59 2017 us=914249 Preserving previous TUN/TAP instance: tun0
Tue Jan  3 01:13:59 2017 us=914291 Initialization Sequence Completed

kill -USR1 client-pid to restart:

Tue Jan  3 01:14:09 2017 us=103647 event_wait : Interrupted system call (code=4)
Tue Jan  3 01:14:09 2017 us=103816 TCP/UDP: Closing socket
Tue Jan  3 01:14:09 2017 us=103854 SIGUSR1[hard,] received, process restarting
Tue Jan  3 01:14:09 2017 us=103873 Restart pause, 5 second(s)
Tue Jan  3 01:14:14 2017 us=126645 Re-using SSL/TLS context
Tue Jan  3 01:14:14 2017 us=177036 Local Options String (VER=V4): 'V4,dev-type tun,link-mtu 1557,tun-mtu 1500,proto UDPv4,keydir 1,cipher AES-256-CBC,auth SHA1,keysize 256,tls-auth,key-method 2,tls-client'
Tue Jan  3 01:14:14 2017 us=177053 Expected Remote Options String (VER=V4): 'V4,dev-type tun,link-mtu 1557,tun-mtu 1500,proto UDPv4,keydir 0,cipher AES-256-CBC,auth SHA1,keysize 256,tls-auth,key-method 2,tls-server'
Tue Jan  3 01:14:14 2017 us=177075 TCP/UDP: Preserving recently used remote address: [AF_INET]xx.xx.xx.136:1194
Tue Jan  3 01:14:14 2017 us=220250 TLS: Initial packet from [AF_INET]xx:1194, sid=8ea5f488 c9741ca4
Tue Jan  3 01:14:14 2017 us=265140 VERIFY OK: depth=1, C=CA, ST=ON, ......
Tue Jan  3 01:14:14 2017 us=266554 VERIFY OK: depth=0, C=CA, ST=ON, ......
Tue Jan  3 01:14:14 2017 us=378413 Control Channel: TLSv1.2, cipher TLSv1/SSLv3 ECDHE-RSA-AES256-GCM-SHA384, 1024 bit RSA
Tue Jan  3 01:14:14 2017 us=378466 [xxxxx] Peer Connection Initiated with [AF_INET]xx.xx.xx.136:1194
Tue Jan  3 01:14:15 2017 us=520680 SENT CONTROL [sanselvpn]: 'PUSH_REQUEST' (status=1)
Tue Jan  3 01:14:15 2017 us=553546 PUSH: Received control message: 'PUSH_REPLY,tun-ipv6,route-gateway 10.9.0.1,topology subnet,ping 10,ping-restart 120,ifconfig 10.9.0.2 255.255.255.0,peer-id 0,cipher AES-256-GCM'
Tue Jan  3 01:14:15 2017 us=553723 OPTIONS IMPORT: timers and/or timeouts modified
Tue Jan  3 01:14:15 2017 us=553737 OPTIONS IMPORT: --ifconfig/up options modified
Tue Jan  3 01:14:15 2017 us=553749 OPTIONS IMPORT: route-related options modified
Tue Jan  3 01:14:15 2017 us=553761 OPTIONS IMPORT: peer-id set
Tue Jan  3 01:14:15 2017 us=553773 OPTIONS IMPORT: adjusting link_mtu to 1624
Tue Jan  3 01:14:15 2017 us=553784 OPTIONS IMPORT: data channel crypto options modified
Tue Jan  3 01:14:15 2017 us=553935 Data Channel Encrypt: Cipher 'AES-256-GCM' initialized with 256 bit key
Tue Jan  3 01:14:15 2017 us=553953 Data Channel Decrypt: Cipher 'AES-256-GCM' initialized with 256 bit key
Tue Jan  3 01:14:15 2017 us=553970 Preserving previous TUN/TAP instance: tun0
Tue Jan  3 01:14:15 2017 us=554001 Initialization Sequence Completed

Note the pushed ip changed from 10.9.0.10 to 10.9.0.2 but the client doesn't care.

Change History (7)

comment:1 Changed 7 years ago by Selva Nair

Oh oh... more debugging shows something interesting.. The push options digest is the same no matter what I push. Here it is
93b885adfe0da089cdf634904fd59f71,
which is the digest of a null character -- strange, not a null string "" but of a nul terminator:

perl -e 'print chr(0)' | md5sum = 93b885adfe0da089cdf634904fd59f71

Unless I'm doing something wrong, the digest is broken.

comment:2 Changed 7 years ago by David Sommerseth

Cc: Steffan Karger added

This sounds related to this commit:

commit ec4dff3bbdcc9fedf7844701dc5aa2679d503667
Author: Steffan Karger <steffan@karger.me>
Date:   Thu Dec 15 22:46:06 2016 +0100

    Don't reopen tun if cipher changes
    
    When the pulled options change, OpenVPN will attempt to reopen the tun
    device.  That might fail if the process has already dropper privileges,
    and is not needed unless the tun MTU is changed.  This patch therefore
    ignores the cipher value for the digest if a fixed tun-mtu is used.
    
    Additionally, this patch changes the md_ctx_update() call to include the
    trailing zero byte of each option, to make sure that parsing "foo,bar"
    results in a different hash than "foobar".  (Sorry for not catching that
    during the review...)
    
    The unit tests are a bit lame, but it secretly serves as a way to lower
    the bar for adding more buffer.c unit tests.
    
    Trac: #761
    Signed-off-by: Steffan Karger <steffan@karger.me>
    Acked-by: David Sommerseth <davids@openvpn.net>
    Message-Id: <1481838366-32335-1-git-send-email-steffan@karger.me>
    URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13579.html
    Signed-off-by: David Sommerseth <davids@openvpn.net>

syzzer? The md_ctx_update() seems to be doing the wrong thing ....

comment:3 Changed 7 years ago by David Sommerseth

I see this difference between the mtx_ctx_update() before and after this commit:

-            md_ctx_update(ctx, (const uint8_t *) line, strlen(line));
   ...
+    md_ctx_update(ctx, (const uint8_t *) line, strlen(line)+1);

Is it the +1 which causes havoc?

Last edited 7 years ago by David Sommerseth (previous) (diff)

comment:4 Changed 7 years ago by Selva Nair

Not the +1, but the call is moved out of the while loop so its evaluated only after the loop completes at which point all that remains is an empty string. The +1 makes the terminating nul included in the digest computation, so finally we get the digest of just '\0'.

comment:5 Changed 7 years ago by Gert Döring

commit a5dbf8c8dab23c47407c3f833c4f4aae52408af1 (master)
commit bf72ae68c05922c289056f2f851ed36014449cdf (release/2.4)

Author: Selva Nair
Date: Tue Jan 3 16:42:18 2017 -0500

Fix push options digest update

comment:6 Changed 7 years ago by David Sommerseth

Resolution: fixed
Status: newclosed

comment:7 Changed 7 years ago by David Sommerseth

Milestone: release 2.4.1
Note: See TracTickets for help on using tickets.