Fix handling of own certificate in SSL context (#3140)

josesimoes · web-flow · commit 75ce2fd23487 · 2025-04-04T11:49:07.000+01:00
diff --git a/src/HAL/Include/nanoHAL_Network.h b/src/HAL/Include/nanoHAL_Network.h
@@ -300,10 +300,10 @@ typedef struct __nfpack HAL_Configuration_X509DeviceCertificate
     // this is the marker placeholder for this configuration block
     uint8_t Marker[4];
 
-    // Size of the X509 CA Root certificate bundle
+    // Size of the X509 device certificate
     uint32_t CertificateSize;
 
-    // X509 CA Root certificate bundle
+    // X509 device certificate
     uint8_t Certificate[1];
 
 } HAL_Configuration_X509DeviceCertificate;
diff --git a/src/PAL/COM/sockets/ssl/MbedTLS/mbedtls.h b/src/PAL/COM/sockets/ssl/MbedTLS/mbedtls.h
@@ -34,7 +34,8 @@ extern "C"
         mbedtls_ssl_config *conf;
         mbedtls_ssl_context *ssl;
         mbedtls_net_context *server_fd;
-        mbedtls_x509_crt *x509_crt;
+        mbedtls_x509_crt *ca_cert;
+        mbedtls_x509_crt *own_cert;
         mbedtls_pk_context *pk;
     } mbedTLS_NFContext;
 
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
@@ -25,12 +25,12 @@ bool ssl_add_cert_auth_internal(int contextHandle, const char *certificate, int
         /////////////////////////////////////////////////////////////////////////////////////////////////
         // developer notes:                                                                            //
         // this call parses certificates in both string and binary formats                             //
-        // when the formart is a string it has to include the terminator otherwise the parse will fail //
+        // when the format is a string it has to include the terminator otherwise the parse will fail //
         /////////////////////////////////////////////////////////////////////////////////////////////////
-        if (mbedtls_x509_crt_parse(context->x509_crt, (const unsigned char *)certificate, certLength) == 0)
+        if (mbedtls_x509_crt_parse(context->ca_cert, (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->ca_cert, NULL);
 
             // done
             return true;
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
@@ -30,24 +30,58 @@ bool ssl_exit_context_internal(int contextHandle)
     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_x509_crt_free(context->ca_cert);
+    mbedtls_x509_crt_free(context->own_cert);
     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
-    if (context->pk != NULL)
+    if (context->pk)
     {
         platform_free(context->pk);
     }
-    platform_free(context->server_fd);
-    platform_free(context->entropy);
-    platform_free(context->ctr_drbg);
-    platform_free(context->conf);
-    platform_free(context->x509_crt);
-    platform_free(context->ssl);
-    platform_free(context);
+
+    if (context->server_fd)
+    {
+        platform_free(context->server_fd);
+    }
+
+    if (context->entropy)
+    {
+        platform_free(context->entropy);
+    }
+
+    if (context->ctr_drbg)
+    {
+        platform_free(context->ctr_drbg);
+    }
+
+    if (context->conf)
+    {
+        platform_free(context->conf);
+    }
+
+    if (context->ca_cert)
+    {
+        platform_free(context->ca_cert);
+    }
+
+    if (context->own_cert)
+    {
+        platform_free(context->own_cert);
+    }
+
+    if (context->ssl)
+    {
+        platform_free(context->ssl);
+    }
+
+    if (context)
+    {
+        platform_free(context);
+    }
 
     memset(&g_SSL_Driver.ContextArray[contextHandle], 0, sizeof(g_SSL_Driver.ContextArray[contextHandle]));
 
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
@@ -32,7 +32,6 @@ bool ssl_generic_init_internal(
     int endpoint = 0;
     int ret = 0;
 
-    mbedtls_x509_crt *ownCertificate = NULL;
     HAL_Configuration_X509CaRootBundle *certStore = NULL;
     HAL_Configuration_X509DeviceCertificate *deviceCert = NULL;
 
@@ -64,6 +63,7 @@ bool ssl_generic_init_internal(
     memset(context, 0, sizeof(mbedTLS_NFContext));
 
     // allocate memory for net context
+    // this needs to be freed in ssl_exit_context_internal
     context->server_fd = (mbedtls_net_context *)platform_malloc(sizeof(mbedtls_net_context));
     if (context->server_fd == NULL)
     {
@@ -130,12 +130,12 @@ bool ssl_generic_init_internal(
 
     // create and init X509 CRT
     // this needs to be freed in ssl_exit_context_internal
-    context->x509_crt = (mbedtls_x509_crt *)platform_malloc(sizeof(mbedtls_x509_crt));
-    if (context->x509_crt == NULL)
+    context->ca_cert = (mbedtls_x509_crt *)platform_malloc(sizeof(mbedtls_x509_crt));
+    if (context->ca_cert == NULL)
     {
         goto error;
     }
-    mbedtls_x509_crt_init(context->x509_crt);
+    mbedtls_x509_crt_init(context->ca_cert);
 
     // TODO: review if we can add some instance-unique data to the custom argument below
     if (mbedtls_ctr_drbg_seed(context->ctr_drbg, mbedtls_entropy_func, context->entropy, NULL, 0) != 0)
@@ -207,11 +207,12 @@ bool ssl_generic_init_internal(
             // when the format is a string it has to include the terminator otherwise the parse will fail //
             /////////////////////////////////////////////////////////////////////////////////////////////////
             mbedtls_x509_crt_parse(
-                context->x509_crt,
+                context->ca_cert,
                 (const unsigned char *)certStore->Certificate,
                 certStore->CertificateSize);
 
             platform_free(certStore);
+            certStore = NULL;
         }
     }
 
@@ -260,21 +261,22 @@ bool ssl_generic_init_internal(
         }
 
         // parse certificate
-        ownCertificate = (mbedtls_x509_crt *)platform_malloc(sizeof(mbedtls_x509_crt));
-        if (ownCertificate == NULL)
+        // this needs to be freed in ssl_exit_context_internal
+        context->own_cert = (mbedtls_x509_crt *)platform_malloc(sizeof(mbedtls_x509_crt));
+        if (context->own_cert == NULL)
         {
             goto error;
         }
 
-        mbedtls_x509_crt_init(ownCertificate);
+        mbedtls_x509_crt_init(context->own_cert);
 
-        if (mbedtls_x509_crt_parse(ownCertificate, (const unsigned char *)certificate, certLength))
+        if (mbedtls_x509_crt_parse(context->own_cert, (const unsigned char *)certificate, certLength))
         {
             // failed parsing own certificate failed
             goto error;
         }
 
-        if (mbedtls_ssl_conf_own_cert(context->conf, ownCertificate, context->pk))
+        if (mbedtls_ssl_conf_own_cert(context->conf, context->own_cert, context->pk))
         {
             // configuring own certificate failed
             goto error;
@@ -284,6 +286,7 @@ bool ssl_generic_init_internal(
         if (deviceCert)
         {
             platform_free(deviceCert);
+            deviceCert = NULL;
         }
     }
     else
@@ -294,7 +297,7 @@ bool ssl_generic_init_internal(
         context->pk = NULL;
     }
 
-    mbedtls_ssl_conf_ca_chain(context->conf, context->x509_crt, NULL);
+    mbedtls_ssl_conf_ca_chain(context->conf, context->ca_cert, NULL);
 
     psa_crypto_init();
 
@@ -343,7 +346,8 @@ bool ssl_generic_init_internal(
 
     mbedtls_ctr_drbg_free(context->ctr_drbg);
     mbedtls_entropy_free(context->entropy);
-    mbedtls_x509_crt_free(context->x509_crt);
+    mbedtls_x509_crt_free(context->ca_cert);
+    mbedtls_x509_crt_free(context->own_cert);
     mbedtls_ssl_config_free(context->conf);
     mbedtls_ssl_free(context->ssl);
 
@@ -373,21 +377,21 @@ bool ssl_generic_init_internal(
         platform_free(context->server_fd);
     }
 
-    if (context->x509_crt)
+    if (context->ca_cert)
     {
-        platform_free(context->x509_crt);
+        platform_free(context->ca_cert);
     }
 
-    if (context->pk)
+    if (context->own_cert)
     {
-        platform_free(context->pk);
+        platform_free(context->own_cert);
     }
 
-    if (ownCertificate)
+    if (context->pk)
     {
-        mbedtls_x509_crt_free(ownCertificate);
-        platform_free(ownCertificate);
+        platform_free(context->pk);
     }
+
     if (context)
     {
         platform_free(context);

Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,6 @@ bool ssl_generic_init_internal(`
`32`	`32`	`int endpoint = 0;`
`33`	`33`	`int ret = 0;`
`34`	`34`
`35`		`- mbedtls_x509_crt *ownCertificate = NULL;`
`36`	`35`	`HAL_Configuration_X509CaRootBundle *certStore = NULL;`
`37`	`36`	`HAL_Configuration_X509DeviceCertificate *deviceCert = NULL;`
`38`	`37`
`@@ -64,6 +63,7 @@ bool ssl_generic_init_internal(`
`64`	`63`	`memset(context, 0, sizeof(mbedTLS_NFContext));`
`65`	`64`
`66`	`65`	`// allocate memory for net context`
	`66`	`+ // this needs to be freed in ssl_exit_context_internal`
`67`	`67`	`context->server_fd = (mbedtls_net_context *)platform_malloc(sizeof(mbedtls_net_context));`
`68`	`68`	`if (context->server_fd == NULL)`
`69`	`69`	`{`
`@@ -130,12 +130,12 @@ bool ssl_generic_init_internal(`
`130`	`130`
`131`	`131`	`// create and init X509 CRT`
`132`	`132`	`// this needs to be freed in ssl_exit_context_internal`
`133`		`- context->x509_crt = (mbedtls_x509_crt *)platform_malloc(sizeof(mbedtls_x509_crt));`
`134`		`- if (context->x509_crt == NULL)`
	`133`	`+ context->ca_cert = (mbedtls_x509_crt *)platform_malloc(sizeof(mbedtls_x509_crt));`
	`134`	`+ if (context->ca_cert == NULL)`
`135`	`135`	`{`
`136`	`136`	`goto error;`
`137`	`137`	`}`
`138`		`- mbedtls_x509_crt_init(context->x509_crt);`
	`138`	`+ mbedtls_x509_crt_init(context->ca_cert);`
`139`	`139`
`140`	`140`	`// TODO: review if we can add some instance-unique data to the custom argument below`
`141`	`141`	`if (mbedtls_ctr_drbg_seed(context->ctr_drbg, mbedtls_entropy_func, context->entropy, NULL, 0) != 0)`
`@@ -207,11 +207,12 @@ bool ssl_generic_init_internal(`
`207`	`207`	`// when the format is a string it has to include the terminator otherwise the parse will fail //`
`208`	`208`	`/////////////////////////////////////////////////////////////////////////////////////////////////`
`209`	`209`	`mbedtls_x509_crt_parse(`
`210`		`- context->x509_crt,`
	`210`	`+ context->ca_cert,`
`211`	`211`	`(const unsigned char *)certStore->Certificate,`
`212`	`212`	`certStore->CertificateSize);`
`213`	`213`
`214`	`214`	`platform_free(certStore);`
	`215`	`+ certStore = NULL;`
`215`	`216`	`}`
`216`	`217`	`}`
`217`	`218`
`@@ -260,21 +261,22 @@ bool ssl_generic_init_internal(`
`260`	`261`	`}`
`261`	`262`
`262`	`263`	`// parse certificate`
`263`		`- ownCertificate = (mbedtls_x509_crt *)platform_malloc(sizeof(mbedtls_x509_crt));`
`264`		`- if (ownCertificate == NULL)`
	`264`	`+ // this needs to be freed in ssl_exit_context_internal`
	`265`	`+ context->own_cert = (mbedtls_x509_crt *)platform_malloc(sizeof(mbedtls_x509_crt));`
	`266`	`+ if (context->own_cert == NULL)`
`265`	`267`	`{`
`266`	`268`	`goto error;`
`267`	`269`	`}`
`268`	`270`
`269`		`- mbedtls_x509_crt_init(ownCertificate);`
	`271`	`+ mbedtls_x509_crt_init(context->own_cert);`
`270`	`272`
`271`		`- if (mbedtls_x509_crt_parse(ownCertificate, (const unsigned char *)certificate, certLength))`
	`273`	`+ if (mbedtls_x509_crt_parse(context->own_cert, (const unsigned char *)certificate, certLength))`
`272`	`274`	`{`
`273`	`275`	`// failed parsing own certificate failed`
`274`	`276`	`goto error;`
`275`	`277`	`}`
`276`	`278`
`277`		`- if (mbedtls_ssl_conf_own_cert(context->conf, ownCertificate, context->pk))`
	`279`	`+ if (mbedtls_ssl_conf_own_cert(context->conf, context->own_cert, context->pk))`
`278`	`280`	`{`
`279`	`281`	`// configuring own certificate failed`
`280`	`282`	`goto error;`
`@@ -284,6 +286,7 @@ bool ssl_generic_init_internal(`
`284`	`286`	`if (deviceCert)`
`285`	`287`	`{`
`286`	`288`	`platform_free(deviceCert);`
	`289`	`+ deviceCert = NULL;`
`287`	`290`	`}`
`288`	`291`	`}`
`289`	`292`	`else`
`@@ -294,7 +297,7 @@ bool ssl_generic_init_internal(`
`294`	`297`	`context->pk = NULL;`
`295`	`298`	`}`
`296`	`299`
`297`		`- mbedtls_ssl_conf_ca_chain(context->conf, context->x509_crt, NULL);`
	`300`	`+ mbedtls_ssl_conf_ca_chain(context->conf, context->ca_cert, NULL);`
`298`	`301`
`299`	`302`	`psa_crypto_init();`
`300`	`303`
`@@ -343,7 +346,8 @@ bool ssl_generic_init_internal(`
`343`	`346`
`344`	`347`	`mbedtls_ctr_drbg_free(context->ctr_drbg);`
`345`	`348`	`mbedtls_entropy_free(context->entropy);`
`346`		`- mbedtls_x509_crt_free(context->x509_crt);`
	`349`	`+ mbedtls_x509_crt_free(context->ca_cert);`
	`350`	`+ mbedtls_x509_crt_free(context->own_cert);`
`347`	`351`	`mbedtls_ssl_config_free(context->conf);`
`348`	`352`	`mbedtls_ssl_free(context->ssl);`
`349`	`353`
`@@ -373,21 +377,21 @@ bool ssl_generic_init_internal(`
`373`	`377`	`platform_free(context->server_fd);`
`374`	`378`	`}`
`375`	`379`
`376`		`- if (context->x509_crt)`
	`380`	`+ if (context->ca_cert)`
`377`	`381`	`{`
`378`		`- platform_free(context->x509_crt);`
	`382`	`+ platform_free(context->ca_cert);`
`379`	`383`	`}`
`380`	`384`
`381`		`- if (context->pk)`
	`385`	`+ if (context->own_cert)`
`382`	`386`	`{`
`383`		`- platform_free(context->pk);`
	`387`	`+ platform_free(context->own_cert);`
`384`	`388`	`}`
`385`	`389`
`386`		`- if (ownCertificate)`
	`390`	`+ if (context->pk)`
`387`	`391`	`{`
`388`		`- mbedtls_x509_crt_free(ownCertificate);`
`389`		`- platform_free(ownCertificate);`
	`392`	`+ platform_free(context->pk);`
`390`	`393`	`}`
	`394`	`+`
`391`	`395`	`if (context)`
`392`	`396`	`{`
`393`	`397`	`platform_free(context);`