[IOT-1917]Fix memory leak: cert/key/CRL

Fix memory leak: cert/key/CRL information returned by cred resource
https://jira.iotivity.org/browse/IOT-1917

Change-Id: Ic563b5e5b79ccac8855ebb5b215e475d1b4e57be
Signed-off-by: Oleksandr Dmytrenko <o.dmytrenko@samsung.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/18057
Reviewed-by: Andrii Shtompel <a.shtompel@samsung.com>
Reviewed-by: Kevin Kane <kkane@microsoft.com>
Tested-by: jenkins-iotivity <jenkins@iotivity.org>
Reviewed-by: Greg Zaverucha <gregz@microsoft.com>
(cherry picked from commit dba44fd8ce7219897966054446c992184369fbfa)
Reviewed-on: https://gerrit.iotivity.org/gerrit/20933
Reviewed-by: Phil Coval <philippe.coval@osg.samsung.com>

diff --git a/resource/c_common/byte_array.h b/resource/c_common/byte_array.h
index a736764..2810112 100644 (file)
--- a/resource/c_common/byte_array.h
+++ b/resource/c_common/byte_array.h
@@ -65,6 +65,19 @@ typedef struct ByteArray
         (array).len = 0;            \
     }while(0)
 
+/**@def DEINIT_BYTE_ARRAY(array)
+ *
+ * Deinitializes of existing byte array \a array.
+ *
+ * @param array ByteArray_t
+ */
+#undef DEINIT_BYTE_ARRAY
+#define DEINIT_BYTE_ARRAY(array) do{  \
+        OICFree((array).data);       \
+        (array).data = NULL;        \
+        (array).len = 0;            \
+    }while(0)
+
 /**@def PRINT_BYTE_ARRAY(msg, array)
  *
  * Prints out byte array \a array in hex representation with message \a msg.

diff --git a/resource/csdk/connectivity/api/casecurityinterface.h b/resource/csdk/connectivity/api/casecurityinterface.h
index 1fee457..7705ea3 100644 (file)
--- a/resource/csdk/connectivity/api/casecurityinterface.h
+++ b/resource/csdk/connectivity/api/casecurityinterface.h
@@ -111,6 +111,10 @@ typedef void (*CAgetCredentialTypesHandler)(bool * list, const char* deviceId);
 /**
  * Binary structure containing PKIX related info
  * own certificate chain, public key, CA's and CRL's
+ * The data member of each ByteArray_t must be allocated with OICMalloc or OICCalloc.
+ * The SSL adapter takes ownership of this memory and will free it internally after use.
+ * Callers should not reference this memory after it has been provided to the SSL adapter via the
+ * callback.
  */
 typedef struct
 {

diff --git a/resource/csdk/connectivity/src/adapter_util/ca_adapter_net_ssl.c b/resource/csdk/connectivity/src/adapter_util/ca_adapter_net_ssl.c
index bcaf043..56bb853 100644 (file)
--- a/resource/csdk/connectivity/src/adapter_util/ca_adapter_net_ssl.c
+++ b/resource/csdk/connectivity/src/adapter_util/ca_adapter_net_ssl.c
@@ -69,7 +69,6 @@
 #include <unistd.h>
 #endif
 
-
 /**
  * @def MBED_TLS_VERSION_LEN
  * @brief mbedTLS version string length
@@ -284,8 +283,6 @@ mbedtls_ecp_group_id curve[ADAPTER_CURVE_MAX][2] =
     {MBEDTLS_ECP_DP_SECP256R1, MBEDTLS_ECP_DP_NONE}
 };
 
-static PkiInfo_t g_pkiInfo = {{NULL, 0}, {NULL, 0}, {NULL, 0}, {NULL, 0}};
-
 typedef struct  {
     int code;
     unsigned char alert;
@@ -626,15 +623,46 @@ static int ParseChain(mbedtls_x509_crt * crt, unsigned char * buf, size_t bufLen
     return ret;
 }
 
+/**
+ * Deinit Pki Info
+ *
+ * @param[out] inf structure with certificate, private key and crl to be free.
+ *
+ */
+static void DeInitPkixInfo(PkiInfo_t * inf)
+{
+    OIC_LOG_V(DEBUG, NET_SSL_TAG, "In %s", __func__);
+    if (NULL == inf)
+    {
+        OIC_LOG(ERROR, NET_SSL_TAG, "NULL passed");
+        OIC_LOG_V(DEBUG, NET_SSL_TAG, "Out %s", __func__);
+        return;
+    }
+
+    DEINIT_BYTE_ARRAY(inf->crt);
+    DEINIT_BYTE_ARRAY(inf->key);
+    DEINIT_BYTE_ARRAY(inf->ca);
+    DEINIT_BYTE_ARRAY(inf->crl);
+
+    OIC_LOG_V(DEBUG, NET_SSL_TAG, "Out %s", __func__);
+}
+
 //Loads PKIX related information from SRM
 static int InitPKIX(CATransportAdapter_t adapter)
 {
     OIC_LOG_V(DEBUG, NET_SSL_TAG, "In %s", __func__);
     VERIFY_NON_NULL_RET(g_getPkixInfoCallback, NET_SSL_TAG, "PKIX info callback is NULL", -1);
     // load pk key, cert, trust chain and crl
+    PkiInfo_t pkiInfo = {
+        BYTE_ARRAY_INITIALIZER,
+        BYTE_ARRAY_INITIALIZER,
+        BYTE_ARRAY_INITIALIZER,
+        BYTE_ARRAY_INITIALIZER
+    };
+
     if (g_getPkixInfoCallback)
     {
-        g_getPkixInfoCallback(&g_pkiInfo);
+        g_getPkixInfoCallback(&pkiInfo);
     }
 
     VERIFY_NON_NULL_RET(g_caSslContext, NET_SSL_TAG, "SSL Context is NULL", -1);
@@ -658,7 +686,7 @@ static int InitPKIX(CATransportAdapter_t adapter)
     // optional
     int ret;
     int errNum;
-    int count = ParseChain(&g_caSslContext->crt, g_pkiInfo.crt.data, g_pkiInfo.crt.len, &errNum);
+    int count = ParseChain(&g_caSslContext->crt, pkiInfo.crt.data, pkiInfo.crt.len, &errNum);
     if (0 >= count)
     {
         OIC_LOG(WARNING, NET_SSL_TAG, "Own certificate chain parsing error");
@@ -669,7 +697,7 @@ static int InitPKIX(CATransportAdapter_t adapter)
         OIC_LOG_V(WARNING, NET_SSL_TAG, "Own certificate chain parsing error: %d certs failed to parse", errNum);
         goto required;
     }
-    ret =  mbedtls_pk_parse_key(&g_caSslContext->pkey, g_pkiInfo.key.data, g_pkiInfo.key.len,
+    ret =  mbedtls_pk_parse_key(&g_caSslContext->pkey, pkiInfo.key.data, pkiInfo.key.len,
                                                                                NULL, 0);
     if (0 != ret)
     {
@@ -707,11 +735,12 @@ static int InitPKIX(CATransportAdapter_t adapter)
     }
 
     required:
-    count = ParseChain(&g_caSslContext->ca, g_pkiInfo.ca.data, g_pkiInfo.ca.len, &errNum);
+    count = ParseChain(&g_caSslContext->ca, pkiInfo.ca.data, pkiInfo.ca.len, &errNum);
     if(0 >= count)
     {
         OIC_LOG(ERROR, NET_SSL_TAG, "CA chain parsing error");
         OIC_LOG_V(DEBUG, NET_SSL_TAG, "Out %s", __func__);
+        DeInitPkixInfo(&pkiInfo);
         return -1;
     }
     if(0 != errNum)
@@ -719,7 +748,7 @@ static int InitPKIX(CATransportAdapter_t adapter)
         OIC_LOG_V(WARNING, NET_SSL_TAG, "CA chain parsing warning: %d certs failed to parse", errNum);
     }
 
-    ret = mbedtls_x509_crl_parse_der(&g_caSslContext->crl, g_pkiInfo.crl.data, g_pkiInfo.crl.len);
+    ret = mbedtls_x509_crl_parse_der(&g_caSslContext->crl, pkiInfo.crl.data, pkiInfo.crl.len);
     if(0 != ret)
     {
         OIC_LOG(WARNING, NET_SSL_TAG, "CRL parsing error");
@@ -731,6 +760,8 @@ static int InitPKIX(CATransportAdapter_t adapter)
                  &g_caSslContext->ca, &g_caSslContext->crl);
     }
 
+    DeInitPkixInfo(&pkiInfo);
+
     OIC_LOG_V(DEBUG, NET_SSL_TAG, "Out %s", __func__);
     return 0;
 }
@@ -2123,7 +2154,7 @@ CAResult_t CAdecryptSsl(const CASecureEndpoint_t *sep, uint8_t *data, size_t dat
                     /* If UUID_PREFIX is present, ensure there's enough data for the prefix plus an entire
                      * UUID, to make sure we don't read past the end of the buffer.
                      */
-                    if ((NULL != uuidPos) && 
+                    if ((NULL != uuidPos) &&
                         (name->val.len >= ((uuidPos - name->val.p) + (sizeof(UUID_PREFIX) - 1) + uuidBufLen)))
                     {
                         memcpy(uuid, uuidPos + sizeof(UUID_PREFIX) - 1, uuidBufLen);

diff --git a/resource/csdk/connectivity/test/ssladapter_test.cpp b/resource/csdk/connectivity/test/ssladapter_test.cpp
index a27e9ee..e02a04d 100644 (file)
--- a/resource/csdk/connectivity/test/ssladapter_test.cpp
+++ b/resource/csdk/connectivity/test/ssladapter_test.cpp
@@ -913,12 +913,23 @@ static void PacketReceive(unsigned char *data, int * datalen)
 
 static void infoCallback_that_loads_x509(PkiInfo_t * inf)
 {
-    inf->crt.data = (uint8_t*)serverCert;
     inf->crt.len = sizeof(serverCert);
-    inf->key.data = (uint8_t*)serverPrivateKey;
+    inf->crt.data = (uint8_t*)OICMalloc(inf->crt.len);
+    ASSERT_TRUE(inf->crt.data != NULL);
+    memcpy(inf->crt.data, serverCert, inf->crt.len);
+
     inf->key.len = sizeof(serverPrivateKey);
-    inf->ca.data = (uint8_t*)caCert;
+    inf->key.data = (uint8_t*)OICMalloc(inf->key.len);
+    ASSERT_TRUE(inf->key.data != NULL);
+    memcpy(inf->key.data, serverPrivateKey, inf->key.len);
+
+
     inf->ca.len = sizeof(caCert);
+    inf->ca.data = (uint8_t*)OICMalloc(inf->ca.len);
+    ASSERT_TRUE(inf->ca.data != NULL);
+    memcpy(inf->ca.data, caCert, inf->ca.len);
+
+
     inf->crl.data = NULL;
     inf->crl.len = 0;
 }