Cipher mismatch on reconnect after "TLS: soft reset"

[sbehrens]

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

[sbehrens]

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

[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.

[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 :-)

[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.

[sbehrens]

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

[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?

[sbehrens]

Patch

[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.

[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". Loke 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
         {

[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.

[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!

[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?

[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@…>

[Gert Döring]

more minimal patch, as discussed

[sbehrens]

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

[Gert Döring]

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!