Skip to content

Commit

Permalink
Do more DSA parameter sanity checking up front
Browse files Browse the repository at this point in the history 
DSA parameters have a very rigid structure.  We can do some checks
before we create a lot of `byte[]` to package the data to pack into
a different buffer to send to the OS library.

Checking primality and any other actual value coherency is left to
the underlying cryptographic library, these just filter egregious errors.
  • Loading branch information
@bartonjs
bartonjs authored Jul 11, 2024
1 parent 635f9af commit da8a603
Show file tree
Hide file tree
Showing 2 changed files with 213 additions and 13 deletions.
80 changes: 67 additions & 13 deletions src/libraries/Common/src/System/Security/Cryptography/DSAKeyFormatHelper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand All @@ -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
Expand All @@ -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),
Expand All @@ -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<byte> source,
out int bytesRead,
Expand Down
146 changes: 146 additions & 0 deletions ...Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAKeyFileTests.cs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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<CryptographicException>(
() => 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<CryptographicException>(
() => key.ImportPkcs8PrivateKey(pkcs8, out _),
"corrupted");
}
}

[Fact]
public static void NoFuzzySubjectPublicKeyInfo()
{
Expand Down

0 comments on commit da8a603

Please sign in to comment.