Opened 3 years ago

Closed 3 years ago

#887 closed Bug / Defect (fixed)

Cipher mismatch on reconnect after "TLS: soft reset"

Reported by: sbehrens Owned by:
Priority: major Milestone:
Component: Generic / unclassified Version: OpenVPN 2.4.1 (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords: cipher NCP reconnect PUSH_REPLY soft reset
Cc: Steffan Karger

Description

After a "TLS: soft reset" on the next reconnect the PUSH_REPLY does not include the "cipher AES-256-GCM" which would switch the client away from the default BF-CBC to AES-256-GCM. Client and server use mismatching ciphers afterwards. Configuring "reneg-sec 180" on the server helps to reproduce this issue quickly.

When tls_session_generate_data_channel_keys() is called via tls_session_update_crypto_params() the client and server both use the same cipher AES-256-GCM, but sometimes key_method_2_write() or key_method_2_read() seem to be the one who call tls_session_generate_data_channel_keys() and then the server uses AES-256-GCM but the client uses BF-CBC.

Example for when client + server both establish AES-256-GCM:

TLS: Username/Password authentication succeeded for username 'foo' [CN SET]
TLS: move_session: dest=TM_ACTIVE src=TM_UNTRUSTED reinit_src=1
TLS: tls_multi_process: untrusted session promoted to semi-trusted
Control Channel: TLSv1.2, cipher TLSv1/SSLv3 ECDHE-RSA-AES256-GCM-SHA384
PUSH: Received control message: 'PUSH_REQUEST'
SENT CONTROL [foo]: 'PUSH_REPLY,ping 45,route 3.2.1.69,route 3.2.1.72,route 3.2.1.44,route 10.144.0.1,topology net30,ifconfig 10.144.0.6 10.144.0.5,peer-id 0,cipher AES-256-GCM' (status=1)
Data Channel MTU parms [ L:1333 D:1333 EF:-167 EB:406 ET:0 EL:3 ]
Data Channel Encrypt: Cipher 'AES-256-GCM' initialized with 256 bit key
Data Channel Decrypt: Cipher 'AES-256-GCM' initialized with 256 bit key

Example for when client uses BF-CBC but server uses AES-256-GCM:

TLS: soft reset sec=0 bytes=23248/-1 pkts=237/0
TLS: Username/Password authentication succeeded for username 'foo' [CN SET]
Data Channel Encrypt: Cipher 'AES-256-GCM' initialized with 256 bit key
Data Channel Decrypt: Cipher 'AES-256-GCM' initialized with 256 bit key
TLS: move_session: dest=TM_ACTIVE src=TM_UNTRUSTED reinit_src=1
TLS: tls_multi_process: untrusted session promoted to semi-trusted
Control Channel: TLSv1.2, cipher TLSv1/SSLv3 ECDHE-RSA-AES256-GCM-SHA384
PUSH: Received control message: 'PUSH_REQUEST'
PUSH: client wants to negotiate cipher (NCP), but server has already generated data channel keys, ignoring client request
SENT CONTROL [foo]: 'PUSH_REPLY,ping 45,route 3.2.1.69,route 3.2.1.72,route 3.2.1.44,route 10.144.0.1,topology net30,ifconfig 10.144.0.6 10.144.0.5,peer-id 0' (status=1)
AEAD Decrypt error: cipher final failed

The server config is available in ticket:879#comment:2

Attachments (3)

openvpn-log.txt (17.2 KB) - added by sbehrens 3 years ago.
server log, kill -USR2 output, OS and --version output
0001-Fixup-push-message-after-TLS-soft-reset-for-NCP-case.patch (3.8 KB) - added by sbehrens 3 years ago.
Patch
0001-Fix-NCP-behaviour-on-TLS-reconnect.patch (3.0 KB) - added by Gert Döring 3 years ago.
more minimal patch, as discussed

Download all attachments as: .zip

Change History (16)

Changed 3 years ago by sbehrens

Attachment: openvpn-log.txt added

server log, kill -USR2 output, OS and --version output

comment:1 Changed 3 years ago by sbehrens

Looks like this is a duplicate to the 3rd issue described in ticket:886 (which in turn is a duplicate to the 2nd issue described in ticket:879).
What a perfect example why using a single ticket to submit multiple issues is not very helpful.

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

PUSH: client wants to negotiate cipher (NCP), but server has already generated data channel keys, ignoring client request

that's the crucial thing - we can only generate the keys once, because the key material is then cleared for good. So if keys have already been generated, we can not change crypto parameters = NCP disabled.

Why we're ending there with one side doing a full restart while the other end wants to re-use existing crypto is not something in my area of expertise - so it's good you cc:'ed syzzer already :-)

comment:3 Changed 3 years ago by sbehrens

I've sent the patch "[PATCH] Fixup push message after TLS soft reset for NCP case" to the mailing list which fixes this issue for me.

comment:4 Changed 3 years ago by sbehrens

From: openvpn-devel-owner@…
"You are not allowed to post to this mailing list, and your message has
been automatically rejected."

comment:5 Changed 3 years ago by Steffan Karger

Could that be because the address you're sending the mail from is not subscribed to the list?

Great to hear that you've found a solution though. Could you maybe attach the patch to this ticket?

comment:6 in reply to:  5 Changed 3 years ago by sbehrens

Replying to syzzer:

Could that be because the address you're sending the mail from is not subscribed to the list?

Yes, the subscribed address is a specific mailbox for this mailing list.

Great to hear that you've found a solution though. Could you maybe attach the patch to this ticket?

It's now attached as 0001-Fixup-push-message-after-TLS-soft-reset-for-NCP-case.patch.

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

Without being much involved in this ticket, the patch is close to what I was thinking - "if we have already run through NCP, and the client is asking again, just send the previous response again, and be done with it" :-)

Looking at your patch, it seems to be doing exactly this, in exactly the place which I was considering - that's a good sign.

OTOH, I find it somewhat complicated - without understanding the code around this well enough (syzzer is the one) - can we ever end up in a situation where encryption and decryption ciphers are different? Internally, we do keep different values, but config-wise, everything is "cipher <foo>", so I can't see a way to reach a difference (today).

Given that, the way you use to find the cipher names (translation and all that) might also be more elaborate than needed - on the initial reply, we stuff the NCP cipher in o->ciphername. If that value is kept across TLS reconnects (which I cannot answer) we could reduce the patch to "log a comment, and just send the already-set value once again".

Like this - fully untested, just to explain the idea:

--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -380,7 +380,9 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
         {
             msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
                  "server has already generated data channel keys, "
-                 "ignoring client request" );
+                 "re-sending previously negotiated cipher '%s'",
+                o->ciphername );
+            push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
         }
         else
         {
Last edited 3 years ago by Gert Döring (previous) (diff)

comment:8 Changed 3 years ago by sbehrens

This was my first thought too, to move the push_option_fmt() behind the if/else block, i.e., to alway send o->ciphername. And the PUSH_REQ looks good and contains the "cipher AES-256-GCM" after the soft reset and a client reconnect, the tests are successful.
The reason I made it so complicated is just that my understanding of the code is limited and I wanted to be on the safe side.

comment:9 Changed 3 years ago by Steffan Karger

Encrypt and decrypt ciphers are always the same. We just have two contexts, because we use different keys, replay counters, etc.

The patch that cron2 suggested matches with what I had in mind, but didn't get to actually writing and testing yet. So thank you both very much for picking this up!

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

@sbehrens: cool, thanks for testing and confirming that it works.

(I would not claim to understand the TLS related code, but I've been staring at the NCP stuff many rounds with syzzer, so the easy solution felt logical :-) )

Anyway - since you brought it up and diagnosed it, we need to give credits. Do you want to send a new patch (based on the simple one) or shall I just send a patch, refer to the trac ticket and put credits in the commit note?

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

Patch has been sent to the list, with credits :-) - this is mainly so syzzer can review and test. @sbehrens: if you want some different wording there, or send it yourself, all good with me - I just wanted to see this move.

From: Gert Doering <gert@…>
To: openvpn-devel@…
Subject: [PATCH] Fix NCP behaviour on TLS reconnect.
Message-Id: <20170518102246.5496-1-gert@…>

Changed 3 years ago by Gert Döring

more minimal patch, as discussed

comment:12 Changed 3 years ago by sbehrens

Looks good to me (the patch, the wordings and the credits). This ticket can be closed. Thanks!

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

Resolution: fixed
Status: newclosed

Fix is in tree, and will be in 2.4.3:

commit 5634cecf71ee9a92227bc9c8414c614d1b741abb (master)
commit 13c05ca4e9da88ef30a778c16a97f0c0d767b448 (release/2.4)
Author: Gert Doering
Date: Thu May 18 12:22:46 2017 +0200

Fix NCP behaviour on TLS reconnect.

Acked-by: Steffan Karger <steffan.karger@…>
Message-Id: <20170518102246.5496-1-gert@…>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg14666.html
Signed-off-by: Gert Doering <gert@…>

... thanks again!

Note: See TracTickets for help on using tickets.