From dba44fd8ce7219897966054446c992184369fbfa Mon Sep 17 00:00:00 2001 From: Oleksandr Dmytrenko Date: Tue, 21 Mar 2017 15:40:52 +0200 Subject: [PATCH] [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 Reviewed-on: https://gerrit.iotivity.org/gerrit/18057 Reviewed-by: Andrii Shtompel Reviewed-by: Kevin Kane Tested-by: jenkins-iotivity Reviewed-by: Greg Zaverucha --- resource/c_common/byte_array.h | 13 ++++++ .../csdk/connectivity/api/casecurityinterface.h | 4 ++ .../src/adapter_util/ca_adapter_net_ssl.c | 49 ++++++++++++++++++---- .../csdk/connectivity/test/ssladapter_test.cpp | 17 ++++++-- 4 files changed, 71 insertions(+), 12 deletions(-) diff --git a/resource/c_common/byte_array.h b/resource/c_common/byte_array.h index a736764..2810112 100644 --- 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 5851033..1c0b862 100644 --- a/resource/csdk/connectivity/api/casecurityinterface.h +++ b/resource/csdk/connectivity/api/casecurityinterface.h @@ -112,6 +112,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 11089d1..f753140 100644 --- a/resource/csdk/connectivity/src/adapter_util/ca_adapter_net_ssl.c +++ b/resource/csdk/connectivity/src/adapter_util/ca_adapter_net_ssl.c @@ -67,7 +67,6 @@ #include #endif - /** * @def MBED_TLS_VERSION_LEN * @brief mbedTLS version string length @@ -278,8 +277,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; @@ -620,15 +617,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); @@ -652,7 +680,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"); @@ -663,7 +691,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) { @@ -701,11 +729,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) @@ -713,7 +742,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"); @@ -725,6 +754,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; } @@ -2081,7 +2112,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 83f411c..cdfc9f1 100644 --- a/resource/csdk/connectivity/test/ssladapter_test.cpp +++ b/resource/csdk/connectivity/test/ssladapter_test.cpp @@ -894,12 +894,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; } -- 2.7.4