| Message ID | 20210328120241.27605-2-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Delegated to: | Antonio Quartulli |
| Headers | show |
| Series | [Openvpn-devel,v2,1/2] Move context_auth from context_2 to tls_multi and name it multi_state | expand |
Hi, On 28/03/2021 14:02, Arne Schwabe wrote: > When OpenVPN sees a new (SSL) connection via HARD_RESET or SOFT_RESET with > the same port/ip as an existing session, it will give it the slot of the > renegotiation session (TM_UNTRUSTED). And when the authentication > succeeds it will replace the current session. In the case of a SOFT_RESET > this a renegotiation and we will generated data channel keys at the of > key_method_2_write function as key-id > 0. > > For a HARD RESET the key-id is 0. Since we already have gone through > connect stages and set context_auth to CAS_SUCCEEDED, we don't > call all the connect stages again, and therefore also never call > multi_client_generate_tls_keys for this session. > > This commit changes postponing the key generation to be done only if > the multi_connect has not yet been finished. > > Patch V2: Explain better in the commit message why this change is done. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> Thanks for the more exhaustive explanation. After a short discussion with Arne, I was able to easily replicate this bug and double check that this patch is indeed fixing it. Code looks good, except for one line that is not indented properly. See below: > --- > src/openvpn/ssl.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index 893e5753d..0da558392 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -2240,7 +2240,8 @@ error: > * to the TLS control channel (cleartext). > */ > static bool > -key_method_2_write(struct buffer *buf, struct tls_session *session) > +key_method_2_write(struct buffer *buf, struct tls_multi *multi, > + struct tls_session *session) > { > struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ > > @@ -2331,12 +2332,17 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) > goto error; > } > > - /* Generate tunnel keys if we're a TLS server. > - * If we're a p2mp server and IV_NCP >= 2 is negotiated, the first key > - * generation is postponed until after the pull/push, so we can process pushed > - * cipher directives. > + /* > + * Generate tunnel keys if we're a TLS server. > + * > + * If we're a p2mp server to allow NCP, the first key > + * generation is postponed until after the connect script finished and the > + * NCP options can be processed. Since that always happens at after connect > + * script options are available the CAS_SUCCEEDED status is identical to > + * NCP options are processed and we have no extra state for NCP finished. > */ > - if (session->opt->server && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0)) > + if (session->opt->server && (session->opt->mode != MODE_SERVER > + || multi->multi_state == CAS_SUCCEEDED)) Indentation is bogus here. > { > if (ks->authenticated > KS_AUTH_FALSE) > { > @@ -2826,7 +2832,7 @@ tls_process(struct tls_multi *multi, > if (!buf->len && ((ks->state == S_START && !session->opt->server) > || (ks->state == S_GOT_KEY && session->opt->server))) > { > - if (!key_method_2_write(buf, session)) > + if (!key_method_2_write(buf, multi, session)) > { > goto error; > } > Other than that: Acked-by: Antonio Quartulli <antonio@openvpn.net>
Hi, On Sun, Mar 28, 2021 at 02:02:41PM +0200, Arne Schwabe wrote: [..] > @@ -2240,7 +2240,8 @@ error: > * to the TLS control channel (cleartext). > */ > static bool > -key_method_2_write(struct buffer *buf, struct tls_session *session) > +key_method_2_write(struct buffer *buf, struct tls_multi *multi, > + struct tls_session *session) > { For master, this conflicts with commit 8fa8a17528c (compress migrate), which already introduces the "multi" parameter. Master only needs the second hunk (documentation plus CAS_SUCCEEDED check). For release/2.5, this should be fine. How do we want to proceed? Take this as "for 2.5", and you send a v3 "for master"? I have tested this patch for master, on top of "1666 with the ENABLE_ASYNC_PUSH fixed", and it passes the server side test rig... Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2g 2d 2e 2f 2z1 2z2 3 4 4a 4b 5 5a 5b 5c 5d 5v1 5v2 5v3 5w1 5w2 5w3 5w4 5x1 5x2 5x3 5x4 6 7 7x 7y 8 8a 9 9a. ... and also the "I have a client with --bind and renegotiate and then reconnect right after ctrl-c" test... so this looks ready to go. gert
Hi,
On Sun, Apr 18, 2021 at 12:11:27PM +0200, Gert Doering wrote:
> For release/2.5, this should be fine.
Confirming :-)
The patch 1/2 v2 (1666) needs some amount of force to go into 2.5 due
to context changes, and one extra is_cas_pending(). 2/2 v2 (1667) goes
right in, and succeeds
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2g 2d 2e 2f 2z1 2z2 3 4 4a 4b 5 5a 5b 5c 5d 5v1 5v2 5v3 5w1 5w2 5w3 5w4 5x1 5x2 5x3 5x4 6 7 7x 7y 8 8a 9 9a.
and also the "reconnect from same port" thing works with this patch on 2.5.
gert
Am 18.04.2021 um 12:11 schrieb Gert Doering: > Hi, > > On Sun, Mar 28, 2021 at 02:02:41PM +0200, Arne Schwabe wrote: > [..] >> @@ -2240,7 +2240,8 @@ error: >> * to the TLS control channel (cleartext). >> */ >> static bool >> -key_method_2_write(struct buffer *buf, struct tls_session *session) >> +key_method_2_write(struct buffer *buf, struct tls_multi *multi, >> + struct tls_session *session) >> { > For master, this conflicts with commit 8fa8a17528c (compress migrate), > which already introduces the "multi" parameter. Master only needs the > second hunk (documentation plus CAS_SUCCEEDED check). I can resend it if you want but this should be a trivial conflict. Maybe even 3way merge might fix it. If you don't count this as trivial conflict (i.e. does the same as the other patch) then I can also resend it. Arne
Hi, On Sun, Apr 18, 2021 at 01:48:53PM +0200, Arne Schwabe wrote: > > For master, this conflicts with commit 8fa8a17528c (compress migrate), > > which already introduces the "multi" parameter. Master only needs the > > second hunk (documentation plus CAS_SUCCEEDED check). > > I can resend it if you want but this should be a trivial conflict. Maybe > even 3way merge might fix it. If you don't count this as trivial > conflict (i.e. does the same as the other patch) then I can also resend it. It's a trivial conflict (basically, 2 of 3 hunks are "already in", though the first looks different due to line wrapping, so 3way can't autofix it). I basically just wanted to know how to proceed here :-) - so I'll proceed to merge this to 2.5 "as it is" and to master "only hunk 2". gert
Acked-by: Gert Doering <gert@greenie.muc.de> Adding my own ACK because it needed a bit of whack to go in, and due to my own testing. The patch "as it is" applies to release/2.5 just fine, but conflicts with master because 2 of 3 hunks are already in, in a slightly different formatting. So, master only received the middle hunk, and the 2.5 change was not done by cherrypicking but by directly applying the patch. Tested master and release/2.5 on the server torture test bed, including the "client connects, renegotiates, disconnects, and starts anew from the same source port" test, which would yield a broken session and now works fine. Your patch has been applied to the master and release/2.5 branch. I have added a reference to Trac #1316 to the commit message. commit a005044be9ca77ee8a47cb65a603d0b1c41b99f4 (master) commit 227fbc117d58a87465804f7b2a5cd95ef1b94da6 (release/2.5) Author: Arne Schwabe Date: Sun Mar 28 14:02:41 2021 +0200 Fix condition to generate session keys Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Antonio Quartulli <antonio@openvpn.net> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20210328120241.27605-2-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21873.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 893e5753d..0da558392 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -2240,7 +2240,8 @@ error: * to the TLS control channel (cleartext). */ static bool -key_method_2_write(struct buffer *buf, struct tls_session *session) +key_method_2_write(struct buffer *buf, struct tls_multi *multi, + struct tls_session *session) { struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ @@ -2331,12 +2332,17 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) goto error; } - /* Generate tunnel keys if we're a TLS server. - * If we're a p2mp server and IV_NCP >= 2 is negotiated, the first key - * generation is postponed until after the pull/push, so we can process pushed - * cipher directives. + /* + * Generate tunnel keys if we're a TLS server. + * + * If we're a p2mp server to allow NCP, the first key + * generation is postponed until after the connect script finished and the + * NCP options can be processed. Since that always happens at after connect + * script options are available the CAS_SUCCEEDED status is identical to + * NCP options are processed and we have no extra state for NCP finished. */ - if (session->opt->server && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0)) + if (session->opt->server && (session->opt->mode != MODE_SERVER + || multi->multi_state == CAS_SUCCEEDED)) { if (ks->authenticated > KS_AUTH_FALSE) { @@ -2826,7 +2832,7 @@ tls_process(struct tls_multi *multi, if (!buf->len && ((ks->state == S_START && !session->opt->server) || (ks->state == S_GOT_KEY && session->opt->server))) { - if (!key_method_2_write(buf, session)) + if (!key_method_2_write(buf, multi, session)) { goto error; }
When OpenVPN sees a new (SSL) connection via HARD_RESET or SOFT_RESET with the same port/ip as an existing session, it will give it the slot of the renegotiation session (TM_UNTRUSTED). And when the authentication succeeds it will replace the current session. In the case of a SOFT_RESET this a renegotiation and we will generated data channel keys at the of key_method_2_write function as key-id > 0. For a HARD RESET the key-id is 0. Since we already have gone through connect stages and set context_auth to CAS_SUCCEEDED, we don't call all the connect stages again, and therefore also never call multi_client_generate_tls_keys for this session. This commit changes postponing the key generation to be done only if the multi_connect has not yet been finished. Patch V2: Explain better in the commit message why this change is done. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/ssl.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)