diff --git a/src/libraries/Common/src/System/Security/Cryptography/DSAKeyFormatHelper.cs b/src/libraries/Common/src/System/Security/Cryptography/DSAKeyFormatHelper.cs index 63c7b451e142b..f8ebe2442b359 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/DSAKeyFormatHelper.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/DSAKeyFormatHelper.cs @@ -25,16 +25,6 @@ internal static void ReadDsaPrivateKey( throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding); } - DssParms parms = DssParms.Decode(algId.Parameters.Value, AsnEncodingRules.BER); - - ret = new DSAParameters - { - P = parms.P.ToByteArray(isUnsigned: true, isBigEndian: true), - Q = parms.Q.ToByteArray(isUnsigned: true, isBigEndian: true), - }; - - ret.G = parms.G.ExportKeyParameter(ret.P.Length); - BigInteger x; try @@ -57,6 +47,33 @@ internal static void ReadDsaPrivateKey( throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e); } + DssParms parms = DssParms.Decode(algId.Parameters.Value, AsnEncodingRules.BER); + + // Sanity checks from FIPS 186-4 4.1/4.2. Since FIPS 186-5 withdrew DSA/DSS + // these will never change again. + // + // This technically allows a non-standard combination of 1024-bit P and 256-bit Q, + // but that will get filtered out by the underlying provider. + // These checks just prevent obviously bad data from wasting work on reinterpretation. + if (parms.P.Sign < 0 || + parms.Q.Sign < 0 || + !IsValidPLength(parms.P.GetBitLength()) || + !IsValidQLength(parms.Q.GetBitLength()) || + parms.G <= 1 || + parms.G >= parms.P || + x <= 1 || + x >= parms.Q) + { + throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding); + } + + ret = new DSAParameters + { + P = parms.P.ToByteArray(isUnsigned: true, isBigEndian: true), + Q = parms.Q.ToByteArray(isUnsigned: true, isBigEndian: true), + }; + + ret.G = parms.G.ExportKeyParameter(ret.P.Length); ret.X = x.ExportKeyParameter(ret.Q.Length); // The public key is not contained within the format, calculate it. @@ -69,6 +86,11 @@ internal static void ReadDsaPublicKey( in AlgorithmIdentifierAsn algId, out DSAParameters ret) { + if (!algId.Parameters.HasValue) + { + throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding); + } + BigInteger y; try @@ -88,13 +110,26 @@ internal static void ReadDsaPublicKey( throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e); } - if (!algId.Parameters.HasValue) + DssParms parms = DssParms.Decode(algId.Parameters.Value, AsnEncodingRules.BER); + + // Sanity checks from FIPS 186-4 4.1/4.2. Since FIPS 186-5 withdrew DSA/DSS + // these will never change again. + // + // This technically allows a non-standard combination of 1024-bit P and 256-bit Q, + // but that will get filtered out by the underlying provider. + // These checks just prevent obviously bad data from wasting work on reinterpretation. + if (parms.P.Sign < 0 || + parms.Q.Sign < 0 || + !IsValidPLength(parms.P.GetBitLength()) || + !IsValidQLength(parms.Q.GetBitLength()) || + parms.G <= 1 || + parms.G >= parms.P || + y <= 1 || + y >= parms.P) { throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding); } - DssParms parms = DssParms.Decode(algId.Parameters.Value, AsnEncodingRules.BER); - ret = new DSAParameters { P = parms.P.ToByteArray(isUnsigned: true, isBigEndian: true), @@ -105,6 +140,25 @@ internal static void ReadDsaPublicKey( ret.Y = y.ExportKeyParameter(ret.P.Length); } + private static bool IsValidPLength(long pBitLength) + { + return pBitLength switch + { + // FIPS 186-3/186-4 + 1024 or 2048 or 3072 => true, + // FIPS 186-1/186-2 + >= 512 and < 1024 => pBitLength % 64 == 0, + _ => false, + }; + } + + private static bool IsValidQLength(long qBitLength) + { + // FIPS 186-1/186-2 only allows 160 + // FIPS 186-3/186-4 allow 160/224/256 + return qBitLength is 160 or 224 or 256; + } + internal static void ReadSubjectPublicKeyInfo( ReadOnlySpan source, out int bytesRead, diff --git a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAKeyFileTests.cs b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAKeyFileTests.cs index 980a8a9194c83..3d655f77ea96b 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAKeyFileTests.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAKeyFileTests.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Formats.Asn1; +using System.Numerics; using System.Security.Cryptography.Encryption.RC2.Tests; using System.Text; using Test.Cryptography; @@ -302,6 +304,150 @@ public static void ReadWriteDsa2048SubjectPublicKeyInfo() DSATestData.GetDSA2048Params()); } + [Fact] + [SkipOnPlatform(TestPlatforms.OSX, "DSASecurityTransforms goes straight to OS, has different failure mode")] + public static void ImportNonsensePublicParameters() + { + AsnWriter writer = new AsnWriter(AsnEncodingRules.DER); + + DSAParameters validParameters = DSATestData.GetDSA2048Params(); + BigInteger p = new BigInteger(validParameters.P, true, true); + BigInteger q = new BigInteger(validParameters.Q, true, true); + BigInteger g = new BigInteger(validParameters.G, true, true); + BigInteger y = new BigInteger(validParameters.Y, true, true); + + using (DSA dsa = DSAFactory.Create()) + { + // 1 < y < p, 1 < g < p, q is 160/224/256 bits + // p is 512..1024 % 64, or 1024/2048/3072 bits + ImportSPKI(dsa, p, q, g, p, writer); + ImportSPKI(dsa, p, q, g, BigInteger.One, writer); + ImportSPKI(dsa, p, q, g, BigInteger.MinusOne, writer); + ImportSPKI(dsa, p, q, p, y, writer); + ImportSPKI(dsa, p, q, -g, y, writer); + ImportSPKI(dsa, p, q, BigInteger.One, y, writer); + ImportSPKI(dsa, p, q, BigInteger.MinusOne, y, writer); + ImportSPKI(dsa, p, q << 1, g, y, writer); + ImportSPKI(dsa, p, q >> 1, g, y, writer); + ImportSPKI(dsa, p, -q, g, y, writer); + ImportSPKI(dsa, p >> 1, q, g, y, writer); + ImportSPKI(dsa, p << 1, q, g, y, writer); + ImportSPKI(dsa, BigInteger.One << 4095, q, 2, 97, writer); + } + + static void ImportSPKI( + DSA key, + BigInteger p, + BigInteger q, + BigInteger g, + BigInteger y, + AsnWriter writer) + { + writer.Reset(); + writer.WriteInteger(y); + byte[] encodedPublicKey = writer.Encode(); + writer.Reset(); + + using (writer.PushSequence()) + { + using (writer.PushSequence()) + { + writer.WriteObjectIdentifier("1.2.840.10040.4.1"); + + using (writer.PushSequence()) + { + writer.WriteInteger(p); + writer.WriteInteger(q); + writer.WriteInteger(g); + } + } + + writer.WriteBitString(encodedPublicKey); + } + + byte[] spki = writer.Encode(); + writer.Reset(); + + AssertExtensions.ThrowsContains( + () => key.ImportSubjectPublicKeyInfo(spki, out _), + "corrupted"); + } + } + + [Fact] + public static void ImportNonsensePrivateParameters() + { + AsnWriter writer = new AsnWriter(AsnEncodingRules.DER); + + DSAParameters validParameters = DSATestData.GetDSA2048Params(); + BigInteger p = new BigInteger(validParameters.P, true, true); + BigInteger q = new BigInteger(validParameters.Q, true, true); + BigInteger g = new BigInteger(validParameters.G, true, true); + BigInteger x = new BigInteger(validParameters.X, true, true); + + using (DSA dsa = DSAFactory.Create()) + { + // 1 < x < q, 1 < g < p, q is 160/224/256 bits + // p is 512..1024 % 64, or 1024/2048/3072 bits + ImportPkcs8(dsa, p, q, g, q, writer); + ImportPkcs8(dsa, p, q, g, BigInteger.One, writer); + // x = -1 gets re-interpreted as x = 255 because of a CAPI compat issue. + //ImportPkcs8(dsa, p, q, g, BigInteger.MinusOne, writer); + ImportPkcs8(dsa, p, q, g, -x, writer); + ImportPkcs8(dsa, p, q, p, x, writer); + ImportPkcs8(dsa, p, q, -g, x, writer); + ImportPkcs8(dsa, p, q, BigInteger.One, x, writer); + ImportPkcs8(dsa, p, q, BigInteger.MinusOne, x, writer); + ImportPkcs8(dsa, p, q << 1, g, x, writer); + ImportPkcs8(dsa, p, q >> 1, g, x, writer); + ImportPkcs8(dsa, p >> 1, q, g, x, writer); + ImportPkcs8(dsa, p << 1, q, g, x, writer); + ImportPkcs8(dsa, -q, q, g, x, writer); + ImportPkcs8(dsa, BigInteger.One << 4095, q, 2, 97, writer); + ImportPkcs8(dsa, -p, q, g, x, writer); + } + + static void ImportPkcs8( + DSA key, + BigInteger p, + BigInteger q, + BigInteger g, + BigInteger x, + AsnWriter writer) + { + writer.Reset(); + + using (writer.PushSequence()) + { + writer.WriteInteger(0); + + using (writer.PushSequence()) + { + writer.WriteObjectIdentifier("1.2.840.10040.4.1"); + + using (writer.PushSequence()) + { + writer.WriteInteger(p); + writer.WriteInteger(q); + writer.WriteInteger(g); + } + } + + using (writer.PushOctetString()) + { + writer.WriteInteger(x); + } + } + + byte[] pkcs8 = writer.Encode(); + writer.Reset(); + + AssertExtensions.ThrowsContains( + () => key.ImportPkcs8PrivateKey(pkcs8, out _), + "corrupted"); + } + } + [Fact] public static void NoFuzzySubjectPublicKeyInfo() {