Fixed memory leaks on error paths and added additional NULL checks

.gitignore

@@ -2,8 +2,10 @@
 *.orig
 .deps/
 .libs/
-.cproject
-.project
+.cproject/
+.project/
+.vs/
+.vscode/
 compile
 clean
 depcomp
@@ -56,17 +58,21 @@ src/openssl/Makefile
 win32/apps_a.int/
 win32/binaries/
 win32/configure.txt
+win32/apps.int/
 win32/libxmlsec.int/
 win32/libxmlsec_a.int/
 win32/libxmlsec_mscrypto.int/
 win32/libxmlsec_mscrypto_a.int/
+win32/libxmlsec_mscng.int/
+win32/libxmlsec_mscng_a.int/
 win32/tmp
+win32/*.pdb
 docs/api/code
 docs/api/sgml.tmp
 docs/api/xmlsec-*.txt
 docs/api/*.bak
 docs/api/*.types
 docs/api/sgml.stamp
 docs/html.stamp
-build-all/
+build-*/
 

src/keys.c

@@ -897,7 +897,8 @@ xmlSecKeyDebugDump(xmlSecKeyPtr key, FILE *output) {
 
     fprintf(output, "== KEY\n");
     fprintf(output, "=== method: %s\n",
-            (key->value->id->dataNodeName != NULL) ?
+            ((key->value != NULL) && (key->value->id != NULL) &&
+             (key->value->id->dataNodeName != NULL)) ?
             (char*)(key->value->id->dataNodeName) : "NULL");
 
     fprintf(output, "=== key type: ");
@@ -944,7 +945,10 @@ xmlSecKeyDebugXmlDump(xmlSecKeyPtr key, FILE *output) {
     fprintf(output, "<KeyInfo>\n");
 
     fprintf(output, "<KeyMethod>");
-    xmlSecPrintXmlString(output, key->value->id->dataNodeName);
+    /* xmlSecPrintXmlString() prints "NULL" for a NULL string */
+    xmlSecPrintXmlString(output,
+            ((key->value != NULL) && (key->value->id != NULL)) ?
+            key->value->id->dataNodeName : NULL);
     fprintf(output, "</KeyMethod>\n");
 
     fprintf(output, "<KeyType>");

src/keysmngr.c

@@ -64,6 +64,7 @@ xmlSecKeysMngrCreate(void) {
     ret = xmlSecPtrListInitialize(&(mngr->storesList), xmlSecKeyDataStorePtrListId);
     if(ret < 0) {
         xmlSecInternalError("xmlSecPtrListInitialize(xmlSecKeyDataStorePtrListId)", NULL);
+        xmlFree(mngr);
         return(NULL);
     }
 

src/list.c

@@ -188,6 +188,15 @@ xmlSecPtrListCopy(xmlSecPtrListPtr dst, xmlSecPtrListPtr src) {
         return(0);
     }
 
+    /*
+     * If the klass destroys its items but does not support duplicating
+     * them, raw pointer sharing would leave both lists owning the same
+     * items, so both would destroy them at finalize (double free).
+     * Klasses with destroyItem == NULL hold non-owned pointers (e.g.
+     * static ids) and are safe to share.
+     */
+    xmlSecAssert2((dst->id->duplicateItem != NULL) || (dst->id->destroyItem == NULL), -1);
+
     initialUse = dst->use;
 
     /* allocate memory */

src/mscng/app.c

@@ -391,6 +391,7 @@ xmlSecMSCngAppPkcs12Load(const char *filename,
     if(ret < 0) {
         xmlSecInternalError2("xmlSecBufferReadFile", NULL, "filename=%s",
             xmlSecErrorsSafeString(filename));
+        xmlSecBufferFinalize(&buffer);
         return(NULL);
     }
 

src/mscng/crypto.c

@@ -351,6 +351,7 @@ xmlSecMSCngShutdown(void) {
  */
 int
 xmlSecMSCngGenerateRandom(xmlSecBufferPtr buffer, xmlSecSize size) {
+    xmlSecByte* data;
     NTSTATUS status;
     DWORD dwSize;
     int ret;
@@ -364,10 +365,13 @@ xmlSecMSCngGenerateRandom(xmlSecBufferPtr buffer, xmlSecSize size) {
         return(-1);
     }
 
+    data = xmlSecBufferGetData(buffer);
+    xmlSecAssert2(data != NULL, -1);
+
     XMLSEC_SAFE_CAST_SIZE_TO_ULONG(size, dwSize, return(-1), NULL);
     status = BCryptGenRandom(
         NULL,
-        (PBYTE)xmlSecBufferGetData(buffer),
+        (PBYTE)data,
         dwSize,
         BCRYPT_USE_SYSTEM_PREFERRED_RNG);
     if(status != STATUS_SUCCESS) {

src/mscng/digests.c

@@ -156,14 +156,16 @@ static void xmlSecMSCngDigestFinalize(xmlSecTransformPtr transform) {
     ctx = xmlSecMSCngDigestGetCtx(transform);
     xmlSecAssert(ctx != NULL);
 
-    if(ctx->hAlg != 0) {
-        BCryptCloseAlgorithmProvider(ctx->hAlg, 0);
-    }
-
+    /* NTSTATUS is intentionally ignored: failures at finalization time are not
+     * recoverable and cannot be reported from a void finalize method */
     if(ctx->hHash != 0) {
         BCryptDestroyHash(ctx->hHash);
     }
 
+    if(ctx->hAlg != 0) {
+        BCryptCloseAlgorithmProvider(ctx->hAlg, 0);
+    }
+
     if(ctx->pbHashObject != NULL) {
         xmlFree(ctx->pbHashObject);
     }
@@ -239,6 +241,13 @@ xmlSecMSCngDigestExecute(xmlSecTransformPtr transform,
     xmlSecAssert2(ctx != NULL, -1);
 
     if(transform->status == xmlSecTransformStatusNone) {
+        /* Note: the error paths below may leave hAlg/pbHashObject/pbHash
+         * partially acquired on purpose; xmlSecTransformDestroy() invokes this
+         * klass's finalize() unconditionally when the transform is destroyed,
+         * and it releases any of these resources. The xmlsec transform framework
+         * does not re-invoke execute() after a failure, so the handle is never
+         * re-opened (and thus never leaked) on a retry. */
+
         /* open an algorithm handle */
         status = BCryptOpenAlgorithmProvider(
             &ctx->hAlg,
@@ -264,6 +273,7 @@ xmlSecMSCngDigestExecute(xmlSecTransformPtr transform,
         }
 
         /* allocate the hash object on the heap */
+        xmlSecAssert2(ctx->pbHashObject == NULL, -1);
         ctx->pbHashObject = (PBYTE)xmlMalloc(cbHashObject);
         if(ctx->pbHashObject == NULL) {
             xmlSecMallocError(cbHashObject, NULL);
@@ -284,13 +294,15 @@ xmlSecMSCngDigestExecute(xmlSecTransformPtr transform,
         }
 
         /* allocate the hash buffer on the heap */
+        xmlSecAssert2(ctx->pbHash == NULL, -1);
         ctx->pbHash = (PBYTE)xmlMalloc(ctx->cbHash);
         if(ctx->pbHash == NULL) {
             xmlSecMallocError(ctx->cbHash, NULL);
             return(-1);
         }
 
         /* create the hash */
+        xmlSecAssert2(ctx->hHash == NULL, -1);
         status = BCryptCreateHash(
             ctx->hAlg,
             &ctx->hHash,

src/mscng/kw_aes.c

@@ -375,6 +375,7 @@ xmlSecMSCngKWAesBlockEncrypt(xmlSecTransformPtr transform, const xmlSecByte* in,
     xmlSecBuffer blob;
     int blob_initialized = 0;
     BCRYPT_KEY_DATA_BLOB_HEADER* blobHeader;
+    xmlSecByte* blobData;
     xmlSecSize blobHeaderSize, blobSize;
     xmlSecByte* keyData;
     xmlSecSize keySize;
@@ -428,6 +429,10 @@ xmlSecMSCngKWAesBlockEncrypt(xmlSecTransformPtr transform, const xmlSecByte* in,
         xmlSecMSCngNtError("BCryptGetProperty", NULL, status);
         goto done;
     }
+    if (cbData != sizeof(DWORD)) {
+        xmlSecInternalError("BCryptGetProperty", NULL);
+        goto done;
+    }
 
     pbKeyObject = xmlMalloc(cbKeyObject);
     if (pbKeyObject == NULL) {
@@ -444,12 +449,14 @@ xmlSecMSCngKWAesBlockEncrypt(xmlSecTransformPtr transform, const xmlSecByte* in,
         goto done;
     }
 
-    blobHeader = (BCRYPT_KEY_DATA_BLOB_HEADER*)xmlSecBufferGetData(&blob);
+    blobData = xmlSecBufferGetData(&blob);
+    xmlSecAssert2(blobData != NULL, -1);
+    blobHeader = (BCRYPT_KEY_DATA_BLOB_HEADER*)blobData;
     blobHeader->dwMagic = BCRYPT_KEY_DATA_BLOB_MAGIC;
     blobHeader->dwVersion = BCRYPT_KEY_DATA_BLOB_VERSION1;
     XMLSEC_SAFE_CAST_SIZE_TO_ULONG(keySize, blobHeader->cbKeyData, goto done, NULL);
 
-    memcpy(xmlSecBufferGetData(&blob) + sizeof(BCRYPT_KEY_DATA_BLOB_HEADER),
+    memcpy(blobData + sizeof(BCRYPT_KEY_DATA_BLOB_HEADER),
         keyData, keySize);
 
     blobSize = xmlSecBufferGetSize(&blob);
@@ -462,7 +469,7 @@ xmlSecMSCngKWAesBlockEncrypt(xmlSecTransformPtr transform, const xmlSecByte* in,
         &hKey,
         pbKeyObject,
         cbKeyObject,
-        xmlSecBufferGetData(&blob),
+        blobData,
         dwBlobSize,
         0);
     if (status != STATUS_SUCCESS) {
@@ -525,6 +532,7 @@ xmlSecMSCngKWAesBlockDecrypt(xmlSecTransformPtr transform, const xmlSecByte* in,
     xmlSecBuffer blob;
     int blob_initialized = 0;
     BCRYPT_KEY_DATA_BLOB_HEADER* blobHeader;
+    xmlSecByte* blobData;
     xmlSecSize blobHeaderSize, blobSize;
     xmlSecByte* keyData;
     xmlSecSize keySize;
@@ -578,6 +586,10 @@ xmlSecMSCngKWAesBlockDecrypt(xmlSecTransformPtr transform, const xmlSecByte* in,
         xmlSecMSCngNtError("BCryptGetProperty", NULL, status);
         goto done;
     }
+    if (cbData != sizeof(DWORD)) {
+        xmlSecInternalError("BCryptGetProperty", NULL);
+        goto done;
+    }
 
     pbKeyObject = xmlMalloc(cbKeyObject);
     if (pbKeyObject == NULL) {
@@ -594,12 +606,14 @@ xmlSecMSCngKWAesBlockDecrypt(xmlSecTransformPtr transform, const xmlSecByte* in,
         goto done;
     }
 
-    blobHeader = (BCRYPT_KEY_DATA_BLOB_HEADER*)xmlSecBufferGetData(&blob);
+    blobData = xmlSecBufferGetData(&blob);
+    xmlSecAssert2(blobData != NULL, -1);
+    blobHeader = (BCRYPT_KEY_DATA_BLOB_HEADER*)blobData;
     blobHeader->dwMagic = BCRYPT_KEY_DATA_BLOB_MAGIC;
     blobHeader->dwVersion = BCRYPT_KEY_DATA_BLOB_VERSION1;
     XMLSEC_SAFE_CAST_SIZE_TO_ULONG(keySize, blobHeader->cbKeyData, goto done, NULL);
 
-    memcpy(xmlSecBufferGetData(&blob) + sizeof(BCRYPT_KEY_DATA_BLOB_HEADER),
+    memcpy(blobData + sizeof(BCRYPT_KEY_DATA_BLOB_HEADER),
         keyData, keySize);
 
     blobSize = xmlSecBufferGetSize(&blob);
@@ -612,7 +626,7 @@ xmlSecMSCngKWAesBlockDecrypt(xmlSecTransformPtr transform, const xmlSecByte* in,
         &hKey,
         pbKeyObject,
         cbKeyObject,
-        xmlSecBufferGetData(&blob),
+        blobData,
         dwBlobSize,
         0);
     if (status != STATUS_SUCCESS) {

src/mscng/kw_des.c

@@ -63,7 +63,8 @@ xmlSecMSCngKWDes3GenerateRandom(xmlSecTransformPtr transform XMLSEC_ATTRIBUTE_UN
     NTSTATUS status;
     DWORD dwOutSize;
 
-    UNREFERENCED_PARAMETER(transform);
+    xmlSecAssert2(xmlSecTransformCheckId(transform, xmlSecMSCngTransformKWDes3Id), -1);
+    xmlSecAssert2(xmlSecTransformCheckSize(transform, xmlSecMSCngKWDes3Size), -1);
     xmlSecAssert2(out != NULL, -1);
     xmlSecAssert2(outSize > 0, -1);
     xmlSecAssert2(outWritten != NULL, -1);
@@ -128,6 +129,7 @@ xmlSecMSCngKWDes3Sha1(xmlSecTransformPtr transform, const xmlSecByte * in, xmlSe
         xmlSecMSCngNtError("BCryptGetProperty", NULL, status);
         goto done;
     }
+    xmlSecAssert2(cbData == sizeof(DWORD), -1);
 
     pbHashObject = (PBYTE)xmlMalloc(cbHashObject);
     if(pbHashObject == NULL) {
@@ -145,6 +147,12 @@ xmlSecMSCngKWDes3Sha1(xmlSecTransformPtr transform, const xmlSecByte * in, xmlSe
         xmlSecMSCngNtError("BCryptGetProperty", NULL, status);
         goto done;
     }
+    xmlSecAssert2(cbData == sizeof(DWORD), -1);
+
+    if(outSize < cbHash) {
+        xmlSecInvalidSizeLessThanError("outSize", outSize, (xmlSecSize)cbHash, NULL);
+        goto done;
+    }
 
     pbHash = (PBYTE)xmlMalloc(cbHash);
     if(pbHash == NULL) {
@@ -184,7 +192,7 @@ xmlSecMSCngKWDes3Sha1(xmlSecTransformPtr transform, const xmlSecByte * in, xmlSe
         xmlSecMSCngNtError("BCryptFinishHash", NULL, status);
         goto done;
     }
-    memcpy(out, pbHash, outSize);
+    memcpy(out, pbHash, cbHash);
     XMLSEC_SAFE_CAST_ULONG_TO_SIZE(cbHash, (*outWritten), goto done, NULL);
     res = 0;
 
@@ -220,7 +228,7 @@ xmlSecMSCngKWDes3BlockEncrypt(xmlSecTransformPtr transform, const xmlSecByte * i
     DWORD cbKeyObject;
     xmlSecBuffer blob;
     BCRYPT_KEY_DATA_BLOB_HEADER* blobHeader;
-    xmlSecSize blobHeaderSize, blobSizeInBits;
+    xmlSecSize blobHeaderSize, blockLen;
     NTSTATUS status;
     xmlSecSize keySize, blobSize;
     DWORD dwBlobSize, dwInSize, dwIvSize, dwOutSize;
@@ -271,6 +279,7 @@ xmlSecMSCngKWDes3BlockEncrypt(xmlSecTransformPtr transform, const xmlSecByte * i
         xmlSecMSCngNtError("BCryptGetProperty", NULL, status);
         goto done;
     }
+    xmlSecAssert2(cbData == sizeof(DWORD), -1);
 
     pbKeyObject = xmlMalloc(cbKeyObject);
     if(pbKeyObject == NULL) {
@@ -327,10 +336,14 @@ xmlSecMSCngKWDes3BlockEncrypt(xmlSecTransformPtr transform, const xmlSecByte * i
         xmlSecMSCngNtError("BCryptGetProperty", NULL, status);
         goto done;
     }
-    XMLSEC_SAFE_CAST_ULONG_TO_SIZE(dwBlockLen, blobSizeInBits, goto done, NULL);
+    xmlSecAssert2(dwBlockLenLen == sizeof(dwBlockLen), -1);
+    XMLSEC_SAFE_CAST_ULONG_TO_SIZE(dwBlockLen, blockLen, goto done, NULL);
 
-    if(ivSize < blobSizeInBits / 8) {
-        xmlSecInvalidSizeLessThanError("ivSize", ivSize, blobSizeInBits / 8, NULL);
+    /* CNG requires the IV to be exactly the block length; reject any other
+     * size (this also guarantees that dwIvSize, cast from ivSize below, equals
+     * blockLen) */
+    if(ivSize != blockLen) {
+        xmlSecInvalidSizeError("ivSize", ivSize, blockLen, NULL);
         goto done;
     }
 
@@ -367,6 +380,11 @@ xmlSecMSCngKWDes3BlockEncrypt(xmlSecTransformPtr transform, const xmlSecByte * i
         xmlSecMSCngNtError("BCryptEncrypt", NULL, status);
         goto done;
     }
+    if(cbData != dwInSize) {
+        xmlSecInternalError2("BCryptEncrypt output size", NULL,
+            "size=" XMLSEC_SIZE_FMT, (xmlSecSize)cbData);
+        goto done;
+    }
     XMLSEC_SAFE_CAST_ULONG_TO_SIZE(cbData, (*outWritten), goto done, NULL);
     res = 0;
 
@@ -402,7 +420,7 @@ xmlSecMSCngKWDes3BlockDecrypt(xmlSecTransformPtr transform, const xmlSecByte * i
     DWORD cbKeyObject;
     xmlSecBuffer blob;
     BCRYPT_KEY_DATA_BLOB_HEADER* blobHeader;
-    xmlSecSize blobHeaderSize, blobSizeInBits;
+    xmlSecSize blobHeaderSize, blockLen;
     xmlSecSize keySize, blobSize;
     DWORD dwBlobSize, dwInSize, dwIvSize, dwOutSize;
     NTSTATUS status;
@@ -452,6 +470,7 @@ xmlSecMSCngKWDes3BlockDecrypt(xmlSecTransformPtr transform, const xmlSecByte * i
         xmlSecMSCngNtError("BCryptGetProperty", NULL, status);
         goto done;
     }
+    xmlSecAssert2(cbData == sizeof(DWORD), -1);
 
     pbKeyObject = xmlMalloc(cbKeyObject);
     if(pbKeyObject == NULL) {
@@ -508,10 +527,14 @@ xmlSecMSCngKWDes3BlockDecrypt(xmlSecTransformPtr transform, const xmlSecByte * i
         xmlSecMSCngNtError("BCryptGetProperty", NULL, status);
         goto done;
     }
-    XMLSEC_SAFE_CAST_ULONG_TO_SIZE(dwBlockLen, blobSizeInBits, goto done, NULL);
+    xmlSecAssert2(dwBlockLenLen == sizeof(dwBlockLen), -1);
+    XMLSEC_SAFE_CAST_ULONG_TO_SIZE(dwBlockLen, blockLen, goto done, NULL);
 
-    if(ivSize < blobSizeInBits / 8) {
-        xmlSecInvalidSizeLessThanError("ivSize", ivSize, blobSizeInBits / 8, NULL);
+    /* CNG requires the IV to be exactly the block length; reject any other
+     * size (this also guarantees that dwIvSize, cast from ivSize below, equals
+     * blockLen) */
+    if(ivSize != blockLen) {
+        xmlSecInvalidSizeError("ivSize", ivSize, blockLen, NULL);
         goto done;
     }
 
@@ -539,6 +562,11 @@ xmlSecMSCngKWDes3BlockDecrypt(xmlSecTransformPtr transform, const xmlSecByte * i
         xmlSecMSCngNtError("BCryptDecrypt", NULL, status);
         goto done;
     }
+    if(cbData != dwInSize) {
+        xmlSecInternalError2("BCryptDecrypt output size", NULL,
+            "size=" XMLSEC_SIZE_FMT, (xmlSecSize)cbData);
+        goto done;
+    }
     XMLSEC_SAFE_CAST_ULONG_TO_SIZE(cbData, (*outWritten), goto done, NULL);
     res = 0;