Merged PR 11444004: Fix RSA key import regression, improve ECDSA parameter validation

In !11324214, we added pairwise consistency tests on key import per new FIPS 140-3 requirements. For DSA and ECDSA, we only run these tests if the key object has a private key, which is the correct behavior, because the PCT cannot be performed on a public key without the corresponding private key. Unfortunately, this check was omitted for RSA, which would cause SymCrypt to fastfail when importing a public key.

Also improved parameter validation for `SymCryptEcDsaSign`, and removed extraneous debug assertions in `SymCryptEckeySetValue`, which will make these functions easier to use.

Related work items: #53695133, #53957677

CHANGELOG.md

@@ -3,7 +3,11 @@
 New changes will be listed here as they are developed. The version number is determined
 prior to the creation of a new release, based on the changes contained in that release.
 
+# Version 103.5.1
+
 - Additional internal self-test changes to support FIPS 140-3 certification
+- Fixed a regression in v103.5.0 which caused FIPS self-tests to be erroneously executed when importing an RSA public key, resulting in a fastfail
+- Added parameter validation/removed unnecessary assertions in ECDSA functions to reduce sharp edges
 
 # Version 103.5.0
 

lib/ec_dsa.c

@@ -412,7 +412,7 @@ SymCryptEcDsaSign(
                                         SIZE_T                  cbSignature )
 {
     // We must have a private key to perform PCT or signature
-    if( !pKey->hasPrivateKey )
+    if( !pKey->hasPrivateKey || !(pKey->fAlgorithmInfo & SYMCRYPT_FLAG_ECKEY_ECDSA) )
     {
         return SYMCRYPT_INVALID_ARGUMENT;
     }

lib/eckey.c

@@ -281,9 +281,6 @@ SymCryptEckeySetValue(
 
     UINT32 fValidatePublicKeyOrder = SYMCRYPT_FLAG_ECKEY_PUBLIC_KEY_ORDER_VALIDATION;
 
-    SYMCRYPT_ASSERT( (cbPrivateKey==0) || (cbPrivateKey == SymCryptEcurveSizeofScalarMultiplier( pEckey->pCurve )) );
-    SYMCRYPT_ASSERT( (cbPublicKey==0) || (cbPublicKey == SymCryptEckeySizeofPublicKey( pEckey, ecPointFormat)) );
-
     // Ensure caller has specified what algorithm(s) the key will be used with
     UINT32 algorithmFlags = SYMCRYPT_FLAG_ECKEY_ECDSA | SYMCRYPT_FLAG_ECKEY_ECDH;
     // Make sure only allowed flags are specified

lib/rsakey.c

@@ -982,18 +982,21 @@ SymCryptRsakeySetValue(
             SymCryptRsaSelftest,
             SYMCRYPT_SELFTEST_ALGORITHM_RSA);
 
-        // Unconditionally set the sign flag to enable SignVerify PCT on encrypt-only keypair
+        if( pkRsakey->hasPrivateKey )
-        pkRsakey->fAlgorithmInfo |= SYMCRYPT_FLAG_RSAKEY_SIGN;
-
-        SYMCRYPT_RUN_KEY_PCT(
-            SymCryptRsaSignVerifyPct,
-            pkRsakey,
-            SYMCRYPT_PCT_RSA_SIGN );
-
-        // Unset the sign flag before returning encrypt-only keypair
-        if ( ( flags & SYMCRYPT_FLAG_RSAKEY_SIGN ) == 0 )
         {
-            pkRsakey->fAlgorithmInfo ^= SYMCRYPT_FLAG_RSAKEY_SIGN;
+            // Unconditionally set the sign flag to enable SignVerify PCT on encrypt-only keypair
+            pkRsakey->fAlgorithmInfo |= SYMCRYPT_FLAG_RSAKEY_SIGN;
+
+            SYMCRYPT_RUN_KEY_PCT(
+                SymCryptRsaSignVerifyPct,
+                pkRsakey,
+                SYMCRYPT_PCT_RSA_SIGN );
+
+                // Unset the sign flag before returning encrypt-only keypair
+                if ( ( flags & SYMCRYPT_FLAG_RSAKEY_SIGN ) == 0 )
+                {
+                    pkRsakey->fAlgorithmInfo ^= SYMCRYPT_FLAG_RSAKEY_SIGN;
+                }
         }
     }
 

unittest/lib/testRsaSign.cpp

@@ -1038,6 +1038,89 @@ testRsaSignPss()
     iprint( "\n" );
 }
 
+VOID
+testRsaExportImport()
+{
+    if( !SCTEST_LOOKUP_DISPATCHSYM(SymCryptRsaPssSign) ||
+        !SCTEST_LOOKUP_DISPATCHSYM(SymCryptRsaPssVerify) ||
+        !SCTEST_LOOKUP_DISPATCHSYM(SymCryptRsakeySizeofModulus) ||
+        !SCTEST_LOOKUP_DISPATCHSYM(SymCryptRsakeyAllocate) ||
+        !SCTEST_LOOKUP_DISPATCHSYM(SymCryptRsakeySetValue) ||
+        !SCTEST_LOOKUP_DISPATCHSYM(SymCryptRsakeyGetValue) ||
+        !SCTEST_LOOKUP_DISPATCHSYM(SymCryptRsakeyFree) ||
+        !SCTEST_LOOKUP_DISPATCHSYM(SymCryptSha256Algorithm) )
+    {
+        iprint( "    RsaExportImport skipped\n");
+        return;
+    }
+
+    iprint( "    RsaExportImport" );
+
+    RSAKEY_TESTBLOB blob;
+    PSYMCRYPT_RSAKEY pKeyPair;
+    PSYMCRYPT_RSAKEY pPubKey;
+    BYTE hash[32];
+    BYTE salt[32];
+    BYTE sig[ RSAKEY_MAXKEYSIZE ];
+    SIZE_T cbSig;
+    SYMCRYPT_ERROR scError;
+    SYMCRYPT_RSA_PARAMS params;
+
+    pKeyPair = rsaTestKeyRandom();
+    GENRANDOM( hash, sizeof( hash ) );
+
+    params.version = 1;
+    params.nBitsOfModulus = pKeyPair->nBitsOfModulus;
+    params.nPrimes = 2;
+    params.nPubExp = 1;
+
+    pPubKey = ScDispatchSymCryptRsakeyAllocate( &params, 0 );
+    CHECK( pPubKey != NULL, "?" );
+
+    scError = ScDispatchSymCryptRsakeyGetValue(
+        pKeyPair,
+        blob.abModulus, sizeof(blob.abModulus),
+        &blob.u64PubExp, 1,
+        nullptr, nullptr, 0,
+        SYMCRYPT_NUMBER_FORMAT_MSB_FIRST,
+        0 );
+    CHECK( scError == SYMCRYPT_NO_ERROR, "?" );
+
+    scError = ScDispatchSymCryptRsakeySetValue(
+        blob.abModulus, sizeof(blob.abModulus),
+        &blob.u64PubExp, 1,
+        nullptr, nullptr, 0,
+        SYMCRYPT_NUMBER_FORMAT_MSB_FIRST,
+        SYMCRYPT_FLAG_RSAKEY_SIGN,
+        pPubKey );
+    CHECK( scError == SYMCRYPT_NO_ERROR, "?" );
+
+    scError = ScDispatchSymCryptRsaPssSign(
+        pKeyPair,
+        hash, sizeof( hash ),
+        ScDispatchSymCryptSha256Algorithm, sizeof( salt ),
+        0,
+        SYMCRYPT_NUMBER_FORMAT_MSB_FIRST,
+        sig, ScDispatchSymCryptRsakeySizeofModulus( pKeyPair ),
+        &cbSig );
+    CHECK( scError == SYMCRYPT_NO_ERROR, "?" );
+
+    scError = ScDispatchSymCryptRsaPssVerify(
+        pPubKey,
+        hash, sizeof( hash ),
+        sig, cbSig,
+        SYMCRYPT_NUMBER_FORMAT_MSB_FIRST,
+        ScDispatchSymCryptSha256Algorithm,
+        sizeof( salt ),
+        0 );
+    CHECK( scError == SYMCRYPT_NO_ERROR, "?" );
+
+    ScDispatchSymCryptRsakeyFree( pKeyPair );
+    ScDispatchSymCryptRsakeyFree( pPubKey );
+
+    iprint( "\n" );
+}
+
 VOID
 testRsaSignAlgorithms()
 {
@@ -1067,6 +1150,7 @@ testRsaSignAlgorithms()
     if( isAlgorithmPresent( "RsaSignPss", FALSE ) )
     {
         testRsaSignPss();
+        testRsaExportImport();
     }
 
     nOutstandingAllocs = SYMCRYPT_INTERNAL_VOLATILE_READ64(&g_nOutstandingCheckedAllocs);

version.json

@@ -1 +1 @@
-{ "major": 103, "minor": 5, "patch": 0 }
+{ "major": 103, "minor": 5, "patch": 1 }