From a4bfaa8204826fb6f50a5fa2dd3e63f629659622 Mon Sep 17 00:00:00 2001
From: Hanno Becker <hanno.becker@arm.com>
Date: Fri, 28 Jun 2019 10:34:23 +0100
Subject: [PATCH] Make X.509 CRT cache reference counting unconditional

Previously, reference counting for the CRT frames and PK contexts
handed out by mbedtls_x509_crt_{frame|pk}_acquire() was implemented
only in case threading support was enabled, which leaves the door
open for a potential use-after-free should a single-threaded application
use nested calls to mbedtls_x509_crt_acquire().

Since Mbed TLS itself does not use such nested calls, it might be
preferred long-term to forbid nesting of acquire calls on the API
level, and hence get rid of reference counting in the interest of
code-size benefits. However, this can be considered as an optimization
of X.509 on demand parsing, and for now this commit introduces
reference counting unconditionally to have a safe version of
on demand parsing to build further optimizations upon.
---
 include/mbedtls/x509_crt.h      | 16 ++++++++--------
 include/mbedtls/x509_internal.h |  2 +-
 library/x509_crt.c              |  5 ++---
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h
index 6db9bc9fa..85943c7b3 100644
--- a/include/mbedtls/x509_crt.h
+++ b/include/mbedtls/x509_crt.h
@@ -831,19 +831,17 @@ static inline int mbedtls_x509_crt_frame_acquire( mbedtls_x509_crt const *crt,
 #if defined(MBEDTLS_THREADING_C)
     if( mbedtls_mutex_lock( &crt->cache->frame_mutex ) != 0 )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
+#endif /* MBEDTLS_THREADING_C */
 
     if( crt->cache->frame_readers == 0 )
-#endif /* MBEDTLS_THREADING_C */
-    {
         ret = mbedtls_x509_crt_cache_provide_frame( crt );
-    }
 
-#if defined(MBEDTLS_THREADING_C)
     if( crt->cache->frame_readers == MBEDTLS_X509_CACHE_FRAME_READERS_MAX )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
 
     crt->cache->frame_readers++;
 
+#if defined(MBEDTLS_THREADING_C)
     if( mbedtls_mutex_unlock( &crt->cache->frame_mutex ) != 0 )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
 #endif /* MBEDTLS_THREADING_C */
@@ -864,12 +862,14 @@ static inline int mbedtls_x509_crt_frame_release( mbedtls_x509_crt const *crt )
 #if defined(MBEDTLS_THREADING_C)
     if( mbedtls_mutex_lock( &crt->cache->frame_mutex ) != 0 )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
+#endif /* MBEDTLS_THREADING_C */
 
     if( crt->cache->frame_readers == 0 )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
 
     crt->cache->frame_readers--;
 
+#if defined(MBEDTLS_THREADING_C)
     mbedtls_mutex_unlock( &crt->cache->frame_mutex );
 #endif /* MBEDTLS_THREADING_C */
 
@@ -919,19 +919,17 @@ static inline int mbedtls_x509_crt_pk_acquire( mbedtls_x509_crt const *crt,
 #if defined(MBEDTLS_THREADING_C)
     if( mbedtls_mutex_lock( &crt->cache->pk_mutex ) != 0 )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
+#endif /* MBEDTLS_THREADING_C */
 
     if( crt->cache->pk_readers == 0 )
-#endif /* MBEDTLS_THREADING_C */
-    {
         ret = mbedtls_x509_crt_cache_provide_pk( crt );
-    }
 
-#if defined(MBEDTLS_THREADING_C)
     if( crt->cache->pk_readers == MBEDTLS_X509_CACHE_PK_READERS_MAX )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
 
     crt->cache->pk_readers++;
 
+#if defined(MBEDTLS_THREADING_C)
     if( mbedtls_mutex_unlock( &crt->cache->pk_mutex ) != 0 )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
 #endif /* MBEDTLS_THREADING_C */
@@ -952,12 +950,14 @@ static inline int mbedtls_x509_crt_pk_release( mbedtls_x509_crt const *crt )
 #if defined(MBEDTLS_THREADING_C)
     if( mbedtls_mutex_lock( &crt->cache->pk_mutex ) != 0 )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
+#endif /* MBEDTLS_THREADING_C */
 
     if( crt->cache->pk_readers == 0 )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
 
     crt->cache->pk_readers--;
 
+#if defined(MBEDTLS_THREADING_C)
     mbedtls_mutex_unlock( &crt->cache->pk_mutex );
 #endif /* MBEDTLS_THREADING_C */
 
diff --git a/include/mbedtls/x509_internal.h b/include/mbedtls/x509_internal.h
index bed9772e3..77291089f 100644
--- a/include/mbedtls/x509_internal.h
+++ b/include/mbedtls/x509_internal.h
@@ -37,9 +37,9 @@ struct mbedtls_x509_crt_frame;
 #define MBEDTLS_X509_CACHE_FRAME_READERS_MAX ((uint32_t) -1)
 typedef struct mbedtls_x509_crt_cache
 {
-#if defined(MBEDTLS_THREADING_C)
     uint32_t frame_readers;
     uint32_t pk_readers;
+#if defined(MBEDTLS_THREADING_C)
     mbedtls_threading_mutex_t frame_mutex;
     mbedtls_threading_mutex_t pk_mutex;
 #endif
diff --git a/library/x509_crt.c b/library/x509_crt.c
index c601686f9..fa5124147 100644
--- a/library/x509_crt.c
+++ b/library/x509_crt.c
@@ -112,10 +112,9 @@ int mbedtls_x509_crt_flush_cache_pk( mbedtls_x509_crt const *crt )
 #if defined(MBEDTLS_THREADING_C)
     if( mbedtls_mutex_lock( &crt->cache->pk_mutex ) != 0 )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
-
+#endif
     /* Can only free the PK context if nobody is using it. */
     if( crt->cache->pk_readers == 0 )
-#endif
     {
 #if !defined(MBEDTLS_X509_ON_DEMAND_PARSING)
         /* The cache holds a shallow copy of the PK context
@@ -140,10 +139,10 @@ int mbedtls_x509_crt_flush_cache_frame( mbedtls_x509_crt const *crt )
 #if defined(MBEDTLS_THREADING_C)
     if( mbedtls_mutex_lock( &crt->cache->frame_mutex ) != 0 )
         return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );
+#endif
 
     /* Can only free the frame if nobody is using it. */
     if( crt->cache->frame_readers == 0 )
-#endif
     {
         mbedtls_free( crt->cache->frame );
         crt->cache->frame = NULL;