From 72678071e732273332c6178b4f9781df4fd93836 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mitch=20Lindgren=20=F0=9F=A6=8E?= Date: Wed, 18 Sep 2024 04:42:49 +0000 Subject: [PATCH] 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 | 4 ++ lib/ec_dsa.c | 2 +- lib/eckey.c | 3 -- lib/rsakey.c | 25 ++++++----- unittest/lib/testRsaSign.cpp | 84 ++++++++++++++++++++++++++++++++++++ version.json | 2 +- 6 files changed, 104 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dfeb4d5..b5d71ee 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/ec_dsa.c b/lib/ec_dsa.c index 79138a2..a4074d4 100644 --- a/lib/ec_dsa.c +++ b/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; } diff --git a/lib/eckey.c b/lib/eckey.c index cda1406..73424c6 100644 --- a/lib/eckey.c +++ b/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 diff --git a/lib/rsakey.c b/lib/rsakey.c index f3d45e2..682e8f3 100644 --- a/lib/rsakey.c +++ b/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 - 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 ) + if( pkRsakey->hasPrivateKey ) { - 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; + } } } diff --git a/unittest/lib/testRsaSign.cpp b/unittest/lib/testRsaSign.cpp index d7aecb1..7637524 100644 --- a/unittest/lib/testRsaSign.cpp +++ b/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( ¶ms, 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); diff --git a/version.json b/version.json index 087fadb..e929ac4 100644 --- a/version.json +++ b/version.json @@ -1 +1 @@ -{ "major": 103, "minor": 5, "patch": 0 } \ No newline at end of file +{ "major": 103, "minor": 5, "patch": 1 } \ No newline at end of file