Skip to content

Commit 4a8d5a0

Browse files
bartonjscarlossanlop
bartonjs
authored and
carlossanlop
committed
Limit the size of OID supported by AsnDecoder/AsnReader
1 parent 63d8096 commit 4a8d5a0

File tree

8 files changed

+464
-63
lines changed

8 files changed

+464
-63
lines changed

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

Lines changed: 69 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,34 @@ 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+
59+
if (parms.P.Sign < 0 ||
60+
parms.Q.Sign < 0 ||
61+
!IsValidPLength(parms.P.GetBitLength()) ||
62+
!IsValidQLength(parms.Q.GetBitLength()) ||
63+
parms.G <= 1 ||
64+
parms.G >= parms.P ||
65+
x <= 1 ||
66+
x >= parms.Q)
67+
{
68+
throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
69+
}
70+
71+
ret = new DSAParameters
72+
{
73+
P = parms.P.ToByteArray(isUnsigned: true, isBigEndian: true),
74+
Q = parms.Q.ToByteArray(isUnsigned: true, isBigEndian: true),
75+
};
76+
77+
ret.G = parms.G.ExportKeyParameter(ret.P.Length);
6078
ret.X = x.ExportKeyParameter(ret.Q.Length);
6179

6280
// The public key is not contained within the format, calculate it.
@@ -69,6 +87,11 @@ internal static void ReadDsaPublicKey(
6987
in AlgorithmIdentifierAsn algId,
7088
out DSAParameters ret)
7189
{
90+
if (!algId.Parameters.HasValue)
91+
{
92+
throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
93+
}
94+
7295
BigInteger y;
7396

7497
try
@@ -88,13 +111,27 @@ internal static void ReadDsaPublicKey(
88111
throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding, e);
89112
}
90113

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

96-
DssParms parms = DssParms.Decode(algId.Parameters.Value, AsnEncodingRules.BER);
97-
98135
ret = new DSAParameters
99136
{
100137
P = parms.P.ToByteArray(isUnsigned: true, isBigEndian: true),
@@ -105,6 +142,25 @@ internal static void ReadDsaPublicKey(
105142
ret.Y = y.ExportKeyParameter(ret.P.Length);
106143
}
107144

145+
private static bool IsValidPLength(long pBitLength)
146+
{
147+
return pBitLength switch
148+
{
149+
// FIPS 186-3/186-4
150+
1024 or 2048 or 3072 => true,
151+
// FIPS 186-1/186-2
152+
>= 512 and < 1024 => pBitLength % 64 == 0,
153+
_ => false,
154+
};
155+
}
156+
157+
private static bool IsValidQLength(long qBitLength)
158+
{
159+
// FIPS 186-1/186-2 only allows 160
160+
// FIPS 186-3/186-4 allow 160/224/256
161+
return qBitLength is 160 or 224 or 256;
162+
}
163+
108164
internal static void ReadSubjectPublicKeyInfo(
109165
ReadOnlySpan<byte> source,
110166
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
{

‎src/libraries/System.Formats.Asn1/src/Resources/Strings.resx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,9 @@
144144
<data name="ContentException_NamedBitListValueTooBig" xml:space="preserve">
145145
<value>The encoded named bit list value is larger than the value size of the '{0}' enum.</value>
146146
</data>
147+
<data name="ContentException_OidTooBig" xml:space="preserve">
148+
<value>The encoded object identifier (OID) exceeds the limits supported by this library. Supported OIDs are limited to 64 arcs and each subidentifier is limited to a 128-bit value.</value>
149+
</data>
147150
<data name="ContentException_PrimitiveEncodingRequired" xml:space="preserve">
148151
<value>The encoded value uses a constructed encoding, which is invalid for '{0}' values.</value>
149152
</data>

‎src/libraries/System.Formats.Asn1/src/System.Formats.Asn1.csproj

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
55
<DefineConstants>$(DefineConstants);CP_NO_ZEROMEMORY</DefineConstants>
66
<IsPackable>true</IsPackable>
7+
<GeneratePackageOnBuild>true</GeneratePackageOnBuild>
8+
<ServicingVersion>1</ServicingVersion>
79
<PackageDescription>Provides classes that can read and write the ASN.1 BER, CER, and DER data formats.
810

911
Commonly Used Types:
@@ -12,6 +14,9 @@ System.Formats.Asn1.AsnWriter</PackageDescription>
1214
</PropertyGroup>
1315

1416
<ItemGroup>
17+
<Compile Include="$(CommonPath)System\LocalAppContextSwitches.Common.cs">
18+
<Link>Common\System\LocalAppContextSwitches.Common.cs</Link>
19+
</Compile>
1520
<Compile Include="$(CommonPath)System\Security\Cryptography\CryptoPool.cs">
1621
<Link>Common\System\Security\Cryptography\CryptoPool.cs</Link>
1722
</Compile>
@@ -49,6 +54,7 @@ System.Formats.Asn1.AsnWriter</PackageDescription>
4954
<Compile Include="System\Formats\Asn1\AsnWriter.SetOf.cs" />
5055
<Compile Include="System\Formats\Asn1\AsnWriter.Text.cs" />
5156
<Compile Include="System\Formats\Asn1\AsnWriter.UtcTime.cs" />
57+
<Compile Include="System\Formats\Asn1\LocalAppContextSwitches.cs" />
5258
<Compile Include="System\Formats\Asn1\SetOfValueComparer.cs" />
5359
<Compile Include="System\Formats\Asn1\TagClass.cs" />
5460
<Compile Include="System\Formats\Asn1\UniversalTagNumber.cs" />

‎src/libraries/System.Formats.Asn1/src/System/Formats/Asn1/AsnDecoder.Oid.cs

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,15 +90,55 @@ private static void ReadSubIdentifier(
9090
throw new AsnContentException();
9191
}
9292

93+
// Set semanticBits to a value such that on the first
94+
// iteration of the loop it becomes the correct value.
95+
// So each entry here is [real semantic bits for this value] - 7.
96+
int semanticBits = source[0] switch
97+
{
98+
>= 0b1100_0000 => 0,
99+
>= 0b1010_0000 => -1,
100+
>= 0b1001_0000 => -2,
101+
>= 0b1000_1000 => -3,
102+
>= 0b1000_0100 => -4,
103+
>= 0b1000_0010 => -5,
104+
>= 0b1000_0001 => -6,
105+
_ => 0,
106+
};
107+
93108
// First, see how long the segment is
94109
int end = -1;
95110
int idx;
96111

112+
// None of T-REC-X.660-201107, T-REC-X.680-201508, or T-REC-X.690-201508
113+
// have any recommendations for a minimum (or maximum) size of a
114+
// sub-identifier.
115+
//
116+
// T-REC-X.667-201210 (and earlier versions) discuss the no-registration-
117+
// required UUID space at 2.25.{UUID}, where UUIDs are defined as 128-bit
118+
// values. This gives us a minimum lower bound of 128-bit.
119+
//
120+
// Windows Crypt32 has historically only supported 64-bit values, and
121+
// the "size limitations" FAQ on oid-info.com says that the largest arc
122+
// value is a 39-digit value that corresponds to a 2.25.UUID value.
123+
//
124+
// So, until something argues for a bigger number, our bit-limit is 128.
125+
const int MaxAllowedBits = 128;
126+
97127
for (idx = 0; idx < source.Length; idx++)
98128
{
99129
// If the high bit isn't set this marks the end of the sub-identifier.
100130
bool endOfIdentifier = (source[idx] & 0x80) == 0;
101131

132+
if (!LocalAppContextSwitches.AllowAnySizeOid)
133+
{
134+
semanticBits += 7;
135+
136+
if (semanticBits > MaxAllowedBits)
137+
{
138+
throw new AsnContentException(SR.ContentException_OidTooBig);
139+
}
140+
}
141+
102142
if (endOfIdentifier)
103143
{
104144
end = idx;
@@ -265,8 +305,21 @@ private static string ReadObjectIdentifier(ReadOnlySpan<byte> contents)
265305

266306
contents = contents.Slice(bytesRead);
267307

308+
const int MaxArcs = 64;
309+
int remainingArcs = MaxArcs - 2;
310+
268311
while (!contents.IsEmpty)
269312
{
313+
if (!LocalAppContextSwitches.AllowAnySizeOid)
314+
{
315+
if (remainingArcs <= 0)
316+
{
317+
throw new AsnContentException(SR.ContentException_OidTooBig);
318+
}
319+
320+
remainingArcs--;
321+
}
322+
270323
ReadSubIdentifier(contents, out bytesRead, out smallValue, out largeValue);
271324
// Exactly one should be non-null.
272325
Debug.Assert((smallValue == null) != (largeValue == null));

0 commit comments

Comments
 (0)