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

Message ID 20201215164221.5995-2-arne@rfc2549.org
State Superseded
Headers show
Series
  • [Openvpn-devel,1/2] Move context_auth from context_2 to tls_multi
Related show

Commit Message

Arne Schwabe Dec. 15, 2020, 4:42 p.m.
For --nobind clients OpenVPN reuses the context and tls_multi structs
of the previous clients and does not rerun the connect scripts on
connect. But since it is a new client connection, the key_id is 0 and
we postpone the key generation but it will never happen.

This commit changes postponing the key generation to the right
condition of NCP done for this session.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Comments

Antonio Quartulli March 25, 2021, 10:37 p.m. | #1
Hi,

On 15/12/2020 17:42, Arne Schwabe wrote:
> For --nobind clients OpenVPN reuses the context and tls_multi structs
> of the previous clients and does not rerun the connect scripts on
> connect. But since it is a new client connection, the key_id is 0 and
> we postpone the key generation but it will never happen.

Can you explain how the --nobind on the client is related to the server
behaviour?

Are you saying that a client connecting from the same IP of another
client will share its session and tls_multi object?
Arne Schwabe March 26, 2021, 1:10 p.m. | #2
Am 25.03.21 um 23:37 schrieb Antonio Quartulli:
> Hi,
> 
> On 15/12/2020 17:42, Arne Schwabe wrote:
>> For --nobind clients OpenVPN reuses the context and tls_multi structs
>> of the previous clients and does not rerun the connect scripts on
>> connect. But since it is a new client connection, the key_id is 0 and
>> we postpone the key generation but it will never happen.
> 
> Can you explain how the --nobind on the client is related to the server
> behaviour?
> 
> Are you saying that a client connecting from the same IP of another
> client will share its session and tls_multi object?
(I will also copy that explanation to a v2 of the patch )

When OpenVPN sees a new (SSL) connection via HARD 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. 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.

Arne
Antonio Quartulli March 28, 2021, 1:03 a.m. | #3
Hi,

On 26/03/2021 14:10, Arne Schwabe wrote:
> Am 25.03.21 um 23:37 schrieb Antonio Quartulli:
>> Hi,
>>
>> On 15/12/2020 17:42, Arne Schwabe wrote:
>>> For --nobind clients OpenVPN reuses the context and tls_multi structs
>>> of the previous clients and does not rerun the connect scripts on
>>> connect. But since it is a new client connection, the key_id is 0 and
>>> we postpone the key generation but it will never happen.
>>
>> Can you explain how the --nobind on the client is related to the server
>> behaviour?
>>
>> Are you saying that a client connecting from the same IP of another
>> client will share its session and tls_multi object?
> (I will also copy that explanation to a v2 of the patch )
> 
> When OpenVPN sees a new (SSL) connection via HARD 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. 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.

Thanks for explaining this further.
I agree that adding some more text to the commit message would be
beneficial.

Other than that the patch looks logically correct to me.

Basically context_auth is set CAS_SUCCEEDED upon first connection cycle,
therefore a new connection that re-uses the existing instance will
already have context_auth set to CAS_SUCCEEDED and a new session key
will be generated.

I wonder if the connection can really be fully working if the server
believes that the client was already setup and does not performs the
usual connection steps. But this is another story, unrelated to this
patch...

This said, the new if condition does not look being properly aligned.

Cheers,
Arne Schwabe March 28, 2021, 6:56 a.m. | #4
> 
> Basically context_auth is set CAS_SUCCEEDED upon first connection cycle,
> therefore a new connection that re-uses the existing instance will
> already have context_auth set to CAS_SUCCEEDED and a new session key
> will be generated.
> 
> I wonder if the connection can really be fully working if the server
> believes that the client was already setup and does not performs the
> usual connection steps. But this is another story, unrelated to this
> patch...

It works. From the server perspective this new connection is almost the
same as a renegotiation. On a PUSH_REQUEST the server will just send the
same configuration as before. Basically if the new client (which is more
than often the same client) can do everything the previous client did,
everything is well.

Arne

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index efbf688f..5d322598 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2351,7 +2351,7 @@  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 */
 
@@ -2442,12 +2442,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->context_auth == CAS_SUCCEEDED))
     {
         if (ks->authenticated > KS_AUTH_FALSE)
         {
@@ -2936,7 +2941,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;
             }