Commit 9b1a61ec authored by Kevin Kane's avatar Kevin Kane Committed by Randeep

[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: default avatarKevin Kane <kkane@microsoft.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/14091Tested-by: default avatarjenkins-iotivity <jenkins-iotivity@opendaylight.org>
Reviewed-by: default avatarUze Choi <uzchoi@samsung.com>
Reviewed-by: Randeep's avatarRandeep Singh <randeep.s@samsung.com>
(cherry picked from commit 916ced64)
Reviewed-on: https://gerrit.iotivity.org/gerrit/14443
parent 1ae76d5f
...@@ -111,6 +111,20 @@ void *OICCalloc(size_t num, size_t size); ...@@ -111,6 +111,20 @@ void *OICCalloc(size_t num, size_t size);
*/ */
void OICFree(void *ptr); 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 #ifdef __cplusplus
} }
#endif // __cplusplus #endif // __cplusplus
......
...@@ -24,6 +24,12 @@ ...@@ -24,6 +24,12 @@
#include <stdlib.h> #include <stdlib.h>
#include "oic_malloc.h" #include "oic_malloc.h"
#include "iotivity_config.h"
#ifdef HAVE_WINDOWS_H
#include <windows.h>
#endif
// Enable extra debug logging for malloc. Comment out to disable // Enable extra debug logging for malloc. Comment out to disable
#ifdef ENABLE_MALLOC_DEBUG #ifdef ENABLE_MALLOC_DEBUG
#include "logger.h" #include "logger.h"
...@@ -134,3 +140,18 @@ void OICFree(void *ptr) ...@@ -134,3 +140,18 @@ void OICFree(void *ptr)
free(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
...@@ -52,7 +52,7 @@ ...@@ -52,7 +52,7 @@
#define TAG "OIC_CLOUD_CSR" #define TAG "OIC_CLOUD_CSR"
//TODO: is it required in CSR response? //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 #define MAX_URI_QUERY MAX_URI_LENGTH + MAX_QUERY_LENGTH
...@@ -299,15 +299,15 @@ static int GenerateCSR(char *subject, OCByteString *csr) ...@@ -299,15 +299,15 @@ static int GenerateCSR(char *subject, OCByteString *csr)
OIC_LOG_V(DEBUG, TAG, "Out %s", __func__); OIC_LOG_V(DEBUG, TAG, "Out %s", __func__);
return -1; return -1;
} }
privateKey.bytes = (uint8_t *)OICMalloc(ret * sizeof(char)); g_privateKey.bytes = (uint8_t *)OICMalloc(ret * sizeof(char));
if (NULL == privateKey.bytes) 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__); OIC_LOG_V(DEBUG, TAG, "Out %s", __func__);
return -1; return -1;
} }
memcpy(privateKey.bytes, buf + bufsize - ret, ret * sizeof(uint8_t)); memcpy(g_privateKey.bytes, buf + bufsize - ret, ret * sizeof(uint8_t));
privateKey.len = ret; g_privateKey.len = ret;
// Public key to output // Public key to output
ret = mbedtls_pk_write_pubkey_der(key, buf, bufsize); ret = mbedtls_pk_write_pubkey_der(key, buf, bufsize);
if (ret < 0) if (ret < 0)
...@@ -388,8 +388,8 @@ static OCStackResult HandleCertificateIssueRequest(void *ctx, void **data, OCCli ...@@ -388,8 +388,8 @@ static OCStackResult HandleCertificateIssueRequest(void *ctx, void **data, OCCli
{ {
OicSecKey_t key = OicSecKey_t key =
{ {
privateKey.bytes, g_privateKey.bytes,
privateKey.len, g_privateKey.len,
OIC_ENCODING_DER OIC_ENCODING_DER
}; };
...@@ -425,9 +425,10 @@ static OCStackResult HandleCertificateIssueRequest(void *ctx, void **data, OCCli ...@@ -425,9 +425,10 @@ static OCStackResult HandleCertificateIssueRequest(void *ctx, void **data, OCCli
} }
} }
OICFree(privateKey.bytes); OICClearMemory(g_privateKey.bytes, g_privateKey.len);
privateKey.bytes = NULL; OICFree(g_privateKey.bytes);
privateKey.len = 0; g_privateKey.bytes = NULL;
g_privateKey.len = 0;
OIC_LOG_V(DEBUG, TAG, "OUT: %s", __func__); OIC_LOG_V(DEBUG, TAG, "OUT: %s", __func__);
...@@ -472,7 +473,7 @@ OCStackResult OCCloudCertificateIssueRequest(void* ctx, ...@@ -472,7 +473,7 @@ OCStackResult OCCloudCertificateIssueRequest(void* ctx,
OIC_LOG_BUFFER(DEBUG, TAG, request.bytes, request.len); OIC_LOG_BUFFER(DEBUG, TAG, request.bytes, request.len);
OIC_LOG(DEBUG, TAG, "Private Key:"); 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(); OCRepPayload* payload = OCRepPayloadCreate();
if (!payload) if (!payload)
......
...@@ -72,6 +72,7 @@ OCStackResult PMGeneratePairWiseCredentials(OicSecCredType_t type, size_t keySiz ...@@ -72,6 +72,7 @@ OCStackResult PMGeneratePairWiseCredentials(OicSecCredType_t type, size_t keySiz
res = OC_STACK_OK; res = OC_STACK_OK;
exit: exit:
OICClearMemory(privData, privDataKeySize);
OICFree(privData); OICFree(privData);
if(res != OC_STACK_OK) if(res != OC_STACK_OK)
......
...@@ -521,6 +521,7 @@ static OCStackResult SaveOwnerPSK(OCProvisionDev_t *selectedDeviceInfo) ...@@ -521,6 +521,7 @@ static OCStackResult SaveOwnerPSK(OCProvisionDev_t *selectedDeviceInfo)
OicSecCred_t *cred = GenerateCredential(&selectedDeviceInfo->doxm->deviceID, OicSecCred_t *cred = GenerateCredential(&selectedDeviceInfo->doxm->deviceID,
SYMMETRIC_PAIR_WISE_KEY, NULL, SYMMETRIC_PAIR_WISE_KEY, NULL,
&ownerKey, &ownerDeviceID, NULL); &ownerKey, &ownerDeviceID, NULL);
OICClearMemory(ownerPSK, sizeof(ownerPSK));
VERIFY_NON_NULL(TAG, cred, ERROR); VERIFY_NON_NULL(TAG, cred, ERROR);
// TODO: Added as workaround. Will be replaced soon. // TODO: Added as workaround. Will be replaced soon.
......
...@@ -341,6 +341,7 @@ static OCStackResult provisionCredentials(const OicSecCred_t *cred, ...@@ -341,6 +341,7 @@ static OCStackResult provisionCredentials(const OicSecCred_t *cred,
query, sizeof(query), OIC_RSRC_CRED_URI)) query, sizeof(query), OIC_RSRC_CRED_URI))
{ {
OIC_LOG(ERROR, TAG, "DeviceDiscoveryHandler : Failed to generate query"); OIC_LOG(ERROR, TAG, "DeviceDiscoveryHandler : Failed to generate query");
OCPayloadDestroy((OCPayload *)secPayload);
return OC_STACK_ERROR; return OC_STACK_ERROR;
} }
OIC_LOG_V(DEBUG, TAG, "Query=%s", query); OIC_LOG_V(DEBUG, TAG, "Query=%s", query);
...@@ -564,6 +565,7 @@ OCStackResult SRPProvisionTrustCertChain(void *ctx, OicSecCredType_t type, uint1 ...@@ -564,6 +565,7 @@ OCStackResult SRPProvisionTrustCertChain(void *ctx, OicSecCredType_t type, uint1
query, sizeof(query), OIC_RSRC_CRED_URI)) query, sizeof(query), OIC_RSRC_CRED_URI))
{ {
OIC_LOG(ERROR, TAG, "SRPProvisionTrustCertChain : Failed to generate query"); OIC_LOG(ERROR, TAG, "SRPProvisionTrustCertChain : Failed to generate query");
OCPayloadDestroy((OCPayload *)secPayload);
return OC_STACK_ERROR; return OC_STACK_ERROR;
} }
OIC_LOG_V(DEBUG, TAG, "Query=%s", query); OIC_LOG_V(DEBUG, TAG, "Query=%s", query);
...@@ -574,6 +576,7 @@ OCStackResult SRPProvisionTrustCertChain(void *ctx, OicSecCredType_t type, uint1 ...@@ -574,6 +576,7 @@ OCStackResult SRPProvisionTrustCertChain(void *ctx, OicSecCredType_t type, uint1
if (NULL == certData) if (NULL == certData)
{ {
OIC_LOG(ERROR, TAG, "Memory allocation problem"); OIC_LOG(ERROR, TAG, "Memory allocation problem");
OCPayloadDestroy((OCPayload *)secPayload);
return OC_STACK_NO_MEMORY; return OC_STACK_NO_MEMORY;
} }
certData->deviceInfo = selectedDeviceInfo; certData->deviceInfo = selectedDeviceInfo;
......
...@@ -215,6 +215,7 @@ static void FreeCred(OicSecCred_t *cred) ...@@ -215,6 +215,7 @@ static void FreeCred(OicSecCred_t *cred)
#endif /* __WITH_DTLS__ || __WITH_TLS__*/ #endif /* __WITH_DTLS__ || __WITH_TLS__*/
//Clean PrivateData //Clean PrivateData
OICClearMemory(cred->privateData.data, cred->privateData.len);
OICFree(cred->privateData.data); OICFree(cred->privateData.data);
//Clean Period //Clean Period
...@@ -1287,6 +1288,7 @@ static bool UpdatePersistentStorage(const OicSecCred_t *cred) ...@@ -1287,6 +1288,7 @@ static bool UpdatePersistentStorage(const OicSecCred_t *cred)
{ {
ret = true; ret = true;
} }
OICClearMemory(payload, size);
OICFree(payload); OICFree(payload);
} }
} }
...@@ -1671,6 +1673,8 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi ...@@ -1671,6 +1673,8 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi
{ {
//Derive OwnerPSK locally //Derive OwnerPSK locally
const char* oxmLabel = GetOxmString(doxm->oxmSel); const char* oxmLabel = GetOxmString(doxm->oxmSel);
char* b64Buf = NULL;
size_t b64BufSize = 0;
VERIFY_NON_NULL(TAG, oxmLabel, ERROR); VERIFY_NON_NULL(TAG, oxmLabel, ERROR);
uint8_t ownerPSK[OWNER_PSK_LENGTH_128] = {0}; uint8_t ownerPSK[OWNER_PSK_LENGTH_128] = {0};
...@@ -1679,6 +1683,7 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi ...@@ -1679,6 +1683,7 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi
doxm->owner.id, sizeof(doxm->owner.id), doxm->owner.id, sizeof(doxm->owner.id),
doxm->deviceID.id, sizeof(doxm->deviceID.id), doxm->deviceID.id, sizeof(doxm->deviceID.id),
ownerPSK, OWNER_PSK_LENGTH_128); ownerPSK, OWNER_PSK_LENGTH_128);
OICClearMemory(ownerPSK, sizeof(ownerPSK));
VERIFY_SUCCESS(TAG, pskRet == CA_STATUS_OK, ERROR); VERIFY_SUCCESS(TAG, pskRet == CA_STATUS_OK, ERROR);
OIC_LOG(DEBUG, TAG, "OwnerPSK dump :"); OIC_LOG(DEBUG, TAG, "OwnerPSK dump :");
...@@ -1696,18 +1701,23 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi ...@@ -1696,18 +1701,23 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi
} }
else if(OIC_ENCODING_BASE64 == receviedCred->privateData.encoding) else if(OIC_ENCODING_BASE64 == receviedCred->privateData.encoding)
{ {
B64Result b64res = B64_OK;
uint32_t b64OutSize = 0; uint32_t b64OutSize = 0;
size_t b64BufSize = B64ENCODE_OUT_SAFESIZE((OWNER_PSK_LENGTH_128 + 1)); b64BufSize = B64ENCODE_OUT_SAFESIZE((OWNER_PSK_LENGTH_128 + 1));
char* b64Buf = OICCalloc(1, b64BufSize); b64Buf = OICCalloc(1, b64BufSize);
VERIFY_NON_NULL(TAG, b64Buf, ERROR); 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); receviedCred->privateData.data = (uint8_t *)OICCalloc(1, b64OutSize + 1);
VERIFY_NON_NULL(TAG, receviedCred->privateData.data, ERROR); VERIFY_NON_NULL(TAG, receviedCred->privateData.data, ERROR);
receviedCred->privateData.len = b64OutSize; receviedCred->privateData.len = b64OutSize;
strncpy((char*)receviedCred->privateData.data, b64Buf, b64OutSize); strncpy((char*)receviedCred->privateData.data, b64Buf, b64OutSize);
receviedCred->privateData.data[b64OutSize] = '\0'; receviedCred->privateData.data[b64OutSize] = '\0';
OICClearMemory(b64Buf, b64BufSize);
OICFree(b64Buf);
b64Buf = NULL;
} }
else else
{ {
...@@ -1721,6 +1731,8 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi ...@@ -1721,6 +1731,8 @@ static bool FillPrivateDataOfOwnerPSK(OicSecCred_t* receviedCred, const CAEndpoi
receviedCred->credType == SYMMETRIC_PAIR_WISE_KEY); receviedCred->credType == SYMMETRIC_PAIR_WISE_KEY);
exit: exit:
//receviedCred->privateData.data will be deallocated when deleting credential. //receviedCred->privateData.data will be deallocated when deleting credential.
OICClearMemory(b64Buf, b64BufSize);
OICFree(b64Buf);
return false; return false;
} }
...@@ -2078,6 +2090,7 @@ static OCEntityHandlerResult HandleGetRequest (const OCEntityHandlerRequest * eh ...@@ -2078,6 +2090,7 @@ static OCEntityHandlerResult HandleGetRequest (const OCEntityHandlerRequest * eh
//Send payload to request originator //Send payload to request originator
ehRet = ((SendSRMResponse(ehRequest, ehRet, payload, size)) == OC_STACK_OK) ? ehRet = ((SendSRMResponse(ehRequest, ehRet, payload, size)) == OC_STACK_OK) ?
OC_EH_OK : OC_EH_ERROR; OC_EH_OK : OC_EH_ERROR;
OICClearMemory(payload, size);
OICFree(payload); OICFree(payload);
return ehRet; return ehRet;
} }
...@@ -2215,6 +2228,7 @@ OCStackResult InitCredResource() ...@@ -2215,6 +2228,7 @@ OCStackResult InitCredResource()
//Instantiate 'oic.sec.cred' //Instantiate 'oic.sec.cred'
ret = CreateCredResource(); ret = CreateCredResource();
OICClearMemory(data, size);
OICFree(data); OICFree(data);
return ret; return ret;
} }
...@@ -2558,6 +2572,7 @@ OCStackResult AddTmpPskWithPIN(const OicUuid_t* tmpSubject, OicSecCredType_t cre ...@@ -2558,6 +2572,7 @@ OCStackResult AddTmpPskWithPIN(const OicUuid_t* tmpSubject, OicSecCredType_t cre
cred = GenerateCredential(tmpSubject, credType, NULL, cred = GenerateCredential(tmpSubject, credType, NULL,
&privKey, rownerID, NULL); &privKey, rownerID, NULL);
OICClearMemory(privData, sizeof(privData));
if(NULL == cred) if(NULL == cred)
{ {
OIC_LOG(ERROR, TAG, "GeneratePskWithPIN() : Failed to generate credential"); OIC_LOG(ERROR, TAG, "GeneratePskWithPIN() : Failed to generate credential");
......
...@@ -154,6 +154,7 @@ OCStackResult SavePairingPSK(OCDevAddr *endpoint, ...@@ -154,6 +154,7 @@ OCStackResult SavePairingPSK(OCDevAddr *endpoint,
OicSecCred_t *cred = GenerateCredential(peerDevID, OicSecCred_t *cred = GenerateCredential(peerDevID,
SYMMETRIC_PAIR_WISE_KEY, NULL, SYMMETRIC_PAIR_WISE_KEY, NULL,
&pairingKey, owner, NULL); &pairingKey, owner, NULL);
OICClearMemory(pairingPSK, sizeof(pairingPSK));
VERIFY_NON_NULL(TAG, cred, ERROR); VERIFY_NON_NULL(TAG, cred, ERROR);
res = AddCredential(cred); res = AddCredential(cred);
......
...@@ -1644,6 +1644,7 @@ void OCSecurityPayloadDestroy(OCSecurityPayload* payload) ...@@ -1644,6 +1644,7 @@ void OCSecurityPayloadDestroy(OCSecurityPayload* payload)
return; return;
} }
OICClearMemory(payload->securityData, payload->payloadSize);
OICFree(payload->securityData); OICFree(payload->securityData);
OICFree(payload); OICFree(payload);
} }
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment