(xmlsec-mscng) Hardened parameters validation; improved CRL verification; and fixed comments

src/mscng/app.c

@@ -469,7 +469,7 @@ xmlSecMSCngAppPkcs12LoadMemory(const xmlSecByte* data, xmlSecSize dataSize, cons
         goto cleanup;
     }
 
-    DWORD flags = CRYPT_EXPORTABLE;
+    DWORD flags = CRYPT_EXPORTABLE | PKCS12_ALWAYS_CNG_KSP;
     if (!xmlSecImportGetPersistKey()) {
         flags |= PKCS12_NO_PERSIST_KEY;
     }

src/mscng/ciphers.c

@@ -528,6 +528,7 @@ static int xmlSecMSCngGCMBlockCipherCtxInit(xmlSecMSCngBlockCipherCtxPtr ctx,
             xmlSecMSCngNtError("BCryptGetProperty", cipherName, status);
             return(-1);
         }
+        xmlSecAssert2(bytesRead == sizeof(authTagLengths), -1);
 
         if(ctx->authInfo.pbMacContext == NULL) {
             ctx->authInfo.pbMacContext = xmlMalloc(authTagLengths.dwMaxLength);
@@ -675,6 +676,9 @@ xmlSecMSCngCBCBlockCipherCtxUpdate(xmlSecMSCngBlockCipherCtxPtr ctx,
         inBlocks = (inSize - 1) / blockSize;
     }
     inSize = inBlocks * blockSize;
+    if(inSize == 0) {
+        return(0);
+    }
 
     /* we write out the input size plus maybe one block */
     if((inSize > XMLSEC_SIZE_MAX - blockSize) || (outSize > XMLSEC_SIZE_MAX - inSize - blockSize)) {
@@ -814,6 +818,11 @@ xmlSecMSCngGCMBlockCipherCtxUpdate(xmlSecMSCngBlockCipherCtxPtr ctx,
     XMLSEC_SAFE_CAST_SIZE_TO_ULONG(inSize, inLen, return(-1), cipherName);
 
     outSize = xmlSecBufferGetSize(out);
+    if(outSize > XMLSEC_SIZE_MAX - inSize) {
+        xmlSecInternalError3("xmlSecBufferSetMaxSize", cipherName,
+            "outSize=" XMLSEC_SIZE_FMT "; inSize=" XMLSEC_SIZE_FMT, outSize, inSize);
+        return(-1);
+    }
     ret = xmlSecBufferSetMaxSize(out, outSize + inSize);
     if(ret < 0) {
         xmlSecInternalError2("xmlSecBufferSetMaxSize", cipherName,
@@ -1203,7 +1212,7 @@ xmlSecMSCngGCMBlockCipherCtxFinal(xmlSecMSCngBlockCipherCtxPtr ctx,
         /* check if we really have decrypted the numbers of bytes that we
         * requested */
         if(dwCLen != dwInSize) {
-            xmlSecInternalError3("BCryptEncrypt", cipherName,
+            xmlSecInternalError3("BCryptDecrypt", cipherName,
                 "in-size=%lu; out-size=%lu", dwInSize, dwCLen);
             return(-1);
         }

src/mscng/crypto.c

@@ -87,11 +87,6 @@ xmlSecCryptoGetFunctions_mscng(void) {
     gXmlSecMSCngFunctions->keyDataEcdsaGetKlass         = xmlSecMSCngKeyDataEcdsaGetKlass;
 #endif /* XMLSEC_NO_ECDSA */
 
-#ifdef XMLSEC_MSCNG_TODO
-    gXmlSecMSCngFunctions->keyDataGost2001GetKlass      = xmlSecMSCngKeyDataGost2001GetKlass;
-    gXmlSecMSCngFunctions->keyDataGostR3410_2012GetKlass = xmlSecMSCngKeyDataGostR3410_2012GetKlass;
-#endif /* XMLSEC_NO_GOST */
-
 #ifndef XMLSEC_NO_HMAC
     gXmlSecMSCngFunctions->keyDataHmacGetKlass          = xmlSecMSCngKeyDataHmacGetKlass;
 #endif /* XMLSEC_NO_HMAC */
@@ -146,10 +141,6 @@ xmlSecCryptoGetFunctions_mscng(void) {
     gXmlSecMSCngFunctions->transformDsaSha1GetKlass             = xmlSecMSCngTransformDsaSha1GetKlass;
 #endif /* XMLSEC_NO_SHA1 */
 
-#ifdef XMLSEC_MSCNG_TODO
-    gXmlSecMSCngFunctions->transformDsaSha256GetKlass           = xmlSecMSCngTransformDsaSha256GetKlass;
-#endif /* XMLSEC_NO_SHA256 */
-
 #endif /* XMLSEC_NO_DSA */
 
     /******************************* ECDSA ********************************/
@@ -159,10 +150,6 @@ xmlSecCryptoGetFunctions_mscng(void) {
     gXmlSecMSCngFunctions->transformEcdsaSha1GetKlass           = xmlSecMSCngTransformEcdsaSha1GetKlass;
 #endif /* XMLSEC_NO_SHA1 */
 
-#ifdef XMLSEC_MSCNG_TODO
-    gXmlSecMSCngFunctions->transformEcdsaSha224GetKlass         = xmlSecMSCngTransformEcdsaSha224GetKlass;
-#endif /* XMLSEC_NO_SHA224 */
-
 #ifndef XMLSEC_NO_SHA256
     gXmlSecMSCngFunctions->transformEcdsaSha256GetKlass         = xmlSecMSCngTransformEcdsaSha256GetKlass;
 #endif /* XMLSEC_NO_SHA256 */
@@ -177,19 +164,6 @@ xmlSecCryptoGetFunctions_mscng(void) {
 
 #endif /* XMLSEC_NO_ECDSA */
 
-    /******************************* GOST ********************************/
-#ifdef XMLSEC_MSCNG_TODO
-    gXmlSecMSCngFunctions->transformGost2001GostR3411_94GetKlass                = xmlSecMSCngTransformGost2001GostR3411_94GetKlass;
-    gXmlSecMSCngFunctions->transformGostR3410_2012GostR3411_2012_256GetKlass    = xmlSecMSCngTransformGostR3410_2012GostR3411_2012_256GetKlass;
-    gXmlSecMSCngFunctions->transformGostR3410_2012GostR3411_2012_512GetKlass    = xmlSecMSCngTransformGostR3410_2012GostR3411_2012_512GetKlass;
-#endif /* XMLSEC_NO_GOST */
-
-#ifdef XMLSEC_MSCNG_TODO
-    gXmlSecMSCngFunctions->transformGostR3411_94GetKlass                = xmlSecMSCngTransformGostR3411_94GetKlass;
-    gXmlSecMSCngFunctions->transformGostR3411_2012_256GetKlass          = xmlSecMSCngTransformGostR3411_2012_256GetKlass;
-    gXmlSecMSCngFunctions->transformGostR3411_2012_512GetKlass          = xmlSecMSCngTransformGostR3411_2012_512GetKlass;
-#endif /* XMLSEC_NO_GOST */
-
     /******************************* HMAC ********************************/
 #ifndef XMLSEC_NO_HMAC
 
@@ -213,21 +187,8 @@ xmlSecCryptoGetFunctions_mscng(void) {
     gXmlSecMSCngFunctions->transformHmacSha512GetKlass          = xmlSecMSCngTransformHmacSha512GetKlass;
 #endif /* XMLSEC_NO_SHA512 */
 
-#ifdef XMLSEC_MSCNG_TODO
-    gXmlSecMSCngFunctions->transformHmacRipemd160GetKlass       = xmlSecMSCngTransformHmacRipemd160GetKlass;
-#endif /* XMLSEC_NO_RIPEMD160 */
-
-#ifdef XMLSEC_MSCNG_TODO
-    gXmlSecMSCngFunctions->transformHmacMd5GetKlass             = xmlSecMSCngTransformHmacMd5GetKlass;
-#endif /* XMLSEC_NO_MD5 */
-
 #endif /* XMLSEC_NO_HMAC */
 
-    /******************************* RIPEMD160 ********************************/
-#ifdef XMLSEC_MSCNG_TODO
-    gXmlSecMSCngFunctions->transformRipemd160GetKlass           = xmlSecMSCngTransformRipemd160GetKlass;
-#endif /* XMLSEC_NO_RIPEMD160 */
-
     /******************************* RSA ********************************/
 #ifndef XMLSEC_NO_RSA
 

src/mscng/kt_rsa.c

@@ -468,6 +468,7 @@ xmlSecMSCngRsaOaepNodeRead(xmlSecTransformPtr transform, xmlNodePtr node,
 
     ctx = xmlSecMSCngRsaPkcs1OaepGetCtx(transform);
     xmlSecAssert2(ctx != NULL, -1);
+    xmlSecAssert2(xmlSecBufferGetSize(&(ctx->oaepParams)) == 0, -1);
 
     ret = xmlSecTransformRsaOaepReadParams(node, &(ctx->oaepParams), &algorithm);
     if (ret < 0) {

src/mscng/x509.c

@@ -76,31 +76,59 @@ static int
 xmlSecMSCngKeyDataX509Duplicate(xmlSecKeyDataPtr dst, xmlSecKeyDataPtr src) {
     PCCERT_CONTEXT srcCert = NULL;
     PCCERT_CONTEXT dstCert;
+    PCCRL_CONTEXT srcCrl = NULL;
+    PCCRL_CONTEXT dstCrl;
     xmlSecMSCngX509DataCtxPtr srcCtx;
     int ret;
 
     xmlSecAssert2(xmlSecKeyDataCheckId(dst, xmlSecMSCngKeyDataX509Id), -1);
     xmlSecAssert2(xmlSecKeyDataCheckId(src, xmlSecMSCngKeyDataX509Id), -1);
     srcCtx = xmlSecMSCngX509DataGetCtx(src);
 
-    /* duplicate the certificate store */
+    /* duplicate the certificate store; CertEnumCertificatesInStore frees the
+     * previous certificate context (see
+     * https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certenumcertificatesinstore) */
     while((srcCert = CertEnumCertificatesInStore(srcCtx->hMemStore, srcCert)) != NULL) {
         dstCert = CertDuplicateCertificateContext(srcCert);
         if(dstCert == NULL) {
             xmlSecMSCngLastError("CertDuplicateCertificateContext",
                 xmlSecKeyDataGetName(dst));
+            CertFreeCertificateContext(srcCert);
             return(-1);
         }
 
         ret = xmlSecMSCngKeyDataX509AdoptCert(dst, dstCert);
         if(ret < 0) {
             xmlSecInternalError("xmlSecMSCngKeyDataX509AdoptCert",
                 xmlSecKeyDataGetName(dst));
+            CertFreeCertificateContext(srcCert);
             CertFreeCertificateContext(dstCert);
             return(-1);
         }
     }
 
+    /* duplicate the CRLs; CertEnumCRLsInStore frees the previous CRL context
+     * (see
+     * https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certenumcrlsinstore) */
+    while((srcCrl = CertEnumCRLsInStore(srcCtx->hMemStore, srcCrl)) != NULL) {
+        dstCrl = CertDuplicateCRLContext(srcCrl);
+        if(dstCrl == NULL) {
+            xmlSecMSCngLastError("CertDuplicateCRLContext",
+                xmlSecKeyDataGetName(dst));
+            CertFreeCRLContext(srcCrl);
+            return(-1);
+        }
+
+        ret = xmlSecMSCngKeyDataX509AdoptCrl(dst, dstCrl);
+        if(ret < 0) {
+            xmlSecInternalError("xmlSecMSCngKeyDataX509AdoptCrl",
+                xmlSecKeyDataGetName(dst));
+            CertFreeCRLContext(srcCrl);
+            CertFreeCRLContext(dstCrl);
+            return(-1);
+        }
+    }
+
     if(srcCtx->cert != NULL) {
         /* have a key certificate, duplicate that */
         dstCert = CertDuplicateCertificateContext(srcCtx->cert);
@@ -254,6 +282,8 @@ xmlSecMSCngKeyDataX509AdoptCrl(xmlSecKeyDataPtr data, PCCRL_CONTEXT crl) {
         return(-1);
     }
 
+    /* this just decrements the refcount, so won't free */
+    CertFreeCRLContext(crl);
     return(0);
 }
 

src/mscng/x509vfy.c

@@ -668,32 +668,37 @@ xmlSecMSCngX509StoreContainsCert(HCERTSTORE store, CERT_NAME_BLOB* name,
     xmlSecAssert2(name != NULL, -1);
     xmlSecAssert2(cert != NULL, -1);
 
-    storeCert = CertFindCertificateInStore(store,
-        X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
-        0,
-        CERT_FIND_SUBJECT_NAME,
-        name,
-        NULL);
-    if (storeCert == NULL) {
-        return (0);
-    }
+    /* enumerate all certificates with the same subject: a valid second cert
+     * with the same subject (e.g. CA key rotation) must not be rejected just
+     * because the first matching cert does not verify;
+     * CertFindCertificateInStore frees the previous certificate context (see
+     * https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certfindcertificateinstore) */
+    while (TRUE) {
+        storeCert = CertFindCertificateInStore(store,
+            X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
+            0,
+            CERT_FIND_SUBJECT_NAME,
+            name,
+            storeCert);
+        if (storeCert == NULL) {
+            return (0);
+        }
 
-    ret = xmlSecMSCngX509StoreVerifySubject(cert, storeCert);
-    if (ret < 0) {
-        xmlSecInternalError("xmlSecMSCngX509StoreVerifySubject", NULL);
-        CertFreeCertificateContext(storeCert);
-        return(-1);
-    } else if (ret == 0) {
-        xmlSecOtherError(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED,
-            NULL,
-            "xmlSecMSCngX509StoreVerifySubject");
+        ret = xmlSecMSCngX509StoreVerifySubject(cert, storeCert);
+        if (ret < 0) {
+            xmlSecInternalError("xmlSecMSCngX509StoreVerifySubject", NULL);
+            continue;
+        } else if (ret == 0) {
+            xmlSecOtherError(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED,
+                NULL,
+                "xmlSecMSCngX509StoreVerifySubject");
+            continue;
+        }
+
+        /* success */
         CertFreeCertificateContext(storeCert);
-        return(-1);
+        return(1);
     }
-
-    /* success */
-    CertFreeCertificateContext(storeCert);
-    return(1);
 }
 
 static int

src/mscrypto/kt_rsa.c

@@ -239,6 +239,7 @@ xmlSecMSCryptoRsaPkcs1OaepProcess(xmlSecTransformPtr transform) {
     xmlSecSize keySize;
     int ret;
     HCRYPTKEY hKey = 0;
+    HCRYPTKEY hTmpKey = 0;
     DWORD dwInLen;
     DWORD dwBufLen;
     DWORD dwOutLen;
@@ -313,11 +314,11 @@ xmlSecMSCryptoRsaPkcs1OaepProcess(xmlSecTransformPtr transform) {
         outBuf = xmlSecBufferGetData(out);
         xmlSecAssert2(outBuf != NULL, -1);
 
-        /* set OAEP parameter for the key
-         *
-         * aleksey: I don't understand how this would work in multi-threaded
-         * environment or when key can be re-used multiple times
-         */
+        /* Set the OAEP parameter on a temporary duplicate key, when available,
+         * so the original key is not modified. The raw OAEP label is passed to
+         * KP_OAEP_PARAMS (a CAPI/wincrypt parameter that expects the label L,
+         * not its hash H(L); the provider computes H(L) internally).
+         * https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptsetkeyparam */
         if(xmlSecTransformCheckId(transform, xmlSecMSCryptoTransformRsaOaepId) && xmlSecBufferGetSize(&(ctx->oaepParams)) > 0) {
             xmlSecSize oaepParamsSize;
             CRYPT_DATA_BLOB oaepParams;
@@ -327,17 +328,33 @@ xmlSecMSCryptoRsaPkcs1OaepProcess(xmlSecTransformPtr transform) {
 
             oaepParamsSize = xmlSecBufferGetSize(&(ctx->oaepParams));
             XMLSEC_SAFE_CAST_SIZE_TO_ULONG(oaepParamsSize, oaepParams.cbData, return(-1), xmlSecTransformGetName(transform));
-            if (!CryptSetKeyParam(hKey, KP_OAEP_PARAMS, (const BYTE*)&oaepParams, 0)) {
+            if (!CryptDuplicateKey(hKey, NULL, 0, &hTmpKey)) {
+                xmlSecMSCryptoError("CryptDuplicateKey", xmlSecTransformGetName(transform));
+                return (-1);
+            }
+            if (!CryptSetKeyParam(hTmpKey, KP_OAEP_PARAMS, (const BYTE*)&oaepParams, 0)) {
                 xmlSecMSCryptoError("CryptSetKeyParam", xmlSecTransformGetName(transform));
+                if (hTmpKey != 0) {
+                    CryptDestroyKey(hTmpKey);
+                    hTmpKey = 0;
+                }
                 return (-1);
             }
         }
 
         /* encrypt */
-        if (!CryptEncrypt(hKey, 0, TRUE, ctx->dwFlags, outBuf, &dwInLen, dwBufLen)) {
+        if (!CryptEncrypt(((hTmpKey != 0) ? hTmpKey : hKey), 0, TRUE, ctx->dwFlags, outBuf, &dwInLen, dwBufLen)) {
             xmlSecMSCryptoError("CryptEncrypt", xmlSecTransformGetName(transform));
+            if (hTmpKey != 0) {
+                CryptDestroyKey(hTmpKey);
+                hTmpKey = 0;
+            }
             return (-1);
         }
+        if (hTmpKey != 0) {
+            CryptDestroyKey(hTmpKey);
+            hTmpKey = 0;
+        }
 
         /* The output of CryptEncrypt is in little-endian format, so we have to convert to
          * big-endian first.
@@ -359,11 +376,11 @@ xmlSecMSCryptoRsaPkcs1OaepProcess(xmlSecTransformPtr transform) {
             return (-1);
         }
 
-        /* set OAEP parameter for the key
-         *
-         * aleksey: I don't understand how this would work in multi-threaded
-         * environment or when key can be re-used multiple times
-         */
+        /* Set the OAEP parameter on a temporary duplicate key, when available,
+         * so the original key is not modified. The raw OAEP label is passed to
+         * KP_OAEP_PARAMS (a CAPI/wincrypt parameter that expects the label L,
+         * not its hash H(L); the provider computes H(L) internally).
+         * https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptsetkeyparam */
         if(xmlSecTransformCheckId(transform, xmlSecMSCryptoTransformRsaOaepId) && xmlSecBufferGetSize(&(ctx->oaepParams)) > 0) {
             xmlSecSize oaepParamsSize;
             CRYPT_DATA_BLOB oaepParams;
@@ -373,20 +390,36 @@ xmlSecMSCryptoRsaPkcs1OaepProcess(xmlSecTransformPtr transform) {
 
             oaepParamsSize = xmlSecBufferGetSize(&(ctx->oaepParams));
             XMLSEC_SAFE_CAST_SIZE_TO_ULONG(oaepParamsSize, oaepParams.cbData, { CryptDestroyKey(hKey); return(-1); }, xmlSecTransformGetName(transform));
-            if (!CryptSetKeyParam(hKey, KP_OAEP_PARAMS, (const BYTE*)&oaepParams, 0)) {
+            if (!CryptDuplicateKey(hKey, NULL, 0, &hTmpKey)) {
+                xmlSecMSCryptoError("CryptDuplicateKey", xmlSecTransformGetName(transform));
+                CryptDestroyKey(hKey);
+                return (-1);
+            }
+            if (!CryptSetKeyParam(hTmpKey, KP_OAEP_PARAMS, (const BYTE*)&oaepParams, 0)) {
                 xmlSecMSCryptoError("CryptSetKeyParam", xmlSecTransformGetName(transform));
+                if (hTmpKey != 0) {
+                    CryptDestroyKey(hTmpKey);
+                    hTmpKey = 0;
+                }
                 CryptDestroyKey(hKey);
                 return (-1);
             }
         }
 
         /* decrypt */
-        if (!CryptDecrypt(hKey, 0, TRUE, ctx->dwFlags, outBuf, &dwOutLen)) {
+        if (!CryptDecrypt(((hTmpKey != 0) ? hTmpKey : hKey), 0, TRUE, ctx->dwFlags, outBuf, &dwOutLen)) {
             xmlSecMSCryptoError("CryptDecrypt", xmlSecTransformGetName(transform));
+            if (hTmpKey != 0) {
+                CryptDestroyKey(hTmpKey);
+                hTmpKey = 0;
+            }
             CryptDestroyKey(hKey);
             return(-1);
         }
-
+        if (hTmpKey != 0) {
+            CryptDestroyKey(hTmpKey);
+            hTmpKey = 0;
+        }
         /* hKey comes from CryptGetUserKey and must be destroyed by the caller */
         CryptDestroyKey(hKey);
         hKey = 0;

src/mscrypto/xmlsec-mingw.h

@@ -149,7 +149,7 @@
 
 
 /*structures/types*/
-typedef struct _PUBKEY {
+typedef struct _DSSPUBKEY {
         DWORD magic;
         DWORD bitlen;
 } DSSPUBKEY;