From b80aafdcd218e931729cd1bdcee77b5a16e41596 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Thu, 29 Jul 2021 15:22:12 -0400 Subject: [PATCH 1/3] Enable skipped SSC.Algorithms tests on Andorid DSAKeyGeneration.GenerateSecondMinKey and DSAKeyGeneration.GenerateMinKey no longer fail. Added validation in RSAAndroid.cs to make RSAXml.FromNonsenseXml pass. Fixes https://github.com/dotnet/runtime/issues/50580 Fixes https://github.com/dotnet/runtime/issues/50581 --- .../Security/Cryptography/RSAAndroid.cs | 47 +++++++++++++++++++ .../DSA/DSAKeyGeneration.cs | 2 - .../AlgorithmImplementations/RSA/RSAXml.cs | 1 - 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs b/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs index 6f5a3c88fba10..409b8297d2f5b 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs @@ -381,6 +381,53 @@ public override void ImportParameters(RSAParameters parameters) ValidateParameters(ref parameters); ThrowIfDisposed(); + if (parameters.Exponent == null || parameters.Modulus == null) + { + throw new CryptographicException(SR.Cryptography_InvalidRsaParameters); + } + + if (parameters.D == null) + { + if (parameters.P != null || + parameters.DP != null || + parameters.Q != null || + parameters.DQ != null || + parameters.InverseQ != null) + { + throw new CryptographicException(SR.Cryptography_InvalidRsaParameters); + } + } + else + { + if (parameters.P == null || + parameters.DP == null || + parameters.Q == null || + parameters.DQ == null || + parameters.InverseQ == null) + { + throw new CryptographicException(SR.Cryptography_InvalidRsaParameters); + } + + // Half, rounded up. + int halfModulusLength = (parameters.Modulus.Length + 1) / 2; + + // The same checks are done by RSACryptoServiceProvider on import (when building the key blob) + // Historically RSACng let CNG handle this (reporting NTE_NOT_SUPPORTED), but on RS1 CNG let the + // import succeed, then on private key use (e.g. signing) it would report NTE_INVALID_PARAMETER. + // + // Doing the check here prevents the state in RS1 where the Import succeeds, but corrupts the key, + // and makes for a friendlier exception message. + if (parameters.D.Length != parameters.Modulus.Length || + parameters.P.Length != halfModulusLength || + parameters.Q.Length != halfModulusLength || + parameters.DP.Length != halfModulusLength || + parameters.DQ.Length != halfModulusLength || + parameters.InverseQ.Length != halfModulusLength) + { + throw new CryptographicException(SR.Cryptography_InvalidRsaParameters); + } + } + SafeRsaHandle key = Interop.AndroidCrypto.RsaCreate(); bool imported = false; diff --git a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAKeyGeneration.cs b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAKeyGeneration.cs index bb65dd3d95017..d26dce7b36712 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAKeyGeneration.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAKeyGeneration.cs @@ -24,14 +24,12 @@ public static void VerifyDefaultKeySize_Fips186_2() } [ConditionalFact(nameof(SupportsKeyGeneration))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/50580", TestPlatforms.Android)] public static void GenerateMinKey() { GenerateKey(dsa => GetMin(dsa.LegalKeySizes)); } [ConditionalFact(nameof(SupportsKeyGeneration), nameof(HasSecondMinSize))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/50580", TestPlatforms.Android)] public static void GenerateSecondMinKey() { GenerateKey(dsa => GetSecondMin(dsa.LegalKeySizes)); diff --git a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAXml.cs b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAXml.cs index 0427e190772b5..4885b02b231e0 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAXml.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAXml.cs @@ -1340,7 +1340,6 @@ public static void FromInvalidXml() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/29515", TestPlatforms.OSX | TestPlatforms.Android)] public static void FromNonsenseXml() { // This is DiminishedDPParameters XML, but with a P that is way too long. From f63a5ed67f2118494f3a5054a7ffdaf25f13ac42 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Thu, 29 Jul 2021 17:41:38 -0400 Subject: [PATCH 2/3] Update src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs Co-authored-by: Jeremy Barton --- .../Common/src/System/Security/Cryptography/RSAAndroid.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs b/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs index 409b8297d2f5b..ffa595566cb76 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs @@ -411,12 +411,7 @@ public override void ImportParameters(RSAParameters parameters) // Half, rounded up. int halfModulusLength = (parameters.Modulus.Length + 1) / 2; - // The same checks are done by RSACryptoServiceProvider on import (when building the key blob) - // Historically RSACng let CNG handle this (reporting NTE_NOT_SUPPORTED), but on RS1 CNG let the - // import succeed, then on private key use (e.g. signing) it would report NTE_INVALID_PARAMETER. - // - // Doing the check here prevents the state in RS1 where the Import succeeds, but corrupts the key, - // and makes for a friendlier exception message. + // Matching the .NET Framework RSACryptoServiceProvider behavior, as that's the .NET de facto standard if (parameters.D.Length != parameters.Modulus.Length || parameters.P.Length != halfModulusLength || parameters.Q.Length != halfModulusLength || From 387920a4f3baf61dcb785db7f84df81e8a4a9460 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Fri, 30 Jul 2021 10:01:54 -0400 Subject: [PATCH 3/3] Added comment and added back the active issue for OSX --- .../Common/src/System/Security/Cryptography/RSAAndroid.cs | 2 ++ .../Cryptography/AlgorithmImplementations/RSA/RSAXml.cs | 1 + 2 files changed, 3 insertions(+) diff --git a/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs b/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs index ffa595566cb76..afe27f535cd26 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/RSAAndroid.cs @@ -386,6 +386,8 @@ public override void ImportParameters(RSAParameters parameters) throw new CryptographicException(SR.Cryptography_InvalidRsaParameters); } + // Check that either all parameters are not null or all are null, if a subset were set, then the parameters are invalid. + // If the parameters are all not null, verify the integrity of their lengths. if (parameters.D == null) { if (parameters.P != null || diff --git a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAXml.cs b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAXml.cs index 4885b02b231e0..666f9bea3c409 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAXml.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAXml.cs @@ -1340,6 +1340,7 @@ public static void FromInvalidXml() } [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/29515", TestPlatforms.OSX)] public static void FromNonsenseXml() { // This is DiminishedDPParameters XML, but with a P that is way too long.