ncp patch set breaks --inetd

[Gert Döring]

One of my test cases (which is not run always) triggers a server crash if used with --inetd on the server side.

This *is* one of the more obscure configs, but it used to work, so it should continue so...

Jul 27 19:02:53 gentoo openvpn[31823]: peer info: IV_NCP=2
...
Jul 27 19:02:53 gentoo openvpn[31823]: Data Channel Encrypt: Cipher 'BF-CBC' initialized with 128 bit key
Jul 27 19:02:53 gentoo openvpn[31823]: Data Channel Encrypt: Using 160 bit message hash 'SHA1' for HMAC authentication
Jul 27 19:02:53 gentoo openvpn[31823]: Data Channel Decrypt: Cipher 'BF-CBC' initialized with 128 bit key
Jul 27 19:02:53 gentoo openvpn[31823]: Data Channel Decrypt: Using 160 bit message hash 'SHA1' for HMAC authentication
Jul 27 19:02:53 gentoo openvpn[31823]: Control Channel: TLSv1.2, cipher TLSv1/SSLv3 ECDHE-RSA-AES256-GCM-SHA384, 2048 bit RSA
Jul 27 19:02:53 gentoo openvpn[31823]: [cron2-gentoo-i386] Peer Connection Initiated with [AF_INET]193.149.48.174:40996
Jul 27 19:02:54 gentoo openvpn[31823]: Initialization Sequence Completed
Jul 27 19:02:54 gentoo openvpn[31823]: PUSH: Received control message: 'PUSH_REQUEST'
Jul 27 19:02:54 gentoo openvpn[31823]: send_push_reply(): safe_cap=940
Jul 27 19:02:54 gentoo openvpn[31823]: SENT CONTROL [cron2-gentoo-i386]: 'PUSH_REPLY,route-gateway 10.194.8.1,route 10.194.0.0 255.255.0.0,route-ipv6 fd00:abcd:194::/48,ping 10,ping-restart 30,peer-id 0,cipher AES-256-GCM' (status=1)
Jul 27 19:02:55 gentoo openvpn[31823]: Authenticate/Decrypt? packet error: packet HMAC authentication failed
Jul 27 19:02:55 gentoo openvpn[31823]: Fatal decryption error (process_incoming_link), restarting
Jul 27 19:02:55 gentoo openvpn[31823]: OpenVPN started by inetd/xinetd cannot restart... Exiting.
Jul 27 19:02:55 gentoo openvpn[31823]: TCP/UDP: Closing socket
Jul 27 19:02:55 gentoo openvpn[31823]: Closing TUN/TAP interface
Jul 27 19:02:55 gentoo openvpn[31823]: SIGTERM[soft,decryption-error] received, process exiting
Jul 27 19:02:55 gentoo xinetd[2204]: EXIT: openvpn99 status=0 pid=31823 duration=3(sec)

from cursory reading it seems the server will not postpone crypto init in this scenario, and then fail later on when the client has successfully been told to go to AES-GCM.

A lame fix would be "if (--inetd) { ncp_disable=true; }", while a slightly better fix would be to properly deal with it :-))

The server is called from xinetd like this

        server_args = --inetd nowait --cd /root/openvpn-test-server/tap-tcp-p2mp-inetd --config server.conf

and "server.conf" contains

tls-server
ifconfig 10.194.8.1 255.255.255.0
ifconfig-ipv6 fd00:abcd:194:8::1/64 fd00:abcd:194:8::2

(notably not --server, as openvpn refuses "--inetd && --server") - so this is a variant of peer2peer mode.

Maybe peer2peer mode using --tls-client + --tls-server also breaks if someone young and daring uses "--client" vs. "--tls-server" (which means it's not really --inetd, but "--tls-server with a client that asks for PULL").

[Steffan Karger]

This seems to be a generic problem for --tls-server (without --server) on one side, and --client on the other. I would suggest to disable server-side NCP for everything that's not --server, because the moment where we decide whether to postpone data channel crypto initialization it's not clear whether the client will send a PUSH_REQUEST yet.

Could you test the attached patch?

[Gert Döring]

This patch works, but it feels a bit "fragile", as in "we identify things one by one that might need an exception here" - making the code inside push.c less readable.

So I think we should do it differently - identify options that, if set/unset, will set o->ncp_enabled = false . First and most obvious one is --server - so, if --server is not set (more precisely, mode != SERVER), disable NCP early on.

Second, as we've discussed briefly on IRC, I'd add a safeguard to push.c that will check if the server has already built keys, and if yes, log a diagnostic message (so we can see that it consciously decided to not push ciphers, because <reason>) and refuse to do so.

We might actually need another safeguard in the ccd/ handling - if the client is NOT identifying itself as NCP capable, but the admin has decided to put

   cipher foo
   push "cipher foo"

in the ccd/ file, the server will (I think) currently misbehave due to having generated a key already - and from the logs, it will be hard to understand what is happening. Have not checked the actual code, so maybe this particular caveat is already handled.

[Gert Döring]

no soup for you, we have already eaten

[Gert Döring]

@syzzer: the attached patch de-fuses this particular brittleness - "if we reach the question whether or not to push ciphers, refused to do so if we've already done our keys" (NO SOUP FOR YOU!).

Patch done with "-w" because it changes the indentation of your code, which makes a "proper" patch harder to read.

With this patch, we have:

Jul 31 20:57:59 gentoo openvpn[13819]: PUSH: Received control message: 'PUSH_REQUEST'
Jul 31 20:57:59 gentoo openvpn[13819]: PUSH: client wants to negotiate cipher (NCP), but server has already generated data channel keys, ignoring client request

I still think we should disable NCP upfront unless --client or --mode server is set - patch#2, which leads to:

Jul 31 21:13:44 gentoo openvpn[13964]: disabling NCP mode (--ncp-disable) because not in P2MP client or server mode
...
Jul 31 21:13:46 gentoo openvpn[13964]: PUSH: Received control message: 'PUSH_REQUEST'
Jul 31 21:13:46 gentoo openvpn[13964]: send_push_reply(): safe_cap=940
Jul 31 21:13:46 gentoo openvpn[13964]: SENT CONTROL [cron2-gentoo-i386]: 'PUSH_REPLY,route-gateway 10.194.8.1,route 10.194.0.0 255.255.0.0,route-ipv6 fd00:abcd:194::/48,ping 10,ping-restart 30,peer-id 0' (status=1)

Leaving the final word to you - your code, your fame :-)

[Gert Döring]

disable NCP unless p2mp --client or --mode server

[Steffan Karger]

Yes, this makes sense. If you send these to the list, I'll do an official review-and-ACK.

[Gert Döring]

Replying to syzzer:

Yes, this makes sense. If you send these to the list, I'll do an official review-and-ACK.

Woops. Saw this, and immediately forgot it again. Getting old :-)

Sent the patch set (as a single patch) to the list right now. Should show up as soon as greylisting permits, message-ID is <1471893779-8041-1-git-send-email-gert@…>

[Gert Döring]

commit d90249f73353c175ed9e7dd0a450cd084a729e20
Author: Gert Doering <gert@…>
Date: Mon Aug 22 22:24:47 2016 +0200

Fix problems with NCP and --inetd.

thanks for the review, and close :-)