Skip to content

Commit da8a603

Browse files
bartonjsbartonjs
authored
Do more DSA parameter sanity checking up front
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.
1 parent 635f9af commit da8a603

File tree

2 files changed

+213
-13
lines changed

2 files changed

+213
-13
lines changed

‎src/libraries/Common/src/System/Security/Cryptography/DSAKeyFormatHelper.cs

Lines changed: 67 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,16 +25,6 @@ internal static void ReadDsaPrivateKey(
2525
throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
2626
}
2727

28-
DssParms parms = DssParms.Decode(algId.Parameters.Value, AsnEncodingRules.BER);
29-
30-
ret = new DSAParameters
31-
{
32-
P = parms.P.ToByteArray(isUnsigned: true, isBigEndian: true),
33-
Q = parms.Q.ToByteArray(isUnsigned: true, isBigEndian: true),
34-
};
35-
36-
ret.G = parms.G.ExportKeyParameter(ret.P.Length);
37-
3828
BigInteger x;
3929

4030
try
@@ -57,6 +47,33 @@ internal static void ReadDsaPrivateKey(
5747
throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e);
5848
}
5949

50+
DssParms parms = DssParms.Decode(algId.Parameters.Value, AsnEncodingRules.BER);
51+
52+
// Sanity checks from FIPS 186-4 4.1/4.2. Since FIPS 186-5 withdrew DSA/DSS
53+
// these will never change again.
54+
//
55+
// This technically allows a non-standard combination of 1024-bit P and 256-bit Q,
56+
// but that will get filtered out by the underlying provider.
57+
// These checks just prevent obviously bad data from wasting work on reinterpretation.
58+
if (parms.P.Sign < 0 ||
59+
parms.Q.Sign < 0 ||
60+
!IsValidPLength(parms.P.GetBitLength()) ||
61+
!IsValidQLength(parms.Q.GetBitLength()) ||
62+
parms.G <= 1 ||
63+
parms.G >= parms.P ||
64+
x <= 1 ||
65+
x >= parms.Q)
66+
{
67+
throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
68+
}
69+
70+
ret = new DSAParameters
71+
{
72+
P = parms.P.ToByteArray(isUnsigned: true, isBigEndian: true),
73+
Q = parms.Q.ToByteArray(isUnsigned: true, isBigEndian: true),
74+
};
75+
76+
ret.G = parms.G.ExportKeyParameter(ret.P.Length);
6077
ret.X = x.ExportKeyParameter(ret.Q.Length);
6178

6279
// The public key is not contained within the format, calculate it.
@@ -69,6 +86,11 @@ internal static void ReadDsaPublicKey(
6986
in AlgorithmIdentifierAsn algId,
7087
out DSAParameters ret)
7188
{
89+
if (!algId.Parameters.HasValue)
90+
{
91+
throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
92+
}
93+
7294
BigInteger y;
7395

7496
try
@@ -88,13 +110,26 @@ internal static void ReadDsaPublicKey(
88110
throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e);
89111
}
90112

91-
if (!algId.Parameters.HasValue)
113+
DssParms parms = DssParms.Decode(algId.Parameters.Value, AsnEncodingRules.BER);
114+
115+
// Sanity checks from FIPS 186-4 4.1/4.2. Since FIPS 186-5 withdrew DSA/DSS
116+
// these will never change again.
117+
//
118+
// This technically allows a non-standard combination of 1024-bit P and 256-bit Q,
119+
// but that will get filtered out by the underlying provider.
120+
// These checks just prevent obviously bad data from wasting work on reinterpretation.
121+
if (parms.P.Sign < 0 ||
122+
parms.Q.Sign < 0 ||
123+
!IsValidPLength(parms.P.GetBitLength()) ||
124+
!IsValidQLength(parms.Q.GetBitLength()) ||
125+
parms.G <= 1 ||
126+
parms.G >= parms.P ||
127+
y <= 1 ||
128+
y >= parms.P)
92129
{
93130
throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
94131
}
95132

96-
DssParms parms = DssParms.Decode(algId.Parameters.Value, AsnEncodingRules.BER);
97-
98133
ret = new DSAParameters
99134
{
100135
P = parms.P.ToByteArray(isUnsigned: true, isBigEndian: true),
@@ -105,6 +140,25 @@ internal static void ReadDsaPublicKey(
105140
ret.Y = y.ExportKeyParameter(ret.P.Length);
106141
}
107142

143+
private static bool IsValidPLength(long pBitLength)
144+
{
145+
return pBitLength switch
146+
{
147+
// FIPS 186-3/186-4
148+
1024 or 2048 or 3072 => true,
149+
// FIPS 186-1/186-2
150+
>= 512 and < 1024 => pBitLength % 64 == 0,
151+
_ => false,
152+
};
153+
}
154+
155+
private static bool IsValidQLength(long qBitLength)
156+
{
157+
// FIPS 186-1/186-2 only allows 160
158+
// FIPS 186-3/186-4 allow 160/224/256
159+
return qBitLength is 160 or 224 or 256;
160+
}
161+
108162
internal static void ReadSubjectPublicKeyInfo(
109163
ReadOnlySpan<byte> source,
110164
out int bytesRead,

‎src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAKeyFileTests.cs

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System.Formats.Asn1;
5+
using System.Numerics;
46
using System.Security.Cryptography.Encryption.RC2.Tests;
57
using System.Text;
68
using Test.Cryptography;
@@ -302,6 +304,150 @@ public static void ReadWriteDsa2048SubjectPublicKeyInfo()
302304
DSATestData.GetDSA2048Params());
303305
}
304306

307+
[Fact]
308+
[SkipOnPlatform(TestPlatforms.OSX, "DSASecurityTransforms goes straight to OS, has different failure mode")]
309+
public static void ImportNonsensePublicParameters()
310+
{
311+
AsnWriter writer = new AsnWriter(AsnEncodingRules.DER);
312+
313+
DSAParameters validParameters = DSATestData.GetDSA2048Params();
314+
BigInteger p = new BigInteger(validParameters.P, true, true);
315+
BigInteger q = new BigInteger(validParameters.Q, true, true);
316+
BigInteger g = new BigInteger(validParameters.G, true, true);
317+
BigInteger y = new BigInteger(validParameters.Y, true, true);
318+
319+
using (DSA dsa = DSAFactory.Create())
320+
{
321+
// 1 < y < p, 1 < g < p, q is 160/224/256 bits
322+
// p is 512..1024 % 64, or 1024/2048/3072 bits
323+
ImportSPKI(dsa, p, q, g, p, writer);
324+
ImportSPKI(dsa, p, q, g, BigInteger.One, writer);
325+
ImportSPKI(dsa, p, q, g, BigInteger.MinusOne, writer);
326+
ImportSPKI(dsa, p, q, p, y, writer);
327+
ImportSPKI(dsa, p, q, -g, y, writer);
328+
ImportSPKI(dsa, p, q, BigInteger.One, y, writer);
329+
ImportSPKI(dsa, p, q, BigInteger.MinusOne, y, writer);
330+
ImportSPKI(dsa, p, q << 1, g, y, writer);
331+
ImportSPKI(dsa, p, q >> 1, g, y, writer);
332+
ImportSPKI(dsa, p, -q, g, y, writer);
333+
ImportSPKI(dsa, p >> 1, q, g, y, writer);
334+
ImportSPKI(dsa, p << 1, q, g, y, writer);
335+
ImportSPKI(dsa, BigInteger.One << 4095, q, 2, 97, writer);
336+
}
337+
338+
static void ImportSPKI(
339+
DSA key,
340+
BigInteger p,
341+
BigInteger q,
342+
BigInteger g,
343+
BigInteger y,
344+
AsnWriter writer)
345+
{
346+
writer.Reset();
347+
writer.WriteInteger(y);
348+
byte[] encodedPublicKey = writer.Encode();
349+
writer.Reset();
350+
351+
using (writer.PushSequence())
352+
{
353+
using (writer.PushSequence())
354+
{
355+
writer.WriteObjectIdentifier("1.2.840.10040.4.1");
356+
357+
using (writer.PushSequence())
358+
{
359+
writer.WriteInteger(p);
360+
writer.WriteInteger(q);
361+
writer.WriteInteger(g);
362+
}
363+
}
364+
365+
writer.WriteBitString(encodedPublicKey);
366+
}
367+
368+
byte[] spki = writer.Encode();
369+
writer.Reset();
370+
371+
AssertExtensions.ThrowsContains<CryptographicException>(
372+
() => key.ImportSubjectPublicKeyInfo(spki, out _),
373+
"corrupted");
374+
}
375+
}
376+
377+
[Fact]
378+
public static void ImportNonsensePrivateParameters()
379+
{
380+
AsnWriter writer = new AsnWriter(AsnEncodingRules.DER);
381+
382+
DSAParameters validParameters = DSATestData.GetDSA2048Params();
383+
BigInteger p = new BigInteger(validParameters.P, true, true);
384+
BigInteger q = new BigInteger(validParameters.Q, true, true);
385+
BigInteger g = new BigInteger(validParameters.G, true, true);
386+
BigInteger x = new BigInteger(validParameters.X, true, true);
387+
388+
using (DSA dsa = DSAFactory.Create())
389+
{
390+
// 1 < x < q, 1 < g < p, q is 160/224/256 bits
391+
// p is 512..1024 % 64, or 1024/2048/3072 bits
392+
ImportPkcs8(dsa, p, q, g, q, writer);
393+
ImportPkcs8(dsa, p, q, g, BigInteger.One, writer);
394+
// x = -1 gets re-interpreted as x = 255 because of a CAPI compat issue.
395+
//ImportPkcs8(dsa, p, q, g, BigInteger.MinusOne, writer);
396+
ImportPkcs8(dsa, p, q, g, -x, writer);
397+
ImportPkcs8(dsa, p, q, p, x, writer);
398+
ImportPkcs8(dsa, p, q, -g, x, writer);
399+
ImportPkcs8(dsa, p, q, BigInteger.One, x, writer);
400+
ImportPkcs8(dsa, p, q, BigInteger.MinusOne, x, writer);
401+
ImportPkcs8(dsa, p, q << 1, g, x, writer);
402+
ImportPkcs8(dsa, p, q >> 1, g, x, writer);
403+
ImportPkcs8(dsa, p >> 1, q, g, x, writer);
404+
ImportPkcs8(dsa, p << 1, q, g, x, writer);
405+
ImportPkcs8(dsa, -q, q, g, x, writer);
406+
ImportPkcs8(dsa, BigInteger.One << 4095, q, 2, 97, writer);
407+
ImportPkcs8(dsa, -p, q, g, x, writer);
408+
}
409+
410+
static void ImportPkcs8(
411+
DSA key,
412+
BigInteger p,
413+
BigInteger q,
414+
BigInteger g,
415+
BigInteger x,
416+
AsnWriter writer)
417+
{
418+
writer.Reset();
419+
420+
using (writer.PushSequence())
421+
{
422+
writer.WriteInteger(0);
423+
424+
using (writer.PushSequence())
425+
{
426+
writer.WriteObjectIdentifier("1.2.840.10040.4.1");
427+
428+
using (writer.PushSequence())
429+
{
430+
writer.WriteInteger(p);
431+
writer.WriteInteger(q);
432+
writer.WriteInteger(g);
433+
}
434+
}
435+
436+
using (writer.PushOctetString())
437+
{
438+
writer.WriteInteger(x);
439+
}
440+
}
441+
442+
byte[] pkcs8 = writer.Encode();
443+
writer.Reset();
444+
445+
AssertExtensions.ThrowsContains<CryptographicException>(
446+
() => key.ImportPkcs8PrivateKey(pkcs8, out _),
447+
"corrupted");
448+
}
449+
}
450+
305451
[Fact]
306452
public static void NoFuzzySubjectPublicKeyInfo()
307453
{

0 commit comments

Comments
 (0)