From 304736d60c967412788c5aaf0435de4653a76a5a Mon Sep 17 00:00:00 2001
From: Hanno Becker <hanno.becker@arm.com>
Date: Wed, 10 Oct 2018 11:23:33 +0100
Subject: [PATCH] Reinitialize PK ctx in mbedtls_pk_parse_key before reuse are
 free

Context: This commit makes a change to mbedtls_pk_parse_key() which
is responsible for parsing of private keys. The function doesn't know
the key format in advance (PEM vs. DER, encrypted vs. unencrypted) and
tries them one by one, resetting the PK context in between.

Issue: The previous code resets the PK context through a call to
mbedtls_pk_free() along, lacking the accompanying mbedtls_pk_init()
call. Practically, this is not an issue because functionally
mbedtls_pk_free() + mbedtls_pk_init() is equivalent to mbedtls_pk_free()
with the current implementation of these functions, but strictly
speaking it's nonetheless a violation of the API semantics according
to which xxx_free() functions leave a context in uninitialized state.
(yet not entirely random, because xxx_free() functions must be idempotent,
so they cannot just fill the context they operate on with garbage).

Change: The commit adds calls to mbedtls_pk_init() after those calls
to mbedtls_pk_free() within mbedtls_pk_parse_key() after which the
PK context might still be used.
---
 library/pkparse.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/library/pkparse.c b/library/pkparse.c
index a899082b5..be9d8290a 100644
--- a/library/pkparse.c
+++ b/library/pkparse.c
@@ -1239,6 +1239,7 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk,
         return( 0 );
 
     mbedtls_pk_free( pk );
+    mbedtls_pk_init( pk );
 
     if( ret == MBEDTLS_ERR_PK_PASSWORD_MISMATCH )
     {
@@ -1250,39 +1251,42 @@ int mbedtls_pk_parse_key( mbedtls_pk_context *pk,
         return( 0 );
 
     mbedtls_pk_free( pk );
+    mbedtls_pk_init( pk );
 
 #if defined(MBEDTLS_RSA_C)
 
     pk_info = mbedtls_pk_info_from_type( MBEDTLS_PK_RSA );
-    if( ( ret = mbedtls_pk_setup( pk, pk_info ) ) != 0 ||
-        ( ret = pk_parse_key_pkcs1_der( mbedtls_pk_rsa( *pk ),
-                                        key, keylen ) ) != 0 )
-    {
-        mbedtls_pk_free( pk );
-    }
-    else
+    if( mbedtls_pk_setup( pk, pk_info ) == 0 &&
+        pk_parse_key_pkcs1_der( mbedtls_pk_rsa( *pk ), key, keylen ) == 0 )
     {
         return( 0 );
     }
 
+    mbedtls_pk_free( pk );
+    mbedtls_pk_init( pk );
 #endif /* MBEDTLS_RSA_C */
 
 #if defined(MBEDTLS_ECP_C)
-
     pk_info = mbedtls_pk_info_from_type( MBEDTLS_PK_ECKEY );
-    if( ( ret = mbedtls_pk_setup( pk, pk_info ) ) != 0 ||
-        ( ret = pk_parse_key_sec1_der( mbedtls_pk_ec( *pk ),
-                                       key, keylen ) ) != 0 )
-    {
-        mbedtls_pk_free( pk );
-    }
-    else
+    if( mbedtls_pk_setup( pk, pk_info ) == 0 &&
+        pk_parse_key_sec1_der( mbedtls_pk_ec( *pk ),
+                               key, keylen ) == 0 )
     {
         return( 0 );
     }
-
+    mbedtls_pk_free( pk );
 #endif /* MBEDTLS_ECP_C */
 
+    /* If MBEDTLS_RSA_C is defined but MBEDTLS_ECP_C isn't,
+     * it is ok to leave the PK context initialized but not
+     * freed: It is the caller's responsibility to call pk_init()
+     * before calling this function, and to call pk_free()
+     * when it fails. If MBEDTLS_ECP_C is defined but MBEDTLS_RSA_C
+     * isn't, this leads to mbedtls_pk_free() being called
+     * twice, once here and once by the caller, but this is
+     * also ok and in line with the mbedtls_pk_free() calls
+     * on failed PEM parsing attempts. */
+
     return( MBEDTLS_ERR_PK_KEY_INVALID_FORMAT );
 }