Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#906 closed Bug / Defect (fixed)

Cipher negotiation succeeds when it should fail

Reported by: signo- Owned by: Steffan Karger
Priority: major Milestone: release 2.4.4
Component: Generic / unclassified Version: OpenVPN 2.4.0 (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords: ncp
Cc: David Sommerseth

Description

When a client and a server specify have ncp enabled and no common ciphers are specified the initial cipher negotiation fails but after an automatic restart it succeeds. I've included my client/server logs as attachments to hopefully clarify the issue..

Reproduction steps:

  1. Start OpenVPN v2.4 server with a specified --cipher and a few ciphers in --ncp-ciphers (Example config below)
  2. Start OpenVPN 2.4 client with --cipher and --ncp-ciphers specified. make sure none of the client ciphers are included in the server's --cipher or --ncp-ciphers list (Example config below)
  3. Client logs should print out the following: Error: pushed cipher not allowed - AES-128-GCM not in AES-192-GCM or AES-256-CBC
  4. After waiting about a minute OpenVPN client will automatically restart and successfully initiate a connection to the server.

Server Version:
OpenVPN 2.4.0 mipsel-unknown-linux-uclibc [SSL (OpenSSL)] [LZO] [LZ4] [EPOLL] [MH/PKTINFO] [AEAD] built on Jun 19 2017

Server Command Line Params:
openvpn --port 1194 --server 10.8.0.0 255.255.255.0 --topology p2p --proto udp --dev-type tun --ncp-ciphers AES-128-GCM:AES-192-CBC:AES-192-GCM:AES-256-CBC:AES-256-GCM --cipher AES-256-CBC --auth sha1 --verify-client-cert require --ca ca.crt --dh dh2048.pem --cert server.crt --key server.key --verb 0 --keepalive 30 150 --reneg-bytes 0 --reneg-sec 3600 --dev ovpn

Client Version:
OpenVPN 2.4.2 x86_64-pc-linux-gnu [SSL (OpenSSL)] [LZO] [LZ4] [EPOLL] [PKCS11] [MH/PKTINFO] [AEAD] built on May 11 2017

Client Command Line Params:
sudo openvpn --remote 192.168.4.1 --dev tun --client --ca ca.crt --cert client.crt --key client.key --cipher AES-192-GCM --ncp-ciphers AES-256-CBC --verb 4 --proto udp --log client.log

Attachments (3)

server.log (23.3 KB) - added by signo- 3 years ago.
Server log
client.log (25.7 KB) - added by signo- 3 years ago.
Client Log
persist-key_enabled_vs_disabled.diff (4.4 KB) - added by mknutson 3 years ago.

Download all attachments as: .zip

Change History (18)

Changed 3 years ago by signo-

Attachment: server.log added

Server log

Changed 3 years ago by signo-

Attachment: client.log added

Client Log

comment:1 Changed 3 years ago by David Sommerseth

Keywords: ncp added
Owner: set to Steffan Karger
Status: newassigned

comment:2 Changed 3 years ago by plaisthos

Looks like it gets into the local options showhow despite not being accepted:

Thu Jun 22 20:35:29 2017 us=735967 Local Options String (VER=V4): 'V4,dev-type tun,link-mtu 1549,tun-mtu 1500,proto UDPv4,cipher AES-128-GCM,auth [null-digest],keysize 128,key-method 2,tls-client'

comment:3 Changed 3 years ago by Gert Döring

That sounds like a variation of the "if NCP succeeds, and then the server gets restarted with --ncp-disabled, the client will stick to the negotiated cipher which no longer works" bug - namely, the client will not properly reset its configuration environment on reconnecting.

We do not have a fix for this in the code yet, and I can't find a trac ticket for it - though I'm sure debbie10t has reported this previously (the other variant).

comment:4 Changed 3 years ago by Steffan Karger

Status: assignedaccepted

Plaithos and cron2 are correct. I'll look into it.

comment:5 Changed 3 years ago by mknutson

I'm also unable to find any Trac tickets related to this. This issue is appears to be distinct from ticket:887 which was an issue on the server side. In this case, the server pushes the same cipher both times, and the client rejects it the first time, but accepts it the second time, so this appears to be a client issue.

comment:6 Changed 3 years ago by mknutson

Just to be sure this isn't already fixed, I've also reproduced this behavior with the latest from the master branch.

Server
sudo ./openvpn --dev tun0 --server 10.8.0.0 255.255.255.0 --ca ca.crt --key server.key --cert server.crt --dh dh2048.pem --cipher BF-CBC --ncp-ciphers AES-256-CBC:AES-128-CBC --verb 3

Client
sudo ./openvpn --dev tun1 --client --remote 127.0.0.1 --nobind --ca ca.crt --key client.key --cert client.crt --cipher BF-CBC --ncp-ciphers BF-CBC --verb 3

Result
The client first rejects the pushed cipher with "Error: pushed cipher not allowed - AES-256-CBC not in BF-CBC or BF-CBC". After the client does a soft restart and pauses for 5 seconds, the client and server agree on AES-256-CBC and the connection is established.

Since AES-256-CBC is not included in the default or user-specified values for ncp-ciphers, the client must be getting this value from the server push.

comment:7 in reply to:  6 Changed 3 years ago by Gert Döring

Replying to mknutson:

Just to be sure this isn't already fixed, I've also reproduced this behavior with the latest from the master branch.

Thanks for testing. I already checked our git logs to see if we ended up having a patch committed - but we've only discussed it, nothing in the tree yet. (We might have an actual patch on the list which is pending review, but I've been too busy in the last few days to check - won't find time to do anything reasonable before Tuesday)

comment:8 Changed 3 years ago by Steffan Karger

So, looking at this again, our behaviour is slightly different for client configs without --persist-key. With --persist-key specified, everything is reset as expected, but without it isn't. Will investigate further...

(Edit: it wasn't --nobind, it was --persist-key.)

Last edited 3 years ago by Steffan Karger (previous) (diff)

Changed 3 years ago by mknutson

comment:9 Changed 3 years ago by mknutson

Nice find, syzzer!

Without really digging into the code, I'm having trouble imagining why adding --persist-key would preserve the cipher option.

Out of curiosity, I captured a log with verb=11 with and without --persist-key. However, when I sanitized the log files and compared them, I found very little to point me in the right direction. Basically, just a few extra pkcs11h_openssl_ex_data_free() calls, along with "PRNG init md=SHA1 size=36" instead of "Re-using SSL/TLS context". I've attached the relevant portion of the diff to this ticket for reference.

comment:10 Changed 3 years ago by Steffan Karger

Thanks for reporting back.

OpenVPN's internal restart logic is quite intricate. Persisting the keys results in part of the internal state, amongst which the options state, to persist over the restart. Since a client has just one options state, which is changed by pushed options, the client preserves the previously pushed options for some configurations.

I can think of several ways to resolve this particular bug, but find it hard what to pick. Two good candidates:

  1. Revert the pushed cipher option immediately after rejecting the pushed cipher. This fixes the issue at hand, and does not change other behaviour.
  1. Revert the entire options state on a connection reset, even when persist-key is used. This fixes this issue in a more generic way, but will also change client behaviour. The current behaviour is probably not on purpose (read: a bug), but users relying on the pushed options to persist over reconnections might be surprised that their config just stopped working.

Maybe do 1. for release/2.4, and 2. for master? cron2, what do you think?

Last edited 3 years ago by Steffan Karger (previous) (diff)

comment:11 in reply to:  10 Changed 3 years ago by Gert Döring

Replying to syzzer:

Maybe do 1. for release/2.4, and 2. for master? cron2, what do you think?

Awww, that particular rathole again... we have an open trac (quite a few years old) that has "on comp-lzo mismatch, it fails on the first connection attempt, but when reconnecting, it suddenly works", which very much sounds like the same thing.

I think we should reset the option state whenever something happens that will cause the client to send a new PUSH_REQUEST - especially since the server might decide to omit something it sent beforehand (due to changed server options), these should then be able to fall back to the client default, not to "what we had last round". cipher sounds like a candidate :-) - but also stuff like ping or compress.

So, for master / 2.5, "2" sounds the way forward.

For 2.4.x, I have mixed feelings - we've hit people with a number of new suprises already (CRL...), so maybe we really should be conservative here, and only fix cipher. So, "1".

In other words: with lots of words I'm agreeing with you.

comment:12 in reply to:  3 Changed 3 years ago by tincantech

Replying to cron2:

We do not have a fix for this in the code yet, and I can't find a trac ticket for it - though I'm sure debbie10t has reported this previously (the other variant).

Here: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg00113.html

comment:13 Changed 3 years ago by Steffan Karger

Just to be clear: 1. will fix this issue, but not tincantech's issue. 2. will resolve that too.

I already sent a patch to the -devel list for 1:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14984.html

Last edited 3 years ago by Steffan Karger (previous) (diff)

comment:14 Changed 3 years ago by Steffan Karger

Milestone: release 2.4.4
Resolution: fixed
Status: acceptedclosed

The patch has been applied to the master and release/2.4 branch.

commit 3be9a1c1cd75627c30dca05bed28c84ad4dc1d37 (master)
commit e050c762e2bd3695275ee675f197f062c63e2b6f (release/2.4)
Author: Steffan Karger
Date: Wed Jun 28 00:20:29 2017 +0200

Undo cipher push in client options state if cipher is rejected

This means that it will be part of the next release. I'm closing this issue, as this particular issue has been resolved. The refactoring part is basically already tracked by #128. And tincantech's issue needs a separate ticket (can you create one, if you haven't done that already, tincantech?).

comment:15 in reply to:  14 Changed 3 years ago by tincantech

Tying up loose ends:

Replying to syzzer:

And tincantech's issue needs a separate ticket

#915

Note: See TracTickets for help on using tickets.