Commit 916ced64 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>
parent 39f4a884
......@@ -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
......
......@@ -24,6 +24,12 @@
#include <stdlib.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
#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
......@@ -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)
......
......@@ -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)
......
......@@ -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.
......
......@@ -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;
......
......@@ -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");
......
......@@ -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);
......
......@@ -1644,6 +1644,7 @@ void OCSecurityPayloadDestroy(OCSecurityPayload* payload)
return;
}
OICClearMemory(payload->securityData, payload->payloadSize);
OICFree(payload->securityData);
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