From 916ced6413b93c3c4448fb4124be429c0f13bb82 Mon Sep 17 00:00:00 2001 From: Kevin Kane Date: Fri, 4 Nov 2016 13:53:34 -0700 Subject: [PATCH] [IOT-1519] Securely zero buffers containing secret data Add an OICClearMemory helper function, and use it to securely clear buffers that contain keys and other secret data that shouldn't be left in the stack or on the heap. Rename privateKey to g_privateKey in csr.c. Fix a couple of leaked payloads on error return paths in secureresourceprovider.c (which will also now zero their contents). Change-Id: If79c840ad758be2a7ca1bf7e6ccccb6dbdc39cf2 Signed-off-by: Kevin Kane Reviewed-on: https://gerrit.iotivity.org/gerrit/14091 Tested-by: jenkins-iotivity Reviewed-by: Uze Choi Reviewed-by: Randeep Singh --- resource/c_common/oic_malloc/include/oic_malloc.h | 14 ++++++++++++ resource/c_common/oic_malloc/src/oic_malloc.c | 21 ++++++++++++++++++ .../csdk/security/provisioning/src/cloud/csr.c | 25 +++++++++++----------- .../provisioning/src/credentialgenerator.c | 1 + .../provisioning/src/ownershiptransfermanager.c | 1 + .../provisioning/src/secureresourceprovider.c | 3 +++ resource/csdk/security/src/credresource.c | 21 +++++++++++++++--- resource/csdk/security/src/dpairingresource.c | 1 + resource/csdk/stack/src/ocpayload.c | 1 + 9 files changed, 73 insertions(+), 15 deletions(-) diff --git a/resource/c_common/oic_malloc/include/oic_malloc.h b/resource/c_common/oic_malloc/include/oic_malloc.h index dc7ae45..68588ff 100644 --- a/resource/c_common/oic_malloc/include/oic_malloc.h +++ b/resource/c_common/oic_malloc/include/oic_malloc.h @@ -111,6 +111,20 @@ void *OICCalloc(size_t num, size_t size); */ void OICFree(void *ptr); +/** + * Securely zero the contents of a memory buffer in a way that won't be + * optimized out by the compiler. Do not use memset for this purpose, because + * compilers may optimize out such calls. For more information on this danger, see: + * https://www.securecoding.cert.org/confluence/display/c/MSC06-C.+Beware+of+compiler+optimizations + * + * This function should be used on buffers containing secret data, such as + * cryptographic keys. + * + * @param buf - Pointer to a block of memory to be zeroed. If NULL, nothing happens. + * @param n - Size of buf in bytes. If zero, nothing happens. + */ +void OICClearMemory(void *buf, size_t n); + #ifdef __cplusplus } #endif // __cplusplus diff --git a/resource/c_common/oic_malloc/src/oic_malloc.c b/resource/c_common/oic_malloc/src/oic_malloc.c index 7175f40..d220121 100644 --- a/resource/c_common/oic_malloc/src/oic_malloc.c +++ b/resource/c_common/oic_malloc/src/oic_malloc.c @@ -24,6 +24,12 @@ #include #include "oic_malloc.h" +#include "iotivity_config.h" + +#ifdef HAVE_WINDOWS_H +#include +#endif + // Enable extra debug logging for malloc. Comment out to disable #ifdef ENABLE_MALLOC_DEBUG #include "logger.h" @@ -134,3 +140,18 @@ void OICFree(void *ptr) free(ptr); } + +void OICClearMemory(void *buf, size_t n) +{ + if (NULL != buf) { +#ifdef HAVE_WINDOWS_H + SecureZeroMemory(buf, n); +#else + volatile unsigned char* p = (volatile unsigned char*)buf; + while (n--) + { + *p++ = 0; + } +#endif + } +} \ No newline at end of file diff --git a/resource/csdk/security/provisioning/src/cloud/csr.c b/resource/csdk/security/provisioning/src/cloud/csr.c index eab8412..1c4fcd1 100644 --- a/resource/csdk/security/provisioning/src/cloud/csr.c +++ b/resource/csdk/security/provisioning/src/cloud/csr.c @@ -52,7 +52,7 @@ #define TAG "OIC_CLOUD_CSR" //TODO: is it required in CSR response? -static OCByteString privateKey = {0, 0}; +static OCByteString g_privateKey = {0, 0}; #define MAX_URI_QUERY MAX_URI_LENGTH + MAX_QUERY_LENGTH @@ -299,15 +299,15 @@ static int GenerateCSR(char *subject, OCByteString *csr) OIC_LOG_V(DEBUG, TAG, "Out %s", __func__); return -1; } - privateKey.bytes = (uint8_t *)OICMalloc(ret * sizeof(char)); - if (NULL == privateKey.bytes) + g_privateKey.bytes = (uint8_t *)OICMalloc(ret * sizeof(char)); + if (NULL == g_privateKey.bytes) { - OIC_LOG(ERROR, TAG, "OICMalloc returned NULL on privateKey.bytes allocation"); + OIC_LOG(ERROR, TAG, "OICMalloc returned NULL on g_privateKey.bytes allocation"); OIC_LOG_V(DEBUG, TAG, "Out %s", __func__); return -1; } - memcpy(privateKey.bytes, buf + bufsize - ret, ret * sizeof(uint8_t)); - privateKey.len = ret; + memcpy(g_privateKey.bytes, buf + bufsize - ret, ret * sizeof(uint8_t)); + g_privateKey.len = ret; // Public key to output ret = mbedtls_pk_write_pubkey_der(key, buf, bufsize); if (ret < 0) @@ -388,8 +388,8 @@ static OCStackResult HandleCertificateIssueRequest(void *ctx, void **data, OCCli { OicSecKey_t key = { - privateKey.bytes, - privateKey.len, + g_privateKey.bytes, + g_privateKey.len, OIC_ENCODING_DER }; @@ -425,9 +425,10 @@ static OCStackResult HandleCertificateIssueRequest(void *ctx, void **data, OCCli } } - OICFree(privateKey.bytes); - privateKey.bytes = NULL; - privateKey.len = 0; + OICClearMemory(g_privateKey.bytes, g_privateKey.len); + OICFree(g_privateKey.bytes); + g_privateKey.bytes = NULL; + g_privateKey.len = 0; OIC_LOG_V(DEBUG, TAG, "OUT: %s", __func__); @@ -472,7 +473,7 @@ OCStackResult OCCloudCertificateIssueRequest(void* ctx, OIC_LOG_BUFFER(DEBUG, TAG, request.bytes, request.len); OIC_LOG(DEBUG, TAG, "Private Key:"); - OIC_LOG_BUFFER(DEBUG, TAG, privateKey.bytes, privateKey.len); + OIC_LOG_BUFFER(DEBUG, TAG, g_privateKey.bytes, g_privateKey.len); OCRepPayload* payload = OCRepPayloadCreate(); if (!payload) diff --git a/resource/csdk/security/provisioning/src/credentialgenerator.c b/resource/csdk/security/provisioning/src/credentialgenerator.c index 623ad74..a6ee58e 100644 --- a/resource/csdk/security/provisioning/src/credentialgenerator.c +++ b/resource/csdk/security/provisioning/src/credentialgenerator.c @@ -72,6 +72,7 @@ OCStackResult PMGeneratePairWiseCredentials(OicSecCredType_t type, size_t keySiz res = OC_STACK_OK; exit: + OICClearMemory(privData, privDataKeySize); OICFree(privData); if(res != OC_STACK_OK) diff --git a/resource/csdk/security/provisioning/src/ownershiptransfermanager.c b/resource/csdk/security/provisioning/src/ownershiptransfermanager.c index ae25cbb..1bf238f 100644 --- a/resource/csdk/security/provisioning/src/ownershiptransfermanager.c +++ b/resource/csdk/security/provisioning/src/ownershiptransfermanager.c @@ -521,6 +521,7 @@ static OCStackResult SaveOwnerPSK(OCProvisionDev_t *selectedDeviceInfo) OicSecCred_t *cred = GenerateCredential(&selectedDeviceInfo->doxm->deviceID, SYMMETRIC_PAIR_WISE_KEY, NULL, &ownerKey, &ownerDeviceID, NULL); + OICClearMemory(ownerPSK, sizeof(ownerPSK)); VERIFY_NON_NULL(TAG, cred, ERROR); // TODO: Added as workaround. Will be replaced soon. diff --git a/resource/csdk/security/provisioning/src/secureresourceprovider.c b/resource/csdk/security/provisioning/src/secureresourceprovider.c index 171b796..ba0e514 100644 --- a/resource/csdk/security/provisioning/src/secureresourceprovider.c +++ b/resource/csdk/security/provisioning/src/secureresourceprovider.c @@ -341,6 +341,7 @@ static OCStackResult provisionCredentials(const OicSecCred_t *cred, query, sizeof(query), OIC_RSRC_CRED_URI)) { OIC_LOG(ERROR, TAG, "DeviceDiscoveryHandler : Failed to generate query"); + OCPayloadDestroy((OCPayload *)secPayload); return OC_STACK_ERROR; } OIC_LOG_V(DEBUG, TAG, "Query=%s", query); @@ -564,6 +565,7 @@ OCStackResult SRPProvisionTrustCertChain(void *ctx, OicSecCredType_t type, uint1 query, sizeof(query), OIC_RSRC_CRED_URI)) { OIC_LOG(ERROR, TAG, "SRPProvisionTrustCertChain : Failed to generate query"); + OCPayloadDestroy((OCPayload *)secPayload); return OC_STACK_ERROR; } OIC_LOG_V(DEBUG, TAG, "Query=%s", query); @@ -574,6 +576,7 @@ OCStackResult SRPProvisionTrustCertChain(void *ctx, OicSecCredType_t type, uint1 if (NULL == certData) { OIC_LOG(ERROR, TAG, "Memory allocation problem"); + OCPayloadDestroy((OCPayload *)secPayload); return OC_STACK_NO_MEMORY; } certData->deviceInfo = selectedDeviceInfo; diff --git a/resource/csdk/security/src/credresource.c b/resource/csdk/security/src/credresource.c index 65cbe65..79b9bf9 100644 --- a/resource/csdk/security/src/credresource.c +++ b/resource/csdk/security/src/credresource.c @@ -215,6 +215,7 @@ static void FreeCred(OicSecCred_t *cred) #endif /* __WITH_DTLS__ || __WITH_TLS__*/ //Clean PrivateData + OICClearMemory(cred->privateData.data, cred->privateData.len); OICFree(cred->privateData.data); //Clean Period @@ -1287,6 +1288,7 @@ static bool UpdatePersistentStorage(const OicSecCred_t *cred) { ret = true; } + OICClearMemory(payload, size); OICFree(payload); } } @@ -1671,6 +1673,8 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi { //Derive OwnerPSK locally const char* oxmLabel = GetOxmString(doxm->oxmSel); + char* b64Buf = NULL; + size_t b64BufSize = 0; VERIFY_NON_NULL(TAG, oxmLabel, ERROR); uint8_t ownerPSK[OWNER_PSK_LENGTH_128] = {0}; @@ -1679,6 +1683,7 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi doxm->owner.id, sizeof(doxm->owner.id), doxm->deviceID.id, sizeof(doxm->deviceID.id), ownerPSK, OWNER_PSK_LENGTH_128); + OICClearMemory(ownerPSK, sizeof(ownerPSK)); VERIFY_SUCCESS(TAG, pskRet == CA_STATUS_OK, ERROR); OIC_LOG(DEBUG, TAG, "OwnerPSK dump :"); @@ -1696,18 +1701,23 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi } else if(OIC_ENCODING_BASE64 == receviedCred->privateData.encoding) { + B64Result b64res = B64_OK; uint32_t b64OutSize = 0; - size_t b64BufSize = B64ENCODE_OUT_SAFESIZE((OWNER_PSK_LENGTH_128 + 1)); - char* b64Buf = OICCalloc(1, b64BufSize); + b64BufSize = B64ENCODE_OUT_SAFESIZE((OWNER_PSK_LENGTH_128 + 1)); + b64Buf = OICCalloc(1, b64BufSize); VERIFY_NON_NULL(TAG, b64Buf, ERROR); - b64Encode(ownerPSK, OWNER_PSK_LENGTH_128, b64Buf, b64BufSize, &b64OutSize); + b64res = b64Encode(ownerPSK, OWNER_PSK_LENGTH_128, b64Buf, b64BufSize, &b64OutSize); + VERIFY_SUCCESS(TAG, B64_OK == b64res, ERROR); receviedCred->privateData.data = (uint8_t *)OICCalloc(1, b64OutSize + 1); VERIFY_NON_NULL(TAG, receviedCred->privateData.data, ERROR); receviedCred->privateData.len = b64OutSize; strncpy((char*)receviedCred->privateData.data, b64Buf, b64OutSize); receviedCred->privateData.data[b64OutSize] = '\0'; + OICClearMemory(b64Buf, b64BufSize); + OICFree(b64Buf); + b64Buf = NULL; } else { @@ -1721,6 +1731,8 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi receviedCred->credType == SYMMETRIC_PAIR_WISE_KEY); exit: //receviedCred->privateData.data will be deallocated when deleting credential. + OICClearMemory(b64Buf, b64BufSize); + OICFree(b64Buf); return false; } @@ -2078,6 +2090,7 @@ static OCEntityHandlerResult HandleGetRequest (const OCEntityHandlerRequest * eh //Send payload to request originator ehRet = ((SendSRMResponse(ehRequest, ehRet, payload, size)) == OC_STACK_OK) ? OC_EH_OK : OC_EH_ERROR; + OICClearMemory(payload, size); OICFree(payload); return ehRet; } @@ -2215,6 +2228,7 @@ OCStackResult InitCredResource() //Instantiate 'oic.sec.cred' ret = CreateCredResource(); + OICClearMemory(data, size); OICFree(data); return ret; } @@ -2558,6 +2572,7 @@ OCStackResult AddTmpPskWithPIN(const OicUuid_t* tmpSubject, OicSecCredType_t cre cred = GenerateCredential(tmpSubject, credType, NULL, &privKey, rownerID, NULL); + OICClearMemory(privData, sizeof(privData)); if(NULL == cred) { OIC_LOG(ERROR, TAG, "GeneratePskWithPIN() : Failed to generate credential"); diff --git a/resource/csdk/security/src/dpairingresource.c b/resource/csdk/security/src/dpairingresource.c index b90de15..81271ab 100644 --- a/resource/csdk/security/src/dpairingresource.c +++ b/resource/csdk/security/src/dpairingresource.c @@ -154,6 +154,7 @@ OCStackResult SavePairingPSK(OCDevAddr *endpoint, OicSecCred_t *cred = GenerateCredential(peerDevID, SYMMETRIC_PAIR_WISE_KEY, NULL, &pairingKey, owner, NULL); + OICClearMemory(pairingPSK, sizeof(pairingPSK)); VERIFY_NON_NULL(TAG, cred, ERROR); res = AddCredential(cred); diff --git a/resource/csdk/stack/src/ocpayload.c b/resource/csdk/stack/src/ocpayload.c index 9a22c3c..22dd491 100644 --- a/resource/csdk/stack/src/ocpayload.c +++ b/resource/csdk/stack/src/ocpayload.c @@ -1644,6 +1644,7 @@ void OCSecurityPayloadDestroy(OCSecurityPayload* payload) return; } + OICClearMemory(payload->securityData, payload->payloadSize); OICFree(payload->securityData); OICFree(payload); } -- 2.7.4