diff mbox series

[Openvpn-devel,v2,2/2] Fix condition to generate session keys

Message ID 20210328120241.27605-2-arne@rfc2549.org
State Accepted
Delegated to: Antonio Quartulli
Headers
Series [Openvpn-devel,v2,1/2] Move context_auth from context_2 to tls_multi and name it multi_state |

Commit Message

Arne Schwabe March 28, 2021, 1:02 a.m. UTC 
  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(-)

Comments

Antonio Quartulli April 3, 2021, 2:30 a.m. UTC | #1 
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>
Gert Doering April 18, 2021, 12:11 a.m. UTC | #2 
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
Gert Doering April 18, 2021, 1:29 a.m. UTC | #3 
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
Arne Schwabe April 18, 2021, 1:48 a.m. UTC | #4 
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
Gert Doering April 18, 2021, 3:33 a.m. UTC | #5 
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
Gert Doering April 20, 2021, 12:28 a.m. UTC | #6 
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 mbox series

Patch

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;
             }