| Message ID | 20220909195902.2011798-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Delegated to: | Heiko Hund |
| Headers | show |
| Series | [Openvpn-devel,1/3] Allows renegotiation only to start if session is fully established | expand |
On Freitag, 9. September 2022 21:59:00 CEST Arne Schwabe wrote: > This change makes the state machine more strict in terms of transation *transitions > Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Heiko Hund <heiko@ist.eigentlich.net> For those who wonder what this is/does, my take on it: basically shields the calls to key_state_soft_reset() on both the sending and receiving side, and ensures both ends have completed the control channel negotiation fully (i.e. are in state S_GENERATED_KEYS). At that time pushed options are guaranteed to be processed/active, which was the main motivation for this change, as I understand it.
As discussed on IRC, I have adjusted the commit message to make *me*
understand what this is about, so maybe other readers find it easier
too :-)
Tested on the server test bench, with pre-NCP p2p client and regular
p2mp clients, and renegotiating often (auth-token test bench). No
issues observed. (So, technically, I have not tested "the behavioural
change", just verified that nothing breaks in - borderline - normal
environments)
Your patch has been applied to the master branch.
commit f786e8a41cde31b437f20a17cb3fd122b489d6b3
Author: Arne Schwabe
Date: Fri Sep 9 21:59:00 2022 +0200
Allows renegotiation only to start if session is fully established
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Heiko Hund <heiko@ist.eigentlich.net>
Message-Id: <20220909195902.2011798-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25162.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 002871288..36a236fe3 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -3011,7 +3011,7 @@ tls_process(struct tls_multi *multi, ASSERT(session_id_defined(&session->session_id)); /* Should we trigger a soft reset? -- new key, keeps old key for a while */ - if (ks->state >= S_ACTIVE + if (ks->state >= S_GENERATED_KEYS && ((session->opt->renegotiate_seconds && now >= ks->established + session->opt->renegotiate_seconds) || (session->opt->renegotiate_bytes > 0 @@ -3733,9 +3733,11 @@ tls_pre_decrypt(struct tls_multi *multi, } /* - * Remote is requesting a key renegotiation + * Remote is requesting a key renegotiation. We only allow renegotiation + * when the previous session is fully established to avoid weird corner + * cases. */ - if (op == P_CONTROL_SOFT_RESET_V1 && TLS_AUTHENTICATED(multi, ks)) + if (op == P_CONTROL_SOFT_RESET_V1 && ks->state >= S_GENERATED_KEYS) { if (!read_control_auth(buf, &session->tls_wrap, from, session->opt))
This change makes the state machine more strict in terms of transation that are allowed. The benefit of this change are two: - allows any option that might be pushed to affect renegotiation consistently This is a prerequisite for the upcoming secure renegotiation patch set - avoids corner cases of a peer (or an attacker) trying to renegotiate the session while the original session is not fully setup. Currently there there are no problems known with this but it is better to avoid the corner case in the first time. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/ssl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)