8000 X509Chain.Build should throw when an internal error occurs · dotnet/runtime@72e5ae9 · GitHub
[go: up one dir, main page]
Skip to content

Commit 72e5ae9

Browse files
GrabYourPitchforksGrabYourPitchforks
committed
X509Chain.Build should throw when an internal error occurs
Chain building can sometimes return nonsensical values when an internal error is encountered. This changes the behavior so that chain building will throw instead of returning nonsensical values. A compat switch is provided so apps can opt out of this behavior. Fixes CVE-2024-0057.
1 parent 21b9057 commit 72e5ae9

File tree

7 files changed

+188
-18
lines changed

7 files changed

+188
-18
lines changed

‎src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,20 @@ public static void GreaterThanOrEqualTo<T>(T actual, T greaterThanOrEqualTo, str
397397
throw new XunitException(AddOptionalUserMessage($"Expected: {actual} to be greater than or equal to {greaterThanOrEqualTo}", userMessage));
398398
}
399399

400+
/// <summary>
401+
/// Validate that a given enum value has the expected flag set.
402+
/// </summary>
403+
/// <typeparam name="T">The enum type.</typeparam>
404+
/// <param name="expected">The flag which should be present in <paramref name="actual"/>.</param>
405+
/// <param name="actual">The value which should contain the flag <paramref name="expected"/>.</param>
406+
public static void HasFlag<T>(T expected, T actual, string userMessage = null) where T : Enum
407+
{
408+
if (!actual.HasFlag(expected))
409+
{
410+
throw new XunitException(AddOptionalUserMessage($"Expected: Value {actual} (of enum type {typeof(T).FullName}) to have the flag {expected} set.", userMessage));
411+
}
412+
}
413+
400414
// NOTE: Consider using SequenceEqual below instead, as it will give more useful information about what
401415
// the actual differences are, especially for large arrays/spans.
402416
/// <summary>

‎src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedCmsTests.netcoreapp.cs

Lines changed: 10 additions & 3 deletions
SignedCms cms = new SignedCms(content, false);
 DEC0
Original file line numberDiff line numberDiff line change
@@ -399,7 +399,10 @@ public static void AddSigner_RSA_EphemeralKey()
399399
{
400400
ContentInfo content = new ContentInfo(new byte[] { 1, 2, 3 });
401401
402-
CmsSigner signer = new CmsSigner(certWithEphemeralKey);
402+
CmsSigner signer = new CmsSigner(certWithEphemeralKey)
403+
{
404+
IncludeOption = X509IncludeOption.EndCertOnly
405+
};
403406
cms.ComputeSignature(signer);
404407
}
405408
}
@@ -429,7 +432,8 @@ public static void AddSigner_DSA_EphemeralKey()
429432
SignedCms cms = new SignedCms(content, false);
430433
CmsSigner signer = new CmsSigner(certWithEphemeralKey)
431434
{
432-
DigestAlgorithm = new Oid(Oids.Sha1, Oids.Sha1)
435+
DigestAlgorithm = new Oid(Oids.Sha1, Oids.Sha1),
436+
IncludeOption = X509IncludeOption.EndCertOnly
433437
};
434438
cms.ComputeSignature(signer);
435439
}
@@ -458,7 +462,10 @@ public static void AddSigner_ECDSA_EphemeralKey()
458462
{
459463
ContentInfo content = new ContentInfo(new byte[] { 1, 2, 3 });
460464
SignedCms cms = new SignedCms(content, false);
461-
CmsSigner signer = new CmsSigner(certWithEphemeralKey);
465+
CmsSigner signer = new CmsSigner(certWithEphemeralKey)
466+
{
467+
IncludeOption = X509IncludeOption.EndCertOnly
468+
};
462469
cms.ComputeSignature(signer);
463470
}
464471
}

‎src/libraries/System.Security.Cryptography/src/Resources/Strings.resx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -834,4 +834,7 @@
834834
<data name="Cryptography_X509_PfxWithoutPassword_ProblemFound" xml:space="preserve">
835835
<value>There was a problem with the PKCS12 (PFX) without a supplied password. See https://go.microsoft.com/fwlink/?linkid=2233907 for more information.</value>
836836
</data>
837+
<data name="Cryptography_X509_ChainBuildingFailed" xml:space="preserve">
838+
<value>An unknown chain building error occurred.</value>
839+
</data>
837840
</root>

‎src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/LocalAppContextSwitches.cs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ internal static partial class LocalAppContextSwitches
1111

1212
internal static long Pkcs12UnspecifiedPasswordIterationLimit { get; } = InitializePkcs12UnspecifiedPasswordIterationLimit();
1313

14+
internal static bool X509ChainBuildThrowOnInternalError { get; } = InitializeX509ChainBuildThrowOnInternalError();
15+
1416
private static long InitializePkcs12UnspecifiedPasswordIterationLimit()
1517
{
1618
object? data = AppContext.GetData("System.Security.Cryptography.Pkcs12UnspecifiedPasswordIterationLimit");
@@ -29,5 +31,12 @@ private static long InitializePkcs12UnspecifiedPasswordIterationLimit()
2931
return DefaultPkcs12UnspecifiedPasswordIterationLimit;
3032
}
3133
}
34+
35+
private static bool InitializeX509ChainBuildThrowOnInternalError()
36+
{
37+
// n.b. the switch defaults to TRUE if it has not been explicitly set.
38+
return AppContext.TryGetSwitch("System.Security.Cryptography.ThrowOnX509ChainBuildInternalError", out bool isEnabled)
39+
? isEnabled : true;
40+
}
3241
}
3342
}

‎src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs

Lines changed: 51 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -137,26 +137,62 @@ internal bool Build(X509Certificate2 certificate, bool throwOnException)
137137
chainPolicy.UrlRetrievalTimeout,
138138
chainPolicy.DisableCertificateDownloads);
139139

140-
if (_pal == null)
141-
return false;
142-
143-
_chainElements = new X509ChainElementCollection(_pal.ChainElements!);
144-
145-
Exception? verificationException;
146-
bool? verified = _pal.Verify(chainPolicy.VerificationFlags, out verificationException);
147-
if (!verified.HasValue)
140+
bool success = false;
141+
if (_pal is not null)
148142
{
149-
if (throwOnException)
150-
{
151-
throw verificationException!;
152-
}
153-
else
143+
_chainElements = new X509ChainElementCollection(_pal.ChainElements!);
144+
145+
Exception? verificationException;
146+
bool? verified = _pal.Verify(chainPolicy.VerificationFlags, out verificationException);
147+
if (!verified.HasValue)
154148
{
155-
verified = false;
149+
if (throwOnException)
150+
{
151+
throw verificationException!;
152+
}
153+
else
154+
{
155+
verified = false;
156+
}
156157
}
158+
159+
success = verified.Value;
160+
}
161+
162+
// There are two reasons success might be false here.
163+
//
164+
// The most common reason is that we built the chain but the chain appears to run
165+
// afoul of policy. This is represented by BuildChain returning a non-null object
166+
// and storing potential policy violations in the chain structure. The public Build
167+
// method returns false to the caller, and the caller can inspect the ChainStatus
168+
// and ChainElements properties and evaluate the failure reason against app-level
169+
// policies. If the caller does not care about these policy violations, they can
170+
// choose to ignore them and to treat chain building as successful.
171+
//
172+
// The other type of failure is that BuildChain simply can't build the chain at all.
173+
// Perhaps something within the certificate is not valid or is unsupported, or perhaps
174+
// there's an internal failure within the OS layer we're invoking, etc. Whatever the
175+
// reason, we're not meaningfully able to initialize the ChainStatus property, which
176+
// means callers may observe a non-empty list of policy violations. Depending on the
177+
// caller's logic, they might incorrectly interpret this as there being no policy
178+
// violations at all, which means they might treat this condition as success.
179+
//
180+
// To avoid callers misinterpeting this latter condition as success, we'll throw an
181+
// exception, which matches general .NET API behavior when a method cannot complete
182+
// its objective. A compat switch is provided to normalize this back to a 'false'
183+
// return value for callers who cannot handle an exception here. If throwOnException
184+
// is false, it means the caller explicitly wants to suppress exceptions and normalize
185+
// them to a false return value.
186+
187+
if (!success
188+
&& throwOnException
189+
&& _pal?.ChainStatus is not { Length: > 0 }
190+
&& LocalAppContextSwitches.X509ChainBuildThrowOnInternalError)
191+
{
192+
throw new CryptographicException(SR.Cryptography_X509_ChainBuildingFailed);
157193
}
158194

159-
return verified.Value;
195+
return success;
160196
}
161197
}
162198

‎src/libraries/System.Security.Cryptography/tests/X509Certificates/ChainTests.cs

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1269,6 +1269,58 @@ public static void BuildChainForSelfSignedSha3Certificate()
12691269
}
12701270
}
12711271

1272+
[Fact]
1273+
public static void BuildChainForSelfSignedCertificate_WithSha256RsaSignature()
1274+
{
1275+
using (ChainHolder chainHolder = new ChainHolder())
1276+
using (X509Certificate2 cert = new X509Certificate2(TestData.SelfSignedCertSha256RsaBytes))
1277+
{
1278+
X509Chain chain = chainHolder.Chain;
1279+
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
1280+
chain.ChainPolicy.VerificationTime = cert.NotBefore.AddHours(2);
1281+
1282+
// No custom root of trust store means that this self-signed cert will at
1283+
// minimum be marked UntrustedRoot.
1284+
1285+
Assert.False(chain.Build(cert));
1286+
AssertExtensions.HasFlag(X509ChainStatusFlags.UntrustedRoot, chain.AllStatusFlags());
1287+
}
1288+
}
1289+
1290+
[Fact]
1291+
public static void BuildChainForSelfSignedCertificate_WithUnknownOidSignature()
1292+
{
1293+
using (ChainHolder chainHolder = new ChainHolder())
1294+
using (X509Certificate2 cert = new X509Certificate2(TestData.SelfSignedCertDummyOidBytes))
1295+
{
1296+
X509Chain chain = chainHolder.Chain;
1297+
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
1298+
chain.ChainPolicy.VerificationTime = cert.NotBefore.AddHours(2);
1299+
1300+
// This tests a self-signed cert whose signature block contains a garbage signing alg OID.
1301+
// Some platforms return NotSignatureValid to indicate that they cannot understand the
1302+
// signature block. Other platforms return PartialChain to indicate that they think the
1303+
// bad signature block might correspond to some unknown, untrusted signer. Yet other
1304+
// platforms simply fail the operation; e.g., Windows's CertGetCertificateChain API returns
1305+
// NTE_BAD_ALGID, which we bubble up as CryptographicException.
1306+
1307+
if (PlatformDetection.UsesAppleCrypto)
1308+
{
1309+
Assert.False(chain.Build(cert));
1310+
AssertExtensions.HasFlag(X509ChainStatusFlags.PartialChain, chain.AllStatusFlags());
1311+
}
1312+
else if (PlatformDetection.IsOpenSslSupported)
1313+
{
1314+
Assert.False(chain.Build(cert));
1315+
AssertExtensions.HasFlag(X509ChainStatusFlags.NotSignatureValid, chain.AllStatusFlags());
1316+
}
1317+
else
1318+
{
1319+
Assert.ThrowsAny<CryptographicException>(() => chain.Build(cert));
1320+
}
1321+
}
1322+
}
1323+
12721324
internal static X509ChainStatusFlags AllStatusFlags(this X509Chain chain)
12731325
{
12741326
return chain.ChainStatus.Aggregate(

‎src/libraries/System.Security.Cryptography/tests/X509Certificates/TestData.cs

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4224,5 +4224,54 @@ internal static DSAParameters GetDSA1024Params()
42244224
"09463C6E50BCA36EB3F8BCB00D8A415D2D0DB5AE08303B301F300706052B0E03" +
42254225
"021A0414A57105D833610A6D07EBFBE51E5486CD3F8BCE0D0414DB32290CC077" +
42264226
"37E9D9446E37F104FA876C861C0102022710").HexToByteArray();
4227+
4228+
// Used for chain building tests (CN=Test chain building)
4229+
internal static readonly byte[] SelfSignedCertSha256RsaBytes = (
4230+
"308202BD308201A5A003020102020900EF79C10DFD657168300D06092A864886F70D0101" +
4231+
"0B0500301E311C301A060355040313135465737420636861696E206275696C64696E6730" +
4232+
"1E170D3231313031333231353735335A170D3232313031333231353735335A301E311C30" +
4233+
"1A060355040313135465737420636861696E206275696C64696E6730820122300D06092A" +
4234+
"864886F70D01010105000382010F003082010A0282010100E3B5BBF862313DEAA9172788" +
4235+
"278B26A3EAB61B9B0326F5CEA91B1A6C6DFD156836A2363BFAC5B0F4A78F4CFF5A11F35A" +
4236+
"831C6D7935D1DFD13DD81DA29AA0645CBA9F4D20BF991C625E6D61CF396C15914DEE41F6" +
4237+
"1190E97B52BFF7AE52B79FD0E2EEE3319EC23C30D27A52A2E8A963557B12BEC0664ADEF9" +
4238+
"3C520B587EC5DABFBC70980DB7473414B4B6BF982EA9AA0969F2A76AA085464AE78DFB2B" +
4239+
"F04BDE7192874679193119C2AABEC04D360F61925921660BF09A0489B7C53464F5FC35B8" +
4240+
"612F5B993D544475C20AC46CD350A34551FEA0ACBD138D8B72F79052BF0EB3BD794A426C" +
4241+
"0117CB77B4F311FFD1C628F8E438E5474509AD51FA035558771546310203010001300D06" +
4242+
"092A864886F70D01010B050003820101000A12CE2FC3DC854113D179725E9D9ADD013A42" +
4243+
"D66340CEA7A465D54EC357AD8FED1828862D8B5C32EB3D21FC8B26A7CFA9D9FB36F593CC" +
4244+
"6AD30C25C96E8100C3F07B1B51430245EE995864749C53B409260B4040705654710C236F" +
4245+
"D9B7DE3F3BE5E6E5047712C5E506419106A57C5290BB206A97F6A3FCC4B4C83E25C3FC6D" +
4246+
"2BAB03B941374086265EE08A90A8C72A63A4053044B9FA3ABD5ED5785CFDDB15A6A327BD" +
4247+
"C0CC2B115B9D33BD6E528E35670E5A6A8D9CF52199F8D693315C60D9ADAD54EF7FDCED36" +
4248+
"0C8C79E84D42AB5CB6355A70951B1ABF1F2B3FB8BEB7E3A8D6BA2293C0DB8C86B0BB060F" +
4249+
"0D6DB9939E88B998662A27F092634BBF21F58EEAAA").HexToByteArray();
4250+
4251+
// This is nearly identical to the cert in Pkcs7SelfSignedCertSha256RsaBytes,
4252+
// but we've replaced the OID (1.2.840.113549.1.1.11 sha256RSA) with a dummy OID
4253+
// 1.3.9999.1234.5678.1234. The cert should load properly into an X509Certificate2
4254+
// object but will cause chain building to fail.
4255+
internal static readonly byte[] SelfSignedCertDummyOidBytes = (
4256+
"308202BD308201A5A003020102020900EF79C10DFD657168300D06092A864886F70D0101" +
4257+
"0B0500301E311C301A060355040313135465737420636861696E206275696C64696E6730" +
4258+
"1E170D3231313031333231353735335A170D3232313031333231353735335A301E311C30" +
4259+
"1A060355040313135465737420636861696E206275696C64696E6730820122300D06092A" +
4260+
"864886F70D01010105000382010F003082010A0282010100E3B5BBF862313DEAA9172788" +
4261+
"278B26A3EAB61B9B0326F5CEA91B1A6C6DFD156836A2363BFAC5B0F4A78F4CFF5A11F35A" +
4262+
"831C6D7935D1DFD13DD81DA29AA0645CBA9F4D20BF991C625E6D61CF396C15914DEE41F6" +
4263+
"1190E97B52BFF7AE52B79FD0E2EEE3319EC23C30D27A52A2E8A963557B12BEC0664ADEF9" +
4264+
"3C520B587EC5DABFBC70980DB7473414B4B6BF982EA9AA0969F2A76AA085464AE78DFB2B" +
4265+
"F04BDE7192874679193119C2AABEC04D360F61925921660BF09A0489B7C53464F5FC35B8" +
4266+
"612F5B993D544475C20AC46CD350A34551FEA0ACBD138D8B72F79052BF0EB3BD794A426C" +
4267+
"0117CB77B4F311FFD1C628F8E438E5474509AD51FA035558771546310203010001300D06" +
4268+
"092BCE0F8952AC2E8952050003820101000A12CE2FC3DC854113D179725E9D9ADD013A42" +
4269+
"D66340CEA7A465D54EC357AD8FED1828862D8B5C32EB3D21FC8B26A7CFA9D9FB36F593CC" +
4270+
"6AD30C25C96E8100C3F07B1B51430245EE995864749C53B409260B4040705654710C236F" +
4271+
"D9B7DE3F3BE5E6E5047712C5E506419106A57C5290BB206A97F6A3FCC4B4C83E25C3FC6D" +
4272+
"2BAB03B941374086265EE08A90A8C72A63A4053044B9FA3ABD5ED5785CFDDB15A6A327BD" +
4273+
"C0CC2B115B9D33BD6E528E35670E5A6A8D9CF52199F8D693315C60D9ADAD54EF7FDCED36" +
4274+
"0C8C79E84D42AB5CB6355A70951B1ABF1F2B3FB8BEB7E3A8D6BA2293C0DB8C86B0BB060F" +
4275+
"0D6DB9939E88B998662A27F092634BBF21F58EEAAA").HexToByteArray();
42274276
}
42284277
}

0 commit comments

Comments
 (0)
0