From 03af7f6ae7f000d91035c98e05ed3297b503c627 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Thu, 26 Apr 2018 13:03:29 +0200
Subject: [PATCH 01/21] Change boolean bitfield to unsigned

Reminder to self: 1 is not a valid value in a 1-bit bitfield. It's
undefined behavior and gcc -ansi -pedantic helpfully complains about it.
---
 include/mbedtls/ssl_internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h
index 506aff395..bbaf3564f 100644
--- a/include/mbedtls/ssl_internal.h
+++ b/include/mbedtls/ssl_internal.h
@@ -310,7 +310,7 @@ struct mbedtls_ssl_handshake_params
 #endif
 
 #if defined(MBEDTLS_SSL_ASYNC_PRIVATE)
-    int async_in_progress : 1;          /*!< an asynchronous operation is in progress */
+    unsigned int async_in_progress : 1; /*!< an asynchronous operation is in progress */
 #endif /* MBEDTLS_SSL_ASYNC_PRIVATE */
 
 #if defined(MBEDTLS_SSL_ASYNC_PRIVATE)

From 6331d786752ea3b2b98bbd98bda4b7786fc2f27f Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Thu, 26 Apr 2018 13:27:43 +0200
Subject: [PATCH 02/21] Don't use the printf format %zd

We target C89 libc, so don't use %zd or %zu. Just use %u, and make
slot numbers `unsigned` for simplicity.
---
 programs/ssl/ssl_server2.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index 272eecdc5..9a226e4e4 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -933,7 +933,7 @@ static const char *const ssl_async_operation_names[] =
 
 typedef struct
 {
-    size_t slot;
+    unsigned slot;
     ssl_async_operation_type_t operation_type;
     mbedtls_md_type_t md_alg;
     unsigned char input[SSL_ASYNC_INPUT_MAX_SIZE];
@@ -950,7 +950,7 @@ static int ssl_async_start( mbedtls_ssl_context *ssl,
 {
     ssl_async_key_context_t *config_data =
         mbedtls_ssl_conf_get_async_config_data( ssl->conf );
-    size_t slot;
+    unsigned slot;
     ssl_async_operation_context_t *ctx = NULL;
     const char *op_name = ssl_async_operation_names[op_type];
 
@@ -971,7 +971,7 @@ static int ssl_async_start( mbedtls_ssl_context *ssl,
                         op_name );
         return( MBEDTLS_ERR_SSL_HW_ACCEL_FALLTHROUGH );
     }
-    mbedtls_printf( "Async %s callback: using key slot %zd, delay=%u.\n",
+    mbedtls_printf( "Async %s callback: using key slot %u, delay=%u.\n",
                     op_name, slot, config_data->slots[slot].delay );
 
     if( config_data->inject_error == SSL_ASYNC_INJECT_ERROR_START )
@@ -1036,7 +1036,7 @@ static int ssl_async_resume( mbedtls_ssl_context *ssl,
     if( ctx->remaining_delay > 0 )
     {
         --ctx->remaining_delay;
-        mbedtls_printf( "Async resume (slot %zd): call %u more times.\n",
+        mbedtls_printf( "Async resume (slot %u): call %u more times.\n",
                         ctx->slot, ctx->remaining_delay );
         return( MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS );
     }
@@ -1059,7 +1059,7 @@ static int ssl_async_resume( mbedtls_ssl_context *ssl,
                                    config_data->f_rng, config_data->p_rng );
             break;
         default:
-            mbedtls_printf( "Async resume (slot %zd): unknown operation type %ld. This shouldn't happen.\n",
+            mbedtls_printf( "Async resume (slot %u): unknown operation type %ld. This shouldn't happen.\n",
                             ctx->slot, (long) ctx->operation_type );
             return( MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE );
             break;
@@ -1072,7 +1072,7 @@ static int ssl_async_resume( mbedtls_ssl_context *ssl,
         return( MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE );
     }
 
-    mbedtls_printf( "Async resume (slot %zd): %s done, status=%d.\n",
+    mbedtls_printf( "Async resume (slot %u): %s done, status=%d.\n",
                     ctx->slot, op_name, ret );
     mbedtls_free( ctx );
     return( ret );

From 4d9ec4dcf77a15c1da64b55f7f70288ebcce7581 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Thu, 26 Apr 2018 14:33:43 +0200
Subject: [PATCH 03/21] Fix uninitialized variable in ssl_server2

---
 programs/ssl/ssl_server2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index 9a226e4e4..bf50f1d50 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -1223,6 +1223,9 @@ int main( int argc, char *argv[] )
     mbedtls_pk_init( &pkey );
     mbedtls_x509_crt_init( &srvcert2 );
     mbedtls_pk_init( &pkey2 );
+#if defined(MBEDTLS_SSL_ASYNC_PRIVATE)
+    memset( &ssl_async_keys, 0, sizeof( ssl_async_keys ) );
+#endif
 #endif
 #if defined(MBEDTLS_DHM_C) && defined(MBEDTLS_FS_IO)
     mbedtls_dhm_init( &dhm );

From 37d417561d855f81f5ff15b3d71b9757a50f5f65 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Thu, 26 Apr 2018 15:06:56 +0200
Subject: [PATCH 04/21] Add test case for SSL async resume after resume

Add a test case for SSL asynchronous signature where f_async_resume is
called twice. Verify that f_async_sign_start is only called once.

This serves as a non-regression test for a bug where f_async_sign_start
was only called once, which turned out to be due to a stale build
artifacts with mismatched numerical values of
MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS.
---
 tests/ssl-opt.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh
index 6261225b2..bf7d914b2 100755
--- a/tests/ssl-opt.sh
+++ b/tests/ssl-opt.sh
@@ -4088,6 +4088,18 @@ run_test    "SSL async private: sign, delay=1" \
             -s "Async resume (slot [0-9]): call 0 more times." \
             -s "Async resume (slot [0-9]): sign done, status=0"
 
+requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
+run_test    "SSL async private: sign, delay=2" \
+            "$P_SRV \
+             async_operations=s async_private_delay1=2 async_private_delay2=2" \
+            "$P_CLI" \
+            0 \
+            -s "Async sign callback: using key slot " \
+            -U "Async sign callback: using key slot " \
+            -s "Async resume (slot [0-9]): call 1 more times." \
+            -s "Async resume (slot [0-9]): call 0 more times." \
+            -s "Async resume (slot [0-9]): sign done, status=0"
+
 # Test that the async callback correctly signs the 36-byte hash of TLS 1.0/1.1
 # with RSA PKCS#1v1.5 as used in TLS 1.0/1.1.
 requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE

From 94e153af775e2531476a89a9bbc9e02da3c4500f Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Thu, 26 Apr 2018 17:57:37 +0200
Subject: [PATCH 05/21] Improve documentation of the async callback's crypto
 parameters

---
 include/mbedtls/ssl.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
index 84bc63ba5..097b86a3c 100644
--- a/include/mbedtls/ssl.h
+++ b/include/mbedtls/ssl.h
@@ -594,9 +594,16 @@ typedef struct mbedtls_ssl_flight_item mbedtls_ssl_flight_item;
  *                    from step 2, with `digestAlgorithm` obtained by calling
  *                    mbedtls_oid_get_oid_by_md() on \p md_alg.
  *
+ * \note            For ECDSA signatures, the output format is the DER encoding
+ *                  `Ecdsa-Sig-Value` defined in
+ *                  [RFC 4492 section 5.4](https://tools.ietf.org/html/rfc4492#section-5.4).
+ *
  * \param ssl             The SSL connection instance. It should not be
  *                        modified other than via mbedtls_ssl_async_set_data().
  * \param cert            Certificate containing the public key.
+ *                        This is one of the pointers passed to
+ *                        mbedtls_ssl_conf_own_cert() when configuring the SSL
+ *                        connection.
  * \param md_alg          Hash algorithm.
  * \param hash            Buffer containing the hash. This buffer is
  *                        no longer valid when the function returns.
@@ -646,9 +653,21 @@ typedef int mbedtls_ssl_async_sign_t( mbedtls_ssl_context *ssl,
  *                  store an operation context for later retrieval
  *                  by the resume callback.
  *
+ * \warning         RSA decryption as used in TLS is subject to a potential
+ *                  timing side channel attack first discovered by Bleichenbacher
+ *                  in 1998. This attack can be remotely exploitable
+ *                  in practice. To avoid this attack, you must ensure that
+ *                  if the callback performs an RSA decryption, the time it
+ *                  takes to execute and return the result does not depend
+ *                  on whether the RSA decryption succeeded or reported
+ *                  invalid padding.
+ *
  * \param ssl             The SSL connection instance. It should not be
  *                        modified other than via mbedtls_ssl_async_set_data().
  * \param cert            Certificate containing the public key.
+ *                        This is one of the pointers passed to
+ *                        mbedtls_ssl_conf_own_cert() when configuring the SSL
+ *                        connection.
  * \param input           Buffer containing the input ciphertext. This buffer
  *                        is no longer valid when the function returns.
  * \param input_len       Size of the \p input buffer in bytes.

From 20e2bdf4b03e88055dc32f93cdcc99ac589e70fd Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 27 Apr 2018 11:50:14 +0200
Subject: [PATCH 06/21] SSL async tests: tighten a few log checks in some test
 cases

---
 tests/ssl-opt.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh
index bf7d914b2..cf2c16875 100755
--- a/tests/ssl-opt.sh
+++ b/tests/ssl-opt.sh
@@ -4237,6 +4237,7 @@ run_test    "SSL async private: error in start" \
             1 \
             -s "Async sign callback: injected error" \
             -S "Async resume" \
+            -S "Async cancel" \
             -s "! mbedtls_ssl_handshake returned"
 
 requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
@@ -4259,6 +4260,7 @@ run_test    "SSL async private: error in resume" \
             1 \
             -s "Async sign callback: using key slot " \
             -s "Async resume callback: sign done but injected error" \
+            -S "Async cancel" \
             -s "! mbedtls_ssl_handshake returned"
 
 requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
@@ -4295,6 +4297,7 @@ run_test    "SSL async private: cancel after start then fall back to transparent
              [ \$? -eq 1 ] &&
              $P_CLI force_ciphersuite=TLS-ECDHE-RSA-WITH-AES-128-CBC-SHA256" \
             0 \
+            -s "Async sign callback: using key slot 0"
             -S "Async resume" \
             -s "Async cancel" \
             -s "! mbedtls_ssl_handshake returned" \

From 07981585d3146f01f7a71d80a7cde2b52a4ef1d7 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Mon, 30 Apr 2018 10:02:45 +0200
Subject: [PATCH 07/21] Fix missing continuation indicator in ssl-opt.sh

---
 tests/ssl-opt.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh
index cf2c16875..597a5f1c1 100755
--- a/tests/ssl-opt.sh
+++ b/tests/ssl-opt.sh
@@ -4297,7 +4297,7 @@ run_test    "SSL async private: cancel after start then fall back to transparent
              [ \$? -eq 1 ] &&
              $P_CLI force_ciphersuite=TLS-ECDHE-RSA-WITH-AES-128-CBC-SHA256" \
             0 \
-            -s "Async sign callback: using key slot 0"
+            -s "Async sign callback: using key slot 0" \
             -S "Async resume" \
             -s "Async cancel" \
             -s "! mbedtls_ssl_handshake returned" \

From d6fbfde994f7cc3a72078c328160bde669131a82 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Mon, 30 Apr 2018 10:23:56 +0200
Subject: [PATCH 08/21] ssl_async_set_key: detect if ctx->slots overflows

---
 programs/ssl/ssl_server2.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index bf50f1d50..838f41d7c 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -904,15 +904,18 @@ typedef struct
     void *p_rng;
 } ssl_async_key_context_t;
 
-void ssl_async_set_key( ssl_async_key_context_t *ctx,
+int ssl_async_set_key( ssl_async_key_context_t *ctx,
                         mbedtls_x509_crt *cert,
                         mbedtls_pk_context *pk,
                         unsigned delay )
 {
+    if( ctx->slots_used >= sizeof( ctx->slots ) / sizeof( *ctx->slots ) )
+        return( -1 );
     ctx->slots[ctx->slots_used].cert = cert;
     ctx->slots[ctx->slots_used].pk = pk;
     ctx->slots[ctx->slots_used].delay = delay;
     ++ctx->slots_used;
+    return( 0 );
 }
 
 #define SSL_ASYNC_INPUT_MAX_SIZE 512
@@ -2297,8 +2300,14 @@ int main( int argc, char *argv[] )
 #if defined(MBEDTLS_SSL_ASYNC_PRIVATE)
         if( opt.async_private_delay1 >= 0 )
         {
-            ssl_async_set_key( &ssl_async_keys, &srvcert, pk,
-                               opt.async_private_delay1 );
+            ret = ssl_async_set_key( &ssl_async_keys, &srvcert, pk,
+                                     opt.async_private_delay1 );
+            if( ret < 0 )
+            {
+                mbedtls_printf( "  Test error: ssl_async_set_key failed (%d)\n",
+                                ret );
+                goto exit;
+            }
             pk = NULL;
         }
 #endif /* MBEDTLS_SSL_ASYNC_PRIVATE */
@@ -2314,8 +2323,14 @@ int main( int argc, char *argv[] )
 #if defined(MBEDTLS_SSL_ASYNC_PRIVATE)
         if( opt.async_private_delay2 >= 0 )
         {
-            ssl_async_set_key( &ssl_async_keys, &srvcert2, pk,
-                               opt.async_private_delay2 );
+            ret = ssl_async_set_key( &ssl_async_keys, &srvcert2, pk,
+                                     opt.async_private_delay2 );
+            if( ret < 0 )
+            {
+                mbedtls_printf( "  Test error: ssl_async_set_key failed (%d)\n",
+                                ret );
+                goto exit;
+            }
             pk = NULL;
         }
 #endif /* MBEDTLS_SSL_ASYNC_PRIVATE */

From 166ce748cfbc1f22f181a381dc32df4d1e0ea8cd Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Mon, 30 Apr 2018 10:30:49 +0200
Subject: [PATCH 09/21] SSL async callback: cert is not always from
 mbedtls_ssl_conf_own_cert

The certificate passed to async callbacks may not be the one set by
mbedtls_ssl_conf_own_cert. For example, when using an SNI callback,
it's whatever the callback is using. Document this, and add a test
case (and code sample) with SNI.
---
 include/mbedtls/ssl.h      | 14 ++++++++++----
 programs/ssl/ssl_server2.c | 25 ++++++++++++++++++++++---
 tests/ssl-opt.sh           | 14 ++++++++++++++
 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
index 097b86a3c..b199e2ea6 100644
--- a/include/mbedtls/ssl.h
+++ b/include/mbedtls/ssl.h
@@ -601,9 +601,12 @@ typedef struct mbedtls_ssl_flight_item mbedtls_ssl_flight_item;
  * \param ssl             The SSL connection instance. It should not be
  *                        modified other than via mbedtls_ssl_async_set_data().
  * \param cert            Certificate containing the public key.
- *                        This is one of the pointers passed to
+ *                        In simple cases, this is one of the pointers passed to
  *                        mbedtls_ssl_conf_own_cert() when configuring the SSL
- *                        connection.
+ *                        connection. However, if other callbacks are used, this
+ *                        property may not hold. For example, if an SNI callback
+ *                        is registered with mbedtls_ssl_conf_sni(), then
+ *                        this callback determines what certificate is used.
  * \param md_alg          Hash algorithm.
  * \param hash            Buffer containing the hash. This buffer is
  *                        no longer valid when the function returns.
@@ -665,9 +668,12 @@ typedef int mbedtls_ssl_async_sign_t( mbedtls_ssl_context *ssl,
  * \param ssl             The SSL connection instance. It should not be
  *                        modified other than via mbedtls_ssl_async_set_data().
  * \param cert            Certificate containing the public key.
- *                        This is one of the pointers passed to
+ *                        In simple cases, this is one of the pointers passed to
  *                        mbedtls_ssl_conf_own_cert() when configuring the SSL
- *                        connection.
+ *                        connection. However, if other callbacks are used, this
+ *                        property may not hold. For example, if an SNI callback
+ *                        is registered with mbedtls_ssl_conf_sni(), then
+ *                        this callback determines what certificate is used.
  * \param input           Buffer containing the input ciphertext. This buffer
  *                        is no longer valid when the function returns.
  * \param input_len       Size of the \p input buffer in bytes.
diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index 838f41d7c..b1f2382cb 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -204,7 +204,7 @@ int main( void )
 #define USAGE_SSL_ASYNC \
     "    async_operations=%%c...   d=decrypt, s=sign (default: -=off)\n" \
     "    async_private_delay1=%%d  Asynchronous delay for key_file or preloaded key\n" \
-    "    async_private_delay2=%%d  Asynchronous delay for key_file2\n" \
+    "    async_private_delay2=%%d  Asynchronous delay for key_file2 and sni\n" \
     "                              default: -1 (not asynchronous)\n" \
     "    async_private_error=%%d   Async callback error injection (default=0=none,\n" \
     "                              1=start, 2=cancel, 3=resume, negative=first time only)"
@@ -897,7 +897,7 @@ typedef enum {
 
 typedef struct
 {
-    ssl_async_key_slot_t slots[2];
+    ssl_async_key_slot_t slots[3]; /* key, key2, sni */
     size_t slots_used;
     ssl_async_inject_error_t inject_error;
     int (*f_rng)(void *, unsigned char *, size_t);
@@ -965,7 +965,9 @@ static int ssl_async_start( mbedtls_ssl_context *ssl,
 
     for( slot = 0; slot < config_data->slots_used; slot++ )
     {
-        if( config_data->slots[slot].cert == cert )
+        if( memcmp( &config_data->slots[slot].cert->pk,
+                    &cert->pk,
+                    sizeof( cert->pk ) ) == 0 )
             break;
     }
     if( slot == config_data->slots_used )
@@ -2376,7 +2378,24 @@ int main( int argc, char *argv[] )
 
 #if defined(SNI_OPTION)
     if( opt.sni != NULL )
+    {
         mbedtls_ssl_conf_sni( &conf, sni_callback, sni_info );
+#if defined(MBEDTLS_SSL_ASYNC_PRIVATE)
+        if( opt.async_private_delay2 >= 0 )
+        {
+            ret = ssl_async_set_key( &ssl_async_keys,
+                                     sni_info->cert, sni_info->key,
+                                     opt.async_private_delay2 );
+            if( ret < 0 )
+            {
+                mbedtls_printf( "  Test error: ssl_async_set_key failed (%d)\n",
+                                ret );
+                goto exit;
+            }
+            sni_info->key = NULL;
+        }
+#endif /* MBEDTLS_SSL_ASYNC_PRIVATE */
+    }
 #endif
 
 #if defined(MBEDTLS_ECP_C)
diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh
index 597a5f1c1..6afca2d12 100755
--- a/tests/ssl-opt.sh
+++ b/tests/ssl-opt.sh
@@ -4112,6 +4112,20 @@ run_test    "SSL async private: sign, RSA, TLS 1.1" \
             -s "Async sign callback: using key slot " \
             -s "Async resume (slot [0-9]): sign done, status=0"
 
+requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
+run_test    "SSL async private: sign, SNI" \
+            "$P_SRV debug_level=3 \
+             async_operations=s async_private_delay1=0 async_private_delay2=0 \
+             crt_file=data_files/server5.crt key_file=data_files/server5.key \
+             sni=localhost,data_files/server2.crt,data_files/server2.key,-,-,-,polarssl.example,data_files/server1-nospace.crt,data_files/server1.key,-,-,-" \
+            "$P_CLI server_name=polarssl.example" \
+            0 \
+            -s "Async sign callback: using key slot " \
+            -s "Async resume (slot [0-9]): sign done, status=0" \
+            -s "parse ServerName extension" \
+            -c "issuer name *: C=NL, O=PolarSSL, CN=PolarSSL Test CA" \
+            -c "subject name *: C=NL, O=PolarSSL, CN=polarssl.example"
+
 requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
 run_test    "SSL async private: decrypt, delay=0" \
             "$P_SRV \

From 02b86d0415b086dce83ca65d7f93203c11be803f Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Mon, 30 Apr 2018 11:54:14 +0200
Subject: [PATCH 10/21] Fix copypasta in the async callback documentation

---
 include/mbedtls/ssl.h | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
index b199e2ea6..b7dc98b5b 100644
--- a/include/mbedtls/ssl.h
+++ b/include/mbedtls/ssl.h
@@ -568,9 +568,8 @@ typedef struct mbedtls_ssl_flight_item mbedtls_ssl_flight_item;
  *                  does not wait for the operation to complete. This allows
  *                  the handshake step to be non-blocking.
  *
- *                  The parameters \p ssl and \p cert are
- *                  guaranteed to remain valid as long as the SSL
- *                  configuration remains valid. On the other hand, this
+ *                  The parameters \p ssl and \p cert are guaranteed to remain
+ *                  valid throughout the handshake. On the other hand, this
  *                  function must save the contents of \p hash if the value
  *                  is needed for later processing, because the \p hash buffer
  *                  is no longer valid after this function returns.
@@ -588,7 +587,7 @@ typedef struct mbedtls_ssl_flight_item mbedtls_ssl_flight_item;
  *                    encoding, treating \p hash as the DigestInfo to be
  *                    padded. In other words, apply EMSA-PKCS1-v1_5 starting
  *                    from step 3, with `T = hash` and `tLen = hash_len`.
- *                  - If \p md_alg is #MBEDTLS_MD_NONE, apply the PKCS#1 v1.5
+ *                  - If `md_alg != MBEDTLS_MD_NONE`, apply the PKCS#1 v1.5
  *                    encoding, treating \p hash as the hash to be encoded and
  *                    padded. In other words, apply EMSA-PKCS1-v1_5 starting
  *                    from step 2, with `digestAlgorithm` obtained by calling
@@ -645,9 +644,8 @@ typedef int mbedtls_ssl_async_sign_t( mbedtls_ssl_context *ssl,
  *                  does not wait for the operation to complete. This allows
  *                  the handshake step to be non-blocking.
  *
- *                  The parameters \p ssl and \p cert are
- *                  guaranteed to remain valid as long as the SSL
- *                  configuration remains valid. On the other hand, this
+ *                  The parameters \p ssl and \p cert are guaranteed to remain
+ *                  valid throughout the handshake. On the other hand, this
  *                  function must save the contents of \p input if the value
  *                  is needed for later processing, because the \p input buffer
  *                  is no longer valid after this function returns.

From a668c601868932c27dbc73d705248cdf8d07f924 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Mon, 30 Apr 2018 11:54:39 +0200
Subject: [PATCH 11/21] Rename mbedtls_ssl_async_{get,set}_data for clarity

Rename to mbedtls_ssl_get_async_operation_data and
mbedtls_ssl_set_async_operation_data so that they're about
"async operation data" and not about some not-obvious "data".
---
 include/mbedtls/ssl.h      | 44 +++++++++++++++++++++-----------------
 library/ssl_srv.c          |  4 ++--
 library/ssl_tls.c          |  4 ++--
 programs/ssl/ssl_server2.c |  6 +++---
 4 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
index b7dc98b5b..ec9018a1f 100644
--- a/include/mbedtls/ssl.h
+++ b/include/mbedtls/ssl.h
@@ -574,8 +574,8 @@ typedef struct mbedtls_ssl_flight_item mbedtls_ssl_flight_item;
  *                  is needed for later processing, because the \p hash buffer
  *                  is no longer valid after this function returns.
  *
- *                  This function may call mbedtls_ssl_async_set_data() to
- *                  store an operation context for later retrieval
+ *                  This function may call mbedtls_ssl_set_async_operation_data()
+ *                  to store an operation context for later retrieval
  *                  by the resume callback.
  *
  * \note            For RSA signatures, this function must produce output
@@ -598,7 +598,8 @@ typedef struct mbedtls_ssl_flight_item mbedtls_ssl_flight_item;
  *                  [RFC 4492 section 5.4](https://tools.ietf.org/html/rfc4492#section-5.4).
  *
  * \param ssl             The SSL connection instance. It should not be
- *                        modified other than via mbedtls_ssl_async_set_data().
+ *                        modified other than via
+ *                        mbedtls_ssl_set_async_operation_data().
  * \param cert            Certificate containing the public key.
  *                        In simple cases, this is one of the pointers passed to
  *                        mbedtls_ssl_conf_own_cert() when configuring the SSL
@@ -650,8 +651,8 @@ typedef int mbedtls_ssl_async_sign_t( mbedtls_ssl_context *ssl,
  *                  is needed for later processing, because the \p input buffer
  *                  is no longer valid after this function returns.
  *
- *                  This function may call mbedtls_ssl_async_set_data() to
- *                  store an operation context for later retrieval
+ *                  This function may call mbedtls_ssl_set_async_operation_data()
+ *                  to store an operation context for later retrieval
  *                  by the resume callback.
  *
  * \warning         RSA decryption as used in TLS is subject to a potential
@@ -664,7 +665,8 @@ typedef int mbedtls_ssl_async_sign_t( mbedtls_ssl_context *ssl,
  *                  invalid padding.
  *
  * \param ssl             The SSL connection instance. It should not be
- *                        modified other than via mbedtls_ssl_async_set_data().
+ *                        modified other than via
+ *                        mbedtls_ssl_set_async_operation_data().
  * \param cert            Certificate containing the public key.
  *                        In simple cases, this is one of the pointers passed to
  *                        mbedtls_ssl_conf_own_cert() when configuring the SSL
@@ -709,13 +711,14 @@ typedef int mbedtls_ssl_async_decrypt_t( mbedtls_ssl_context *ssl,
  *                  does not wait for the operation to complete. This allows
  *                  the handshake step to be non-blocking.
  *
- *                  This function may call mbedtls_ssl_async_get_data() to
- *                  retrieve an operation context set by the start callback.
- *                  It may call mbedtls_ssl_async_set_data() to modify this
- *                  context.
+ *                  This function may call mbedtls_ssl_get_async_operation_data()
+ *                  to retrieve an operation context set by the start callback.
+ *                  It may call mbedtls_ssl_set_async_operation_data() to modify
+ *                  this context.
  *
  * \param ssl             The SSL connection instance. It should not be
- *                        modified other than via mbedtls_ssl_async_set_data().
+ *                        modified other than via
+ *                        mbedtls_ssl_set_async_operation_data().
  * \param output          Buffer containing the output (signature or decrypted
  *                        data) on success.
  * \param output_len      On success, number of bytes written to \p output.
@@ -744,8 +747,8 @@ typedef int mbedtls_ssl_async_resume_t( mbedtls_ssl_context *ssl,
  *                  This callback is called if an SSL connection is closed
  *                  while an asynchronous operation is in progress.
  *
- *                  This function may call mbedtls_ssl_async_get_data() to
- *                  retrieve an operation context set by the start callback.
+ *                  This function may call mbedtls_ssl_get_async_operation_data()
+ *                  to retrieve an operation context set by the start callback.
  *
  * \param ssl             The SSL connection instance. It should not be
  *                        modified.
@@ -1582,11 +1585,12 @@ void *mbedtls_ssl_conf_get_async_config_data( const mbedtls_ssl_config *conf );
  * \param ssl       The SSL context to access.
  *
  * \return          The asynchronous operation user context that was last
- *                  set during the current handshake. If mbedtls_ssl_set_data()
- *                  has not been called during the current handshake yet,
- *                  this function returns \c NULL.
+ *                  set during the current handshake. If
+ *                  mbedtls_ssl_set_async_operation_data() has not yet been
+ *                  called during the current handshake, this function returns
+ *                  \c NULL.
  */
-void *mbedtls_ssl_async_get_data( const mbedtls_ssl_context *ssl );
+void *mbedtls_ssl_get_async_operation_data( const mbedtls_ssl_context *ssl );
 
 /**
  * \brief           Retrieve the asynchronous operation user context.
@@ -1596,10 +1600,10 @@ void *mbedtls_ssl_async_get_data( const mbedtls_ssl_context *ssl );
  *
  * \param ssl       The SSL context to access.
  * \param ctx       The new value of the asynchronous operation user context.
- *                  Call mbedtls_ssl_get_data() later during the same handshake
- *                  to retrieve this value.
+ *                  Call mbedtls_ssl_get_async_operation_data() later during the
+ *                  same handshake to retrieve this value.
  */
-void mbedtls_ssl_async_set_data( mbedtls_ssl_context *ssl,
+void mbedtls_ssl_set_async_operation_data( mbedtls_ssl_context *ssl,
                                  void *ctx );
 #endif /* MBEDTLS_SSL_ASYNC_PRIVATE */
 
diff --git a/library/ssl_srv.c b/library/ssl_srv.c
index 5439f6d61..2b25e091f 100644
--- a/library/ssl_srv.c
+++ b/library/ssl_srv.c
@@ -2852,7 +2852,7 @@ static int ssl_resume_server_key_exchange( mbedtls_ssl_context *ssl,
     if( ret != MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS )
     {
         ssl->handshake->async_in_progress = 0;
-        mbedtls_ssl_async_set_data( ssl, NULL );
+        mbedtls_ssl_set_async_operation_data( ssl, NULL );
     }
     MBEDTLS_SSL_DEBUG_RET( 2, "ssl_resume_server_key_exchange", ret );
     return( ret );
@@ -3406,7 +3406,7 @@ static int ssl_resume_decrypt_pms( mbedtls_ssl_context *ssl,
     if( ret != MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS )
     {
         ssl->handshake->async_in_progress = 0;
-        mbedtls_ssl_async_set_data( ssl, NULL );
+        mbedtls_ssl_set_async_operation_data( ssl, NULL );
     }
     MBEDTLS_SSL_DEBUG_RET( 2, "ssl_decrypt_encrypted_pms", ret );
     return( ret );
diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index 04f34587d..3819b6f7f 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -6499,7 +6499,7 @@ void *mbedtls_ssl_conf_get_async_config_data( const mbedtls_ssl_config *conf )
     return( conf->p_async_config_data );
 }
 
-void *mbedtls_ssl_async_get_data( const mbedtls_ssl_context *ssl )
+void *mbedtls_ssl_get_async_operation_data( const mbedtls_ssl_context *ssl )
 {
     if( ssl->handshake == NULL )
         return( NULL );
@@ -6507,7 +6507,7 @@ void *mbedtls_ssl_async_get_data( const mbedtls_ssl_context *ssl )
         return( ssl->handshake->user_async_ctx );
 }
 
-void mbedtls_ssl_async_set_data( mbedtls_ssl_context *ssl,
+void mbedtls_ssl_set_async_operation_data( mbedtls_ssl_context *ssl,
                                  void *ctx )
 {
     if( ssl->handshake != NULL )
diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index b1f2382cb..876f8156c 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -997,7 +997,7 @@ static int ssl_async_start( mbedtls_ssl_context *ssl,
     memcpy( ctx->input, input, input_len );
     ctx->input_len = input_len;
     ctx->remaining_delay = config_data->slots[slot].delay;
-    mbedtls_ssl_async_set_data( ssl, ctx );
+    mbedtls_ssl_set_async_operation_data( ssl, ctx );
 
     if( ctx->remaining_delay == 0 )
         return( 0 );
@@ -1031,7 +1031,7 @@ static int ssl_async_resume( mbedtls_ssl_context *ssl,
                              size_t *output_len,
                              size_t output_size )
 {
-    ssl_async_operation_context_t *ctx = mbedtls_ssl_async_get_data( ssl );
+    ssl_async_operation_context_t *ctx = mbedtls_ssl_get_async_operation_data( ssl );
     ssl_async_key_context_t *config_data =
         mbedtls_ssl_conf_get_async_config_data( ssl->conf );
     ssl_async_key_slot_t *key_slot = &config_data->slots[ctx->slot];
@@ -1085,7 +1085,7 @@ static int ssl_async_resume( mbedtls_ssl_context *ssl,
 
 static void ssl_async_cancel( mbedtls_ssl_context *ssl )
 {
-    ssl_async_operation_context_t *ctx = mbedtls_ssl_async_get_data( ssl );
+    ssl_async_operation_context_t *ctx = mbedtls_ssl_get_async_operation_data( ssl );
     mbedtls_printf( "Async cancel callback.\n" );
     mbedtls_free( ctx );
 }

From 3dae1cfa3ab0e4c163b5a0f76e2708c000b7bdcb Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Mon, 30 Apr 2018 12:07:56 +0200
Subject: [PATCH 12/21] Async callback: use mbedtls_pk_check_pair to compare
 keys

In the current test code, the object that is used as a public key in
the certificate also contains a private key. However this is because
of the way the stest code is built and does not demonstrate the API in
a useful way. Use mbedtls_pk_check_pair, which is not what real-world
code would do (since the private key would typically be in an external
cryptoprocessor) but is a more representative placeholder.
---
 programs/ssl/ssl_server2.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index 876f8156c..d550b7c4c 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -963,11 +963,14 @@ static int ssl_async_start( mbedtls_ssl_context *ssl,
         mbedtls_printf( "Async %s callback: looking for DN=%s\n", op_name, dn );
     }
 
+    /* Look for a private key that matches the public key in cert.
+     * Since this test code has the private key inside Mbed TLS,
+     * we call mbedtls_pk_check_pair to match a private key with the
+     * public key. */
     for( slot = 0; slot < config_data->slots_used; slot++ )
     {
-        if( memcmp( &config_data->slots[slot].cert->pk,
-                    &cert->pk,
-                    sizeof( cert->pk ) ) == 0 )
+        if( mbedtls_pk_check_pair( &cert->pk,
+                                   config_data->slots[slot].pk ) == 0 )
             break;
     }
     if( slot == config_data->slots_used )

From 7457933a044ba8c4552490b84d6fde0b49fea1ff Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Mon, 30 Apr 2018 13:57:45 +0200
Subject: [PATCH 13/21] SSL async callbacks documentation: clarify resource
 cleanup

Clarify when resume must clean up resources and when cancel is called.
---
 include/mbedtls/ssl.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
index ec9018a1f..a839e84d8 100644
--- a/include/mbedtls/ssl.h
+++ b/include/mbedtls/ssl.h
@@ -576,7 +576,7 @@ typedef struct mbedtls_ssl_flight_item mbedtls_ssl_flight_item;
  *
  *                  This function may call mbedtls_ssl_set_async_operation_data()
  *                  to store an operation context for later retrieval
- *                  by the resume callback.
+ *                  by the resume or cancel callback.
  *
  * \note            For RSA signatures, this function must produce output
  *                  that is consistent with PKCS#1 v1.5 in the same way as
@@ -653,7 +653,7 @@ typedef int mbedtls_ssl_async_sign_t( mbedtls_ssl_context *ssl,
  *
  *                  This function may call mbedtls_ssl_set_async_operation_data()
  *                  to store an operation context for later retrieval
- *                  by the resume callback.
+ *                  by the resume or cancel callback.
  *
  * \warning         RSA decryption as used in TLS is subject to a potential
  *                  timing side channel attack first discovered by Bleichenbacher
@@ -716,6 +716,10 @@ typedef int mbedtls_ssl_async_decrypt_t( mbedtls_ssl_context *ssl,
  *                  It may call mbedtls_ssl_set_async_operation_data() to modify
  *                  this context.
  *
+ *                  Note that when this function returns a status other than
+ *                  #MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS, it must free any
+ *                  resources associated with the operation.
+ *
  * \param ssl             The SSL connection instance. It should not be
  *                        modified other than via
  *                        mbedtls_ssl_set_async_operation_data().
@@ -745,7 +749,12 @@ typedef int mbedtls_ssl_async_resume_t( mbedtls_ssl_context *ssl,
  * \brief           Callback type: cancel external operation.
  *
  *                  This callback is called if an SSL connection is closed
- *                  while an asynchronous operation is in progress.
+ *                  while an asynchronous operation is in progress. Note that
+ *                  this callback is not called if the
+ *                  ::mbedtls_ssl_async_resume_t callback has run and has
+ *                  returned a value other than
+ *                  #MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS, since in that case
+ *                  the asynchronous operation has already completed.
  *
  *                  This function may call mbedtls_ssl_get_async_operation_data()
  *                  to retrieve an operation context set by the start callback.

From ef30742a27ea37ef9bbeef457a6c2e3469046ec6 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Mon, 30 Apr 2018 16:37:03 +0200
Subject: [PATCH 14/21] Clarify "as directed here" in SSL async callback
 documentation

---
 include/mbedtls/ssl.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
index a839e84d8..606d9c2f5 100644
--- a/include/mbedtls/ssl.h
+++ b/include/mbedtls/ssl.h
@@ -624,7 +624,7 @@ typedef struct mbedtls_ssl_flight_item mbedtls_ssl_flight_item;
  *                  propagated up the call chain. The callback should
  *                  use \c MBEDTLS_ERR_PK_xxx error codes, and <b>must not</b>
  *                  use \c MBEDTLS_ERR_SSL_xxx error codes except as
- *                  directed here.
+ *                  directed in the documentation of this callback.
  */
 typedef int mbedtls_ssl_async_sign_t( mbedtls_ssl_context *ssl,
                                       mbedtls_x509_crt *cert,
@@ -690,7 +690,7 @@ typedef int mbedtls_ssl_async_sign_t( mbedtls_ssl_context *ssl,
  *                  propagated up the call chain. The callback should
  *                  use \c MBEDTLS_ERR_PK_xxx error codes, and <b>must not</b>
  *                  use \c MBEDTLS_ERR_SSL_xxx error codes except as
- *                  directed here.
+ *                  directed in the documentation of this callback.
  */
 typedef int mbedtls_ssl_async_decrypt_t( mbedtls_ssl_context *ssl,
                                          mbedtls_x509_crt *cert,
@@ -738,7 +738,7 @@ typedef int mbedtls_ssl_async_decrypt_t( mbedtls_ssl_context *ssl,
  *                  The SSL handshake is aborted. The callback should
  *                  use \c MBEDTLS_ERR_PK_xxx error codes, and <b>must not</b>
  *                  use \c MBEDTLS_ERR_SSL_xxx error codes except as
- *                  directed here.
+ *                  directed in the documentation of this callback.
  */
 typedef int mbedtls_ssl_async_resume_t( mbedtls_ssl_context *ssl,
                                         unsigned char *output,

From f5a9996088582742ac60030d7a1119a7c28ecfaf Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Mon, 30 Apr 2018 16:37:23 +0200
Subject: [PATCH 15/21] ssl_server2: get op_name from context in
 ssl_async_resume as well

---
 programs/ssl/ssl_server2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index d550b7c4c..ac3d1b1c7 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -1039,7 +1039,7 @@ static int ssl_async_resume( mbedtls_ssl_context *ssl,
         mbedtls_ssl_conf_get_async_config_data( ssl->conf );
     ssl_async_key_slot_t *key_slot = &config_data->slots[ctx->slot];
     int ret;
-    const char *op_name = NULL;
+    const char *op_name;
 
     if( ctx->remaining_delay > 0 )
     {
@@ -1052,14 +1052,12 @@ static int ssl_async_resume( mbedtls_ssl_context *ssl,
     switch( ctx->operation_type )
     {
         case ASYNC_OP_DECRYPT:
-            op_name = "decrypt";
             ret = mbedtls_pk_decrypt( key_slot->pk,
                                       ctx->input, ctx->input_len,
                                       output, output_len, output_size,
                                       config_data->f_rng, config_data->p_rng );
             break;
         case ASYNC_OP_SIGN:
-            op_name = "sign";
             ret = mbedtls_pk_sign( key_slot->pk,
                                    ctx->md_alg,
                                    ctx->input, ctx->input_len,
@@ -1073,6 +1071,8 @@ static int ssl_async_resume( mbedtls_ssl_context *ssl,
             break;
     }
 
+    op_name = ssl_async_operation_names[ctx->operation_type];
+
     if( config_data->inject_error == SSL_ASYNC_INJECT_ERROR_RESUME )
     {
         mbedtls_printf( "Async resume callback: %s done but injected error\n",

From 2636fade52491b07a1fae3c8a9099ea3990f6c04 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Tue, 12 Jun 2018 14:17:39 +0200
Subject: [PATCH 16/21] ssl_async_resume: free the operation context on error

---
 programs/ssl/ssl_server2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index ac3d1b1c7..a7b019c7f 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -1077,6 +1077,7 @@ static int ssl_async_resume( mbedtls_ssl_context *ssl,
     {
         mbedtls_printf( "Async resume callback: %s done but injected error\n",
                         op_name );
+        mbedtls_free( ctx );
         return( MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE );
     }
 

From e2479890611c458e6f6c8729438f1a9a9df4d266 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Wed, 13 Jun 2018 18:06:51 +0200
Subject: [PATCH 17/21] SNI + SSL async callback: make all keys async

When testing async callbacks with SNI, make all the keys async, not
just the first one. Otherwise the test is fragile with respect to
whether a key is used directly or through the async callbacks.
---
 programs/ssl/ssl_server2.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index a7b019c7f..ae50b3d31 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -897,7 +897,7 @@ typedef enum {
 
 typedef struct
 {
-    ssl_async_key_slot_t slots[3]; /* key, key2, sni */
+    ssl_async_key_slot_t slots[4]; /* key, key2, sni1, sni2 */
     size_t slots_used;
     ssl_async_inject_error_t inject_error;
     int (*f_rng)(void *, unsigned char *, size_t);
@@ -2387,16 +2387,20 @@ int main( int argc, char *argv[] )
 #if defined(MBEDTLS_SSL_ASYNC_PRIVATE)
         if( opt.async_private_delay2 >= 0 )
         {
-            ret = ssl_async_set_key( &ssl_async_keys,
-                                     sni_info->cert, sni_info->key,
-                                     opt.async_private_delay2 );
-            if( ret < 0 )
+            sni_entry *cur;
+            for( cur = sni_info; cur != NULL; cur = cur->next )
             {
-                mbedtls_printf( "  Test error: ssl_async_set_key failed (%d)\n",
-                                ret );
-                goto exit;
+                ret = ssl_async_set_key( &ssl_async_keys,
+                                         cur->cert, cur->key,
+                                         opt.async_private_delay2 );
+                if( ret < 0 )
+                {
+                    mbedtls_printf( "  Test error: ssl_async_set_key failed (%d)\n",
+                                    ret );
+                    goto exit;
+                }
+                cur->key = NULL;
             }
-            sni_info->key = NULL;
         }
 #endif /* MBEDTLS_SSL_ASYNC_PRIVATE */
     }

From 4481744538f9bad09f73090678bd071fdc430e1a Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Wed, 13 Jun 2018 18:09:28 +0200
Subject: [PATCH 18/21] Fix memory leak in ssl_server2 with SNI + async
 callback

In ssl_server2, the private key objects are normally local variables
of the main function. However this does not hold for private keys in
the SNI configuration. When async callbacks are used, the test code
transfers the ownership of the private keys to the async callbacks.
Therefore the test code must free the SNI private keys through the
async callbacks (but it must not free the straight private keys this
way since they are not even heap-allocated).
---
 programs/ssl/ssl_server2.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index ae50b3d31..81041c44d 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -882,9 +882,10 @@ static int mbedtls_status_is_ssl_in_progress( int ret )
 #if defined(MBEDTLS_SSL_ASYNC_PRIVATE)
 typedef struct
 {
-    mbedtls_x509_crt *cert;
-    mbedtls_pk_context *pk;
-    unsigned delay;
+    mbedtls_x509_crt *cert; /*!< Certificate corresponding to the key */
+    mbedtls_pk_context *pk; /*!< Private key */
+    unsigned delay; /*!< Number of resume steps to go through */
+    unsigned pk_owned : 1; /*!< Whether to free the pk object on exit */
 } ssl_async_key_slot_t;
 
 typedef enum {
@@ -905,15 +906,17 @@ typedef struct
 } ssl_async_key_context_t;
 
 int ssl_async_set_key( ssl_async_key_context_t *ctx,
-                        mbedtls_x509_crt *cert,
-                        mbedtls_pk_context *pk,
-                        unsigned delay )
+                       mbedtls_x509_crt *cert,
+                       mbedtls_pk_context *pk,
+                       int pk_take_ownership,
+                       unsigned delay )
 {
     if( ctx->slots_used >= sizeof( ctx->slots ) / sizeof( *ctx->slots ) )
         return( -1 );
     ctx->slots[ctx->slots_used].cert = cert;
     ctx->slots[ctx->slots_used].pk = pk;
     ctx->slots[ctx->slots_used].delay = delay;
+    ctx->slots[ctx->slots_used].pk_owned = pk_take_ownership;
     ++ctx->slots_used;
     return( 0 );
 }
@@ -1067,6 +1070,7 @@ static int ssl_async_resume( mbedtls_ssl_context *ssl,
         default:
             mbedtls_printf( "Async resume (slot %u): unknown operation type %ld. This shouldn't happen.\n",
                             ctx->slot, (long) ctx->operation_type );
+            mbedtls_free( ctx );
             return( MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE );
             break;
     }
@@ -2306,7 +2310,7 @@ int main( int argc, char *argv[] )
 #if defined(MBEDTLS_SSL_ASYNC_PRIVATE)
         if( opt.async_private_delay1 >= 0 )
         {
-            ret = ssl_async_set_key( &ssl_async_keys, &srvcert, pk,
+            ret = ssl_async_set_key( &ssl_async_keys, &srvcert, pk, 0,
                                      opt.async_private_delay1 );
             if( ret < 0 )
             {
@@ -2329,7 +2333,7 @@ int main( int argc, char *argv[] )
 #if defined(MBEDTLS_SSL_ASYNC_PRIVATE)
         if( opt.async_private_delay2 >= 0 )
         {
-            ret = ssl_async_set_key( &ssl_async_keys, &srvcert2, pk,
+            ret = ssl_async_set_key( &ssl_async_keys, &srvcert2, pk, 0,
                                      opt.async_private_delay2 );
             if( ret < 0 )
             {
@@ -2391,7 +2395,7 @@ int main( int argc, char *argv[] )
             for( cur = sni_info; cur != NULL; cur = cur->next )
             {
                 ret = ssl_async_set_key( &ssl_async_keys,
-                                         cur->cert, cur->key,
+                                         cur->cert, cur->key, 1,
                                          opt.async_private_delay2 );
                 if( ret < 0 )
                 {
@@ -3018,6 +3022,17 @@ exit:
     mbedtls_x509_crt_free( &srvcert2 );
     mbedtls_pk_free( &pkey2 );
 #endif
+#if defined(MBEDTLS_SSL_ASYNC_PRIVATE)
+    for( i = 0; (size_t) i < ssl_async_keys.slots_used; i++ )
+    {
+        if( ssl_async_keys.slots[i].pk_owned )
+        {
+            mbedtls_pk_free( ssl_async_keys.slots[i].pk );
+            mbedtls_free( ssl_async_keys.slots[i].pk );
+            ssl_async_keys.slots[i].pk = NULL;
+        }
+    }
+#endif
 #if defined(SNI_OPTION)
     sni_free( sni_info );
 #endif

From c306a059d18f002b7701d1f71a70077f1dec922a Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Tue, 12 Jun 2018 15:06:40 +0200
Subject: [PATCH 19/21] SSL async tests: add a few test cases for error in
 decrypt

The code paths in the library are different for decryption and for
signature. Improve the test coverage by doing some error path tests
for decryption in addition to signature.
---
 tests/ssl-opt.sh | 43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh
index 6afca2d12..3ea56db8e 100755
--- a/tests/ssl-opt.sh
+++ b/tests/ssl-opt.sh
@@ -4243,7 +4243,7 @@ run_test    "SSL async private: fall back to transparent key" \
             -s "Async sign callback: no key matches this certificate."
 
 requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
-run_test    "SSL async private: error in start" \
+run_test    "SSL async private: sign, error in start" \
             "$P_SRV \
              async_operations=s async_private_delay1=1 async_private_delay2=1 \
              async_private_error=1" \
@@ -4255,7 +4255,7 @@ run_test    "SSL async private: error in start" \
             -s "! mbedtls_ssl_handshake returned"
 
 requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
-run_test    "SSL async private: cancel after start" \
+run_test    "SSL async private: sign, cancel after start" \
             "$P_SRV \
              async_operations=s async_private_delay1=1 async_private_delay2=1 \
              async_private_error=2" \
@@ -4266,7 +4266,7 @@ run_test    "SSL async private: cancel after start" \
             -s "Async cancel"
 
 requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
-run_test    "SSL async private: error in resume" \
+run_test    "SSL async private: sign, error in resume" \
             "$P_SRV \
              async_operations=s async_private_delay1=1 async_private_delay2=1 \
              async_private_error=3" \
@@ -4277,6 +4277,41 @@ run_test    "SSL async private: error in resume" \
             -S "Async cancel" \
             -s "! mbedtls_ssl_handshake returned"
 
+requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
+run_test    "SSL async private: decrypt, error in start" \
+            "$P_SRV \
+             async_operations=d async_private_delay1=1 async_private_delay2=1 \
+             async_private_error=1" \
+            "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA" \
+            1 \
+            -s "Async decrypt callback: injected error" \
+            -S "Async resume" \
+            -S "Async cancel" \
+            -s "! mbedtls_ssl_handshake returned"
+
+requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
+run_test    "SSL async private: decrypt, cancel after start" \
+            "$P_SRV \
+             async_operations=d async_private_delay1=1 async_private_delay2=1 \
+             async_private_error=2" \
+            "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA" \
+            1 \
+            -s "Async decrypt callback: using key slot " \
+            -S "Async resume" \
+            -s "Async cancel"
+
+requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
+run_test    "SSL async private: decrypt, error in resume" \
+            "$P_SRV \
+             async_operations=d async_private_delay1=1 async_private_delay2=1 \
+             async_private_error=3" \
+            "$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA" \
+            1 \
+            -s "Async decrypt callback: using key slot " \
+            -s "Async resume callback: decrypt done but injected error" \
+            -S "Async cancel" \
+            -s "! mbedtls_ssl_handshake returned"
+
 requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
 run_test    "SSL async private: cancel after start then operate correctly" \
             "$P_SRV \
@@ -4320,7 +4355,7 @@ run_test    "SSL async private: cancel after start then fall back to transparent
 
 # key1: ECDSA, key2: RSA; use key1 through async, then key2 directly
 requires_config_enabled MBEDTLS_SSL_ASYNC_PRIVATE
-run_test    "SSL async private: error in resume then fall back to transparent key" \
+run_test    "SSL async private: sign, error in resume then fall back to transparent key" \
             "$P_SRV \
              async_operations=s async_private_delay1=1 async_private_error=-3 \
              key_file=data_files/server5.key crt_file=data_files/server5.crt \

From ace05929e87817324aed83688f4b424780bf70cf Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Wed, 13 Jun 2018 18:16:41 +0200
Subject: [PATCH 20/21] Fix harmless use of uninitialized memory in
 ssl_parse_encrypted_pms

In ssl_parse_encrypted_pms, some operational failures from
ssl_decrypt_encrypted_pms lead to diff being set to a value that
depended on some uninitialized unsigned char and size_t values. This didn't
affect the behavior of the program (assuming an implementation with no
trap values for size_t) because all that matters is whether diff is 0,
but Valgrind rightfully complained about the use of uninitialized
memory. Behave nicely and initialize the offending memory.
---
 library/ssl_srv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/library/ssl_srv.c b/library/ssl_srv.c
index 2b25e091f..b49b9e1dd 100644
--- a/library/ssl_srv.c
+++ b/library/ssl_srv.c
@@ -3513,6 +3513,15 @@ static int ssl_parse_encrypted_pms( mbedtls_ssl_context *ssl,
     size_t i, peer_pmslen;
     unsigned int diff;
 
+    /* In case of a failure in decryption, the decryption may write less than
+     * 2 bytes of output, but we always read the first two bytes. It doesn't
+     * matter in the end because diff will be nonzero in that case due to
+     * peer_pmslen being less than 48, and we only care whether diff is 0.
+     * But do initialize peer_pms for robustness anyway. This also makes
+     * memory analyzers happy (don't access uninitialized memory, even
+     * if it's an unsigned char). */
+    peer_pms[0] = peer_pms[1] = ~0;
+
     ret = ssl_decrypt_encrypted_pms( ssl, p, end,
                                      peer_pms,
                                      &peer_pmslen,

From d5d983e16830b1a131b08c8f7746398a256a7e58 Mon Sep 17 00:00:00 2001
From: Gilles Peskine <Gilles.Peskine@arm.com>
Date: Fri, 15 Jun 2018 14:05:10 +0200
Subject: [PATCH 21/21] ssl_server2: handle mbedtls_x509_dn_gets failure

If mbedtls_x509_dn_gets fails, the server could end up calling printf
on an uninitialized buffer. Check if the function succeeds. Found by
Coverity.
---
 programs/ssl/ssl_server2.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index 81041c44d..3a413ad5e 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -962,8 +962,9 @@ static int ssl_async_start( mbedtls_ssl_context *ssl,
 
     {
         char dn[100];
-        mbedtls_x509_dn_gets( dn, sizeof( dn ), &cert->subject );
-        mbedtls_printf( "Async %s callback: looking for DN=%s\n", op_name, dn );
+        if( mbedtls_x509_dn_gets( dn, sizeof( dn ), &cert->subject ) > 0 )
+            mbedtls_printf( "Async %s callback: looking for DN=%s\n",
+                            op_name, dn );
     }
 
     /* Look for a private key that matches the public key in cert.