Rework SSL code (#1427)

* Rework SSL code - Add missing calls to platform free memory and mbedTLS API calls to free TLS vars memory on TLS context init failure and exit. - The above fixed a memory leak with TLS context. Related with nanoframework/Home#509. - Rework ssl close socket to account for bad or non existing TLS context and not nulling required TLS context vars required to properly close context and free memory. - Several renames in SSL_Context struct for clarity. - Rename ssl close socket (was missing an _). - Rework several declarations to match our coding style guidelines. - Replace booleans at several code locations. - Replace SSL private functions with ones from C standard libs. - Clean up code and includes in several files. - Remove ssl types source file (wasn't being used). - Remove calls to (not used anymore) time callbacks. Signed-off-by: José Simões <jose.simoes@eclo.solutions> * Update ssl_close_socket_internal.cpp Return 0 ( no error )
nanoframework · Aug 4, 2019 · 9fd78b8 · 9fd78b8
1 parent 37e1d2a
commit 9fd78b8
Show file tree

Hide file tree

Showing 24 changed files with 558 additions and 260 deletions.
diff --git a/CMake/Modules/FindNF_Networking.cmake b/CMake/Modules/FindNF_Networking.cmake
@@ -36,7 +36,7 @@ set(NF_Networking_Security_SRCS
     ssl.cpp
     ssl_accept_internal.cpp
     ssl_add_cert_auth_internal.cpp
-    ssl_closesocket_internal.cpp
+    ssl_close_socket_internal.cpp
     ssl_connect_internal.cpp
     ssl_decode_private_key_internal.cpp
     ssl_exit_context_internal.cpp
@@ -49,7 +49,6 @@ set(NF_Networking_Security_SRCS
     ssl_uninitialize_internal.cpp
     ssl_write_internal.cpp
 
-    # ssl_types.cpp
 )
 
 

diff --git a/src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_SslNative.cpp b/src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_SslNative.cpp
@@ -536,8 +536,6 @@ HRESULT Library_sys_net_native_System_Net_Security_SslNative::InitHelper( CLR_RT
         password = hbPwd->StringText();
     }
 
-    SSL_RegisterTimeCallback( Time_GetDateTime );
-
     if(isServer)
     {
         result = (SSL_ServerInit( sslMode, sslVerify, (const char*)sslCert, sslCert == NULL ? 0 : arrCert->m_numOfElements, pk, pk == NULL ? 0 : privateKey->m_numOfElements, password, hal_strlen_s(password), sslContext ) ? 0 : -1);

diff --git a/src/PAL/COM/sockets/ssl/mbedTLS/nf_mbedtls_config.h b/src/PAL/COM/sockets/ssl/mbedTLS/nf_mbedtls_config.h
@@ -91,7 +91,7 @@ time_t nf_get_unix_epoch();
 #define MBEDTLS_SSL_DTLS_HELLO_VERIFY
 #define MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE
 #define MBEDTLS_SSL_DTLS_BADMAC_LIMIT
-#define MBEDTLS_SSL_EXPORT_KEYS
+#define MBEDTLS_SSL_SERVER_NAME_INDICATION
 #define MBEDTLS_SSL_TRUNCATED_HMAC
 #define MBEDTLS_X509_CHECK_KEY_USAGE
 #define MBEDTLS_X509_CHECK_EXTENDED_KEY_USAGE

diff --git a/src/PAL/COM/sockets/ssl/mbedTLS/ssl_accept_internal.cpp b/src/PAL/COM/sockets/ssl/mbedTLS/ssl_accept_internal.cpp
@@ -6,20 +6,22 @@
 #include <ssl.h>
 #include "mbedtls.h"
 
-int ssl_accept_internal( int sd, int sslContextHandle )
+int ssl_accept_internal( 
+    int sd, 
+    int contextHandle )
 {
     mbedTLS_NFContext* context;
     mbedtls_ssl_context *ssl;
     int nonblock = 0;
     int ret = SOCK_SOCKET_ERROR;
 
-    // Check sslContextHandle range
-    if((sslContextHandle >= (int)ARRAYSIZE(g_SSL_Driver.m_sslContextArray)) || (sslContextHandle < 0))
+    // Check contextHandle range
+    if((contextHandle >= (int)ARRAYSIZE(g_SSL_Driver.ContextArray)) || (contextHandle < 0))
     {
         goto error;
     }
 
-    context = (mbedTLS_NFContext*)g_SSL_Driver.m_sslContextArray[sslContextHandle].SslContext;
+    context = (mbedTLS_NFContext*)g_SSL_Driver.ContextArray[contextHandle].Context;
     ssl = context->ssl;
 
     // sanity check
@@ -30,7 +32,6 @@ int ssl_accept_internal( int sd, int sslContextHandle )
 
     // TODO check how to handle server certificates and certificate chain parsing
 
-
     // if( ( ret = mbedtls_net_bind( context->server_fd, NULL, "4433", MBEDTLS_NET_PROTO_TCP ) ) != 0 )
     // {
     //     goto error;

diff --git a/src/PAL/COM/sockets/ssl/mbedTLS/ssl_add_cert_auth_internal.cpp b/src/PAL/COM/sockets/ssl/mbedTLS/ssl_add_cert_auth_internal.cpp
@@ -6,21 +6,25 @@
 #include <ssl.h>
 #include "mbedtls.h"
 
-bool ssl_add_cert_auth_internal( int sslContextHandle, const char* certificate, int certLength, const char* certPassword )
+bool ssl_add_cert_auth_internal( 
+    int contextHandle, 
+    const char* certificate, 
+    int certLength, 
+    const char* certPassword )
 {
     (void)certPassword;
 
     mbedTLS_NFContext* context;
 
-    // Check sslContextHandle range
-    if((sslContextHandle >= (int)ARRAYSIZE(g_SSL_Driver.m_sslContextArray)) || (sslContextHandle < 0))
+    // Check contextHandle range
+    if((contextHandle >= (int)ARRAYSIZE(g_SSL_Driver.ContextArray)) || (contextHandle < 0))
     {
-        return FALSE;
+        return false;
     }
 
     // Retrieve SSL struct from g_SSL_Driver    
     // sd should already have been created
-    context = (mbedTLS_NFContext*)g_SSL_Driver.m_sslContextArray[sslContextHandle].SslContext;
+    context = (mbedTLS_NFContext*)g_SSL_Driver.ContextArray[contextHandle].Context;
 
     if (context != NULL)
     {
@@ -32,10 +36,13 @@ bool ssl_add_cert_auth_internal( int sslContextHandle, const char* certificate,
         if( mbedtls_x509_crt_parse(context->x509_crt, (const unsigned char*)certificate, certLength ) == 0)
         {
             // add to CA chain
-            mbedtls_ssl_conf_ca_chain( context->conf, context->x509_crt, NULL );
+            mbedtls_ssl_conf_ca_chain( 
+                context->conf, 
+                context->x509_crt, 
+                NULL );
 
             // done
-            return TRUE;
+            return true;
         }
     }
 

diff --git a/.../ssl/mbedTLS/ssl_closesocket_internal.cpp → ...ssl/mbedTLS/ssl_close_socket_internal.cpp b/.../ssl/mbedTLS/ssl_closesocket_internal.cpp → ...ssl/mbedTLS/ssl_close_socket_internal.cpp
@@ -6,21 +6,20 @@
 #include <ssl.h>
 #include "mbedtls.h"
 
-int  ssl_closesocket_internal( int sd )
+int ssl_close_socket_internal( int sd )
 {
     mbedTLS_NFContext* context= (mbedTLS_NFContext*)SOCKET_DRIVER.GetSocketSslData(sd);
-    mbedtls_ssl_context *ssl = context->ssl;
 
     // sanity check
-    if(ssl == NULL)
+    if(context != NULL)
     {
-        return SOCK_SOCKET_ERROR;
-    }
+        mbedtls_ssl_context *ssl = context->ssl;
+
+        // be nice and notify the peer that the connection is being closed
+        mbedtls_ssl_close_notify(ssl);
 
-    SOCKET_DRIVER.SetSocketSslData(sd, NULL);
-    SOCKET_DRIVER.UnregisterSocket(sd);
-
-    mbedtls_ssl_close_notify(ssl);
+        SOCKET_DRIVER.SetSocketSslData(sd, NULL);
+    }
 
     SOCK_close( sd );
 

diff --git a/src/PAL/COM/sockets/ssl/mbedTLS/ssl_connect_internal.cpp b/src/PAL/COM/sockets/ssl/mbedTLS/ssl_connect_internal.cpp
@@ -8,23 +8,26 @@
 #include <ssl.h>
 #include "mbedtls.h"
 
-int ssl_connect_internal(int sd, const char* szTargetHost, int sslContextHandle)
+int ssl_connect_internal(
+    int sd, 
+    const char* szTargetHost, 
+    int contextHandle)
 {
     mbedTLS_NFContext* context;
     int nonblock = 0;
 
     int ret = SOCK_SOCKET_ERROR;
 
-    // Check sslContextHandle range
-    if((sslContextHandle >= (int)ARRAYSIZE(g_SSL_Driver.m_sslContextArray)) || (sslContextHandle < 0))
+    // Check contextHandle range
+    if((contextHandle >= (int)ARRAYSIZE(g_SSL_Driver.ContextArray)) || (contextHandle < 0))
     {
         goto error;
     }
 
     // Retrieve SSL struct from g_SSL_Driver    
     // sd should already have been created
     // Now do the SSL negotiation
-    context = (mbedTLS_NFContext*)g_SSL_Driver.m_sslContextArray[sslContextHandle].SslContext;
+    context = (mbedTLS_NFContext*)g_SSL_Driver.ContextArray[contextHandle].Context;
     if (context == NULL)
     {
         return false;

diff --git a/src/PAL/COM/sockets/ssl/mbedTLS/ssl_exit_context_internal.cpp b/src/PAL/COM/sockets/ssl/mbedTLS/ssl_exit_context_internal.cpp
@@ -9,32 +9,38 @@
 #include "mbedtls.h"
 
 
-bool ssl_exit_context_internal(int sslContextHandle )
+bool ssl_exit_context_internal( int contextHandle )
 {
-    mbedTLS_NFContext* context = NULL;
+    mbedTLS_NFContext* context;
 
-    // Check sslContextHandle range
-    if((sslContextHandle >= (int)ARRAYSIZE(g_SSL_Driver.m_sslContextArray)) || (sslContextHandle < 0) || (g_SSL_Driver.m_sslContextArray[sslContextHandle].SslContext == NULL))
+    // Check contextHandle range
+    if((contextHandle >= (int)ARRAYSIZE(g_SSL_Driver.ContextArray)) || (contextHandle < 0) || (g_SSL_Driver.ContextArray[contextHandle].Context == NULL))
     {
-        return FALSE;
+        return false;
     }
 
-    context = (mbedTLS_NFContext*)g_SSL_Driver.m_sslContextArray[sslContextHandle].SslContext;
+    context = (mbedTLS_NFContext*)g_SSL_Driver.ContextArray[contextHandle].Context;
     if (context == NULL)
     {
-        return FALSE;
+        return false;
     }
 
     mbedtls_pk_free(context->pk);
     mbedtls_net_free(context->server_fd);
     mbedtls_ctr_drbg_free( context->ctr_drbg );
     mbedtls_entropy_free( context->entropy );
+    mbedtls_ssl_config_free( context->conf );
+    mbedtls_x509_crt_free(context->x509_crt);
+    mbedtls_ssl_free(context->ssl);
 
     // zero memory to wipe any security critical info in RAM
     memset(context->ssl, 0, sizeof(mbedtls_ssl_context));
 
     // free memory
-    platform_free(context->pk);
+    if(context->pk != NULL)
+    {
+        platform_free(context->pk);
+    }
     platform_free(context->server_fd);
     platform_free(context->entropy);
     platform_free(context->ctr_drbg);
@@ -43,9 +49,9 @@ bool ssl_exit_context_internal(int sslContextHandle )
     platform_free(context->ssl);
     platform_free(context);
 
-    NANOCLR_SSL_MEMSET(&g_SSL_Driver.m_sslContextArray[sslContextHandle], 0, sizeof(g_SSL_Driver.m_sslContextArray[sslContextHandle]));
+    memset(&g_SSL_Driver.ContextArray[contextHandle], 0, sizeof(g_SSL_Driver.ContextArray[contextHandle]));
 
-    g_SSL_Driver.m_sslContextCount --;
+    g_SSL_Driver.ContextCount --;
 
-    return TRUE;
+    return true;
 }
diff --git a/src/PAL/COM/sockets/ssl/mbedTLS/ssl_generic.cpp b/src/PAL/COM/sockets/ssl/mbedTLS/ssl_generic.cpp
@@ -12,7 +12,10 @@
 // this one lives in lwIPSocket.cpp
 extern int errno;
 
-int sslRecv(void *ctx, unsigned char *buf, size_t len)
+int sslRecv(
+    void *ctx, 
+    unsigned char *buf, 
+    size_t len)
 {
     (void)buf;
     (void)len;
@@ -31,8 +34,13 @@ int sslRecv(void *ctx, unsigned char *buf, size_t len)
     return 0;
 }
 
-// mbed TLS requires a function with this signature, so we are wrapping the call to our debug_printf here 
-void nf_debug( void *ctx, int level, const char *file, int line, const char *str )
+// mbedTLS requires a function with this signature, so we are wrapping the call to our debug_printf here 
+void nf_debug( 
+    void *ctx, 
+    int level, 
+    const char *file, 
+    int line, 
+    const char *str )
 {
     (void)level;
     (void)ctx;
@@ -71,7 +79,10 @@ int net_would_block( const mbedtls_net_context *ctx )
     return( 0 );
 }
 
-int mbedtls_net_recv( void *ctx, unsigned char *buf, size_t len )
+int mbedtls_net_recv( 
+    void *ctx, 
+    unsigned char *buf, 
+    size_t len )
 {
     int32_t ret;
     int32_t fd = ((mbedtls_net_context *) ctx)->fd;
@@ -106,7 +117,10 @@ int mbedtls_net_recv( void *ctx, unsigned char *buf, size_t len )
     return ret;
 }
 
-int mbedtls_net_send( void *ctx, const unsigned char *buf, size_t len )
+int mbedtls_net_send( 
+    void *ctx, 
+    const unsigned char *buf, 
+    size_t len )
 {
     int32_t ret;
     int fd = ((mbedtls_net_context *) ctx)->fd;
@@ -141,7 +155,11 @@ int mbedtls_net_send( void *ctx, const unsigned char *buf, size_t len )
     return ret;
 }
 
-int mbedtls_net_recv_timeout( void *ctx, unsigned char *buf, size_t len, uint32_t timeout )
+int mbedtls_net_recv_timeout( 
+    void *ctx, 
+    unsigned char *buf, 
+    size_t len, 
+    uint32_t timeout )
 {
     int ret;
     struct timeval tv;

diff --git a/src/PAL/COM/sockets/ssl/mbedTLS/ssl_generic_init_internal.cpp b/src/PAL/COM/sockets/ssl/mbedTLS/ssl_generic_init_internal.cpp
@@ -18,14 +18,15 @@ bool ssl_generic_init_internal(
     int privateKeyLength,
     const char* password,
     int passwordLength,
-    int& sslContextHandle, 
+    int& contextHandle, 
     bool isServer )
 {
     (void)sslMode;
 
     int sslContexIndex = -1;
     int authMode = MBEDTLS_SSL_VERIFY_NONE;
     int endpoint = 0;
+    int ret = 0;
 
     mbedtls_x509_crt* ownCertificate = NULL;
 
@@ -35,9 +36,9 @@ bool ssl_generic_init_internal(
     ///////////////////////
     mbedTLS_NFContext* context;
 
-    for(uint32_t i=0; i<ARRAYSIZE(g_SSL_Driver.m_sslContextArray); i++)
+    for(uint32_t i=0; i<ARRAYSIZE(g_SSL_Driver.ContextArray); i++)
     { 
-        if(g_SSL_Driver.m_sslContextArray[i].SslContext == NULL)
+        if(g_SSL_Driver.ContextArray[i].Context == NULL)
         {
             sslContexIndex = i;           
             break;
@@ -238,6 +239,11 @@ bool ssl_generic_init_internal(
         }
 
     }
+    else
+    {
+        // no PK, need to set it to NULL
+        context->pk = NULL;
+    }
 
     mbedtls_ssl_conf_ca_chain( context->conf, context->x509_crt, NULL );
 
@@ -256,22 +262,30 @@ bool ssl_generic_init_internal(
     mbedtls_ssl_conf_dbg( context->conf, nf_debug, stdout );
   #endif
 
-    if( mbedtls_ssl_setup( context->ssl, context->conf ) != 0 )
+    ret = mbedtls_ssl_setup( context->ssl, context->conf );
+    if( ret  != 0 )
     {
         // ssl_setup_failed
         goto error;
     }
 
     //////////////////////////////////////
 
-    g_SSL_Driver.m_sslContextArray[sslContexIndex].SslContext = context;
-    g_SSL_Driver.m_sslContextCount++;
+    g_SSL_Driver.ContextArray[sslContexIndex].Context = context;
+    g_SSL_Driver.ContextCount++;
 
-    sslContextHandle = sslContexIndex;
+    contextHandle = sslContexIndex;
 
-    return TRUE;
+    return true;
 
 error:
+    mbedtls_pk_free(context->pk);
+    mbedtls_net_free(context->server_fd);
+    mbedtls_ctr_drbg_free( context->ctr_drbg );
+    mbedtls_entropy_free( context->entropy );
+    mbedtls_x509_crt_free(context->x509_crt);
+    mbedtls_ssl_config_free( context->conf );
+    mbedtls_ssl_free(context->ssl);
 
     // check for any memory allocation that needs to be freed before exiting
     if(context->ssl != NULL) platform_free(context->ssl);
@@ -281,7 +295,16 @@ bool ssl_generic_init_internal(
     if(context->server_fd != NULL) platform_free(context->server_fd);
     if(context->x509_crt != NULL) platform_free(context->x509_crt);
     if(context->pk != NULL) platform_free(context->pk);
-    if(ownCertificate != NULL) mbedtls_x509_crt_free( ownCertificate );
+
+    if(ownCertificate != NULL)
+    {
+        mbedtls_x509_crt_free( ownCertificate );
+        platform_free(ownCertificate);
+    }
+    if(context->pk != NULL)
+    {
+        platform_free(context);
+    }
 
-    return FALSE;
+    return false;
 }