[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 <kkane@microsoft.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/14091
Tested-by: jenkins-iotivity <jenkins-iotivity@opendaylight.org>
Reviewed-by: Uze Choi <uzchoi@samsung.com>
Reviewed-by: Randeep Singh <randeep.s@samsung.com>

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 (file)
--- 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 (file)
--- 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 <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

diff --git a/resource/csdk/security/provisioning/src/cloud/csr.c b/resource/csdk/security/provisioning/src/cloud/csr.c
index eab8412..1c4fcd1 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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);
 }