diff mbox series

[Openvpn-devel] mbedTLS: Make sure TLS session survives move

Message ID 20200331071437.12708-1-tom.van.leeuwen@technolution.nl
State Accepted
Headers show
Series [Openvpn-devel] mbedTLS: Make sure TLS session survives move | expand

Commit Message

Tom van Leeuwen March 30, 2020, 8:14 p.m. UTC 
From: Tom van Leeuwen <tom.van.leeuwen@technolution.eu>

When a client disconnects from a server compiled with mbedTLS, the server
cannot process the PUSH_REQUEST from a new connection with the same client
IP and port number. This is the case when the client binds to a static port.

This behavior is initiated by move_session(), which copies the content of the
tls_session to a new session and re-initializes the old session once the new
session is authenticated.
This tls_session contains, among other things, an mbedtls_ssl_config and 
bio_ctx structure. However, the mbedtls context has internal pointers to the
mbedtls_ssl_config and bio_ctx. When the session is moved, these internal
pointers point to the reinitialized session and as a result all received
packets that are stored in the bio_ctx of the moved session can never be read
by the mbedtls session. The PUSH_REQUEST is therefore never seen by the server.

Since there is no public method to update these internal pointers, this
patch dynamically allocates the mbedtls_ssl_config and bio_ctx and stores
the pointers to those structures in the tls_session instead.

Trac #880

Signed-off-by: Tom van Leeuwen <tom.van.leeuwen@technolution.eu>

Comments

Arne Schwabe April 1, 2020, 12:04 a.m. UTC | #1 
Am 31.03.20 um 09:14 schrieb Tom van Leeuwen:
> From: Tom van Leeuwen <tom.van.leeuwen@technolution.eu>
> 
> When a client disconnects from a server compiled with mbedTLS, the server
> cannot process the PUSH_REQUEST from a new connection with the same client
> IP and port number. This is the case when the client binds to a static port.
> 
> This behavior is initiated by move_session(), which copies the content of the
> tls_session to a new session and re-initializes the old session once the new
> session is authenticated.
> This tls_session contains, among other things, an mbedtls_ssl_config and 
> bio_ctx structure. However, the mbedtls context has internal pointers to the
> mbedtls_ssl_config and bio_ctx. When the session is moved, these internal
> pointers point to the reinitialized session and as a result all received
> packets that are stored in the bio_ctx of the moved session can never be read
> by the mbedtls session. The PUSH_REQUEST is therefore never seen by the server.
> 
> Since there is no public method to update these internal pointers, this
> patch dynamically allocates the mbedtls_ssl_config and bio_ctx and stores
> the pointers to those structures in the tls_session instead.

I have not verified that this fix actually fixes the bug but the
explaination is solid. I however checked the code that handling of ctx
is correct after the patch. Also the problem of moving ctx and having
pointers to the wrong object is present before this patch and the patch
fixes that bug.

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering April 1, 2020, 1:07 a.m. UTC | #2 
Your patch has been applied to the master and release/2.4 branch (bugfix).

Cherrypicking to release/2.4 caused merge conflicts due to slightly
different *context* in two places (EKM patch in master), but these were
easily fixed because very obvious.  I've tested the resulting 2.4 on
linux with mbedtls 2.19.1 and it works correctly (compiles + t_client).

commit a59e0754afd37a606d96cf24cea771ace3467289 (master)
commit 0efbd8e98eb7adfbf7ee6591941be0ff3c406d57 (release/2.4)
Author: Tom van LeeuwenS
Date:   Tue Mar 31 09:14:37 2020 +0200

     mbedTLS: Make sure TLS session survives move

     Signed-off-by: Tom van Leeuwen <tom.van.leeuwen@technolution.eu>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20200331071437.12708-1-tom.van.leeuwen@technolution.nl>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19661.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 0f0b035b..4f194ad7 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -1036,21 +1036,22 @@  key_state_ssl_init(struct key_state_ssl *ks_ssl,
     CLEAR(*ks_ssl);
 
     /* Initialise SSL config */
-    mbedtls_ssl_config_init(&ks_ssl->ssl_config);
-    mbedtls_ssl_config_defaults(&ks_ssl->ssl_config, ssl_ctx->endpoint,
+    ALLOC_OBJ_CLEAR(ks_ssl->ssl_config, mbedtls_ssl_config);
+    mbedtls_ssl_config_init(ks_ssl->ssl_config);
+    mbedtls_ssl_config_defaults(ks_ssl->ssl_config, ssl_ctx->endpoint,
                                 MBEDTLS_SSL_TRANSPORT_STREAM, MBEDTLS_SSL_PRESET_DEFAULT);
 #ifdef MBEDTLS_DEBUG_C
     mbedtls_debug_set_threshold(3);
 #endif
-    mbedtls_ssl_conf_dbg(&ks_ssl->ssl_config, my_debug, NULL);
-    mbedtls_ssl_conf_rng(&ks_ssl->ssl_config, mbedtls_ctr_drbg_random,
+    mbedtls_ssl_conf_dbg(ks_ssl->ssl_config, my_debug, NULL);
+    mbedtls_ssl_conf_rng(ks_ssl->ssl_config, mbedtls_ctr_drbg_random,
                          rand_ctx_get());
 
-    mbedtls_ssl_conf_cert_profile(&ks_ssl->ssl_config, &ssl_ctx->cert_profile);
+    mbedtls_ssl_conf_cert_profile(ks_ssl->ssl_config, &ssl_ctx->cert_profile);
 
     if (ssl_ctx->allowed_ciphers)
     {
-        mbedtls_ssl_conf_ciphersuites(&ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
+        mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
     }
 
     /* Disable record splitting (for now).  OpenVPN assumes records are sent
@@ -1058,35 +1059,35 @@  key_state_ssl_init(struct key_state_ssl *ks_ssl,
      * testing.  Since OpenVPN is not susceptible to BEAST, we can just
      * disable record splitting as a quick fix. */
 #if defined(MBEDTLS_SSL_CBC_RECORD_SPLITTING)
-    mbedtls_ssl_conf_cbc_record_splitting(&ks_ssl->ssl_config,
+    mbedtls_ssl_conf_cbc_record_splitting(ks_ssl->ssl_config,
                                           MBEDTLS_SSL_CBC_RECORD_SPLITTING_DISABLED);
 #endif /* MBEDTLS_SSL_CBC_RECORD_SPLITTING */
 
     /* Initialise authentication information */
     if (is_server)
     {
-        mbed_ok(mbedtls_ssl_conf_dh_param_ctx(&ks_ssl->ssl_config,
+        mbed_ok(mbedtls_ssl_conf_dh_param_ctx(ks_ssl->ssl_config,
                                               ssl_ctx->dhm_ctx));
     }
 
-    mbed_ok(mbedtls_ssl_conf_own_cert(&ks_ssl->ssl_config, ssl_ctx->crt_chain,
+    mbed_ok(mbedtls_ssl_conf_own_cert(ks_ssl->ssl_config, ssl_ctx->crt_chain,
                                       ssl_ctx->priv_key));
 
     /* Initialise SSL verification */
 #if P2MP_SERVER
     if (session->opt->ssl_flags & SSLF_CLIENT_CERT_OPTIONAL)
     {
-        mbedtls_ssl_conf_authmode(&ks_ssl->ssl_config, MBEDTLS_SSL_VERIFY_OPTIONAL);
+        mbedtls_ssl_conf_authmode(ks_ssl->ssl_config, MBEDTLS_SSL_VERIFY_OPTIONAL);
     }
     else if (!(session->opt->ssl_flags & SSLF_CLIENT_CERT_NOT_REQUIRED))
 #endif
     {
-        mbedtls_ssl_conf_authmode(&ks_ssl->ssl_config, MBEDTLS_SSL_VERIFY_REQUIRED);
+        mbedtls_ssl_conf_authmode(ks_ssl->ssl_config, MBEDTLS_SSL_VERIFY_REQUIRED);
     }
-    mbedtls_ssl_conf_verify(&ks_ssl->ssl_config, verify_callback, session);
+    mbedtls_ssl_conf_verify(ks_ssl->ssl_config, verify_callback, session);
 
     /* TODO: mbed TLS does not currently support sending the CA chain to the client */
-    mbedtls_ssl_conf_ca_chain(&ks_ssl->ssl_config, ssl_ctx->ca_chain, ssl_ctx->crl);
+    mbedtls_ssl_conf_ca_chain(ks_ssl->ssl_config, ssl_ctx->ca_chain, ssl_ctx->crl);
 
     /* Initialize minimum TLS version */
     {
@@ -1103,7 +1104,7 @@  key_state_ssl_init(struct key_state_ssl *ks_ssl,
             tls_version_to_major_minor(tls_version_min, &major, &minor);
         }
 
-        mbedtls_ssl_conf_min_version(&ks_ssl->ssl_config, major, minor);
+        mbedtls_ssl_conf_min_version(ks_ssl->ssl_config, major, minor);
     }
 
     /* Initialize maximum TLS version */
@@ -1116,7 +1117,7 @@  key_state_ssl_init(struct key_state_ssl *ks_ssl,
         {
             int major, minor;
             tls_version_to_major_minor(tls_version_max, &major, &minor);
-            mbedtls_ssl_conf_max_version(&ks_ssl->ssl_config, major, minor);
+            mbedtls_ssl_conf_max_version(ks_ssl->ssl_config, major, minor);
         }
     }
 
@@ -1124,7 +1125,7 @@  key_state_ssl_init(struct key_state_ssl *ks_ssl,
     /* Initialize keying material exporter */
     if (session->opt->ekm_size)
     {
-        mbedtls_ssl_conf_export_keys_ext_cb(&ks_ssl->ssl_config,
+        mbedtls_ssl_conf_export_keys_ext_cb(ks_ssl->ssl_config,
                 mbedtls_ssl_export_keys_cb, session);
     }
 #endif
@@ -1132,11 +1133,11 @@  key_state_ssl_init(struct key_state_ssl *ks_ssl,
     /* Initialise SSL context */
     ALLOC_OBJ_CLEAR(ks_ssl->ctx, mbedtls_ssl_context);
     mbedtls_ssl_init(ks_ssl->ctx);
-    mbedtls_ssl_setup(ks_ssl->ctx, &ks_ssl->ssl_config);
+    mbedtls_ssl_setup(ks_ssl->ctx, ks_ssl->ssl_config);
 
     /* Initialise BIOs */
-    CLEAR(ks_ssl->bio_ctx);
-    mbedtls_ssl_set_bio(ks_ssl->ctx, &ks_ssl->bio_ctx, ssl_bio_write,
+    ALLOC_OBJ_CLEAR(ks_ssl->bio_ctx, bio_ctx);
+    mbedtls_ssl_set_bio(ks_ssl->ctx, ks_ssl->bio_ctx, ssl_bio_write,
                         ssl_bio_read, NULL);
 }
 
@@ -1152,9 +1153,17 @@  key_state_ssl_free(struct key_state_ssl *ks_ssl)
             mbedtls_ssl_free(ks_ssl->ctx);
             free(ks_ssl->ctx);
         }
-        mbedtls_ssl_config_free(&ks_ssl->ssl_config);
-        buf_free_entries(&ks_ssl->bio_ctx.in);
-        buf_free_entries(&ks_ssl->bio_ctx.out);
+        if (ks_ssl->ssl_config)
+        {
+            mbedtls_ssl_config_free(ks_ssl->ssl_config);
+            free(ks_ssl->ssl_config);
+        }
+        if (ks_ssl->bio_ctx)
+        {
+            buf_free_entries(&ks_ssl->bio_ctx->in);
+            buf_free_entries(&ks_ssl->bio_ctx->out);
+            free(ks_ssl->bio_ctx);
+        }
         CLEAR(*ks_ssl);
     }
 }
@@ -1249,7 +1258,7 @@  key_state_read_ciphertext(struct key_state_ssl *ks, struct buffer *buf,
         len = maxlen;
     }
 
-    retval = endless_buf_read(&ks->bio_ctx.out, BPTR(buf), len);
+    retval = endless_buf_read(&ks->bio_ctx->out, BPTR(buf), len);
 
     /* Error during read, check for retry error */
     if (retval < 0)
@@ -1294,7 +1303,7 @@  key_state_write_ciphertext(struct key_state_ssl *ks, struct buffer *buf)
         return 0;
     }
 
-    retval = endless_buf_write(&ks->bio_ctx.in, BPTR(buf), buf->len);
+    retval = endless_buf_write(&ks->bio_ctx->in, BPTR(buf), buf->len);
 
     if (retval < 0)
     {
diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
index c1c676dc..92381f1a 100644
--- a/src/openvpn/ssl_mbedtls.h
+++ b/src/openvpn/ssl_mbedtls.h
@@ -109,9 +109,9 @@  struct tls_root_ctx {
 };
 
 struct key_state_ssl {
-    mbedtls_ssl_config ssl_config;      /**< mbedTLS global ssl config */
+    mbedtls_ssl_config *ssl_config;     /**< mbedTLS global ssl config */
     mbedtls_ssl_context *ctx;           /**< mbedTLS connection context */
-    bio_ctx bio_ctx;
+    bio_ctx *bio_ctx;
 
     /** Keying material exporter cache (RFC 5705). */
     uint8_t *exported_key_material;