Skip to content

Commit

Permalink
X509Chain.Build should throw when an internal error occurs
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
GrabYourPitchforks committed Nov 22, 2023
1 parent 21b9057 commit 72e5ae9
Show file tree
Hide file tree
Showing 7 changed files with 188 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,20 @@ public static void GreaterThanOrEqualTo<T>(T actual, T greaterThanOrEqualTo, str
throw new XunitException(AddOptionalUserMessage($"Expected: {actual} to be greater than or equal to {greaterThanOrEqualTo}", userMessage));
}

/// <summary>
/// Validate that a given enum value has the expected flag set.
/// </summary>
/// <typeparam name="T">The enum type.</typeparam>
/// <param name="expected">The flag which should be present in <paramref name="actual"/>.</param>
/// <param name="actual">The value which should contain the flag <paramref name="expected"/>.</param>
public static void HasFlag<T>(T expected, T actual, string userMessage = null) where T : Enum
{
if (!actual.HasFlag(expected))
{
throw new XunitException(AddOptionalUserMessage($"Expected: Value {actual} (of enum type {typeof(T).FullName}) to have the flag {expected} set.", userMessage));
}
}

// NOTE: Consider using SequenceEqual below instead, as it will give more useful information about what
// the actual differences are, especially for large arrays/spans.
/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,10 @@ public static void AddSigner_RSA_EphemeralKey()
{
ContentInfo content = new ContentInfo(new byte[] { 1, 2, 3 });
SignedCms cms = new SignedCms(content, false);
CmsSigner signer = new CmsSigner(certWithEphemeralKey);
CmsSigner signer = new CmsSigner(certWithEphemeralKey)
{
IncludeOption = X509IncludeOption.EndCertOnly
};
cms.ComputeSignature(signer);
}
}
Expand Down Expand Up @@ -429,7 +432,8 @@ public static void AddSigner_DSA_EphemeralKey()
SignedCms cms = new SignedCms(content, false);
CmsSigner signer = new CmsSigner(certWithEphemeralKey)
{
DigestAlgorithm = new Oid(Oids.Sha1, Oids.Sha1)
DigestAlgorithm = new Oid(Oids.Sha1, Oids.Sha1),
IncludeOption = X509IncludeOption.EndCertOnly
};
cms.ComputeSignature(signer);
}
Expand Down Expand Up @@ -458,7 +462,10 @@ public static void AddSigner_ECDSA_EphemeralKey()
{
ContentInfo content = new ContentInfo(new byte[] { 1, 2, 3 });
SignedCms cms = new SignedCms(content, false);
CmsSigner signer = new CmsSigner(certWithEphemeralKey);
CmsSigner signer = new CmsSigner(certWithEphemeralKey)
{
IncludeOption = X509IncludeOption.EndCertOnly
};
cms.ComputeSignature(signer);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -834,4 +834,7 @@
<data name="Cryptography_X509_PfxWithoutPassword_ProblemFound" xml:space="preserve">
<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>
</data>
<data name="Cryptography_X509_ChainBuildingFailed" xml:space="preserve">
<value>An unknown chain building error occurred.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ internal static partial class LocalAppContextSwitches

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

internal static bool X509ChainBuildThrowOnInternalError { get; } = InitializeX509ChainBuildThrowOnInternalError();

private static long InitializePkcs12UnspecifiedPasswordIterationLimit()
{
object? data = AppContext.GetData("System.Security.Cryptography.Pkcs12UnspecifiedPasswordIterationLimit");
Expand All @@ -29,5 +31,12 @@ private static long InitializePkcs12UnspecifiedPasswordIterationLimit()
return DefaultPkcs12UnspecifiedPasswordIterationLimit;
}
}

private static bool InitializeX509ChainBuildThrowOnInternalError()
{
// n.b. the switch defaults to TRUE if it has not been explicitly set.
return AppContext.TryGetSwitch("System.Security.Cryptography.ThrowOnX509ChainBuildInternalError", out bool isEnabled)
? isEnabled : true;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -137,26 +137,62 @@ internal bool Build(X509Certificate2 certificate, bool throwOnException)
chainPolicy.UrlRetrievalTimeout,
chainPolicy.DisableCertificateDownloads);

if (_pal == null)
return false;

_chainElements = new X509ChainElementCollection(_pal.ChainElements!);

Exception? verificationException;
bool? verified = _pal.Verify(chainPolicy.VerificationFlags, out verificationException);
if (!verified.HasValue)
bool success = false;
if (_pal is not null)
{
if (throwOnException)
{
throw verificationException!;
}
else
_chainElements = new X509ChainElementCollection(_pal.ChainElements!);

Exception? verificationException;
bool? verified = _pal.Verify(chainPolicy.VerificationFlags, out verificationException);
if (!verified.HasValue)
{
verified = false;
if (throwOnException)
{
throw verificationException!;
}
else
{
verified = false;
}
}

success = verified.Value;
}

// There are two reasons success might be false here.
//
// The most common reason is that we built the chain but the chain appears to run
// afoul of policy. This is represented by BuildChain returning a non-null object
// and storing potential policy violations in the chain structure. The public Build
// method returns false to the caller, and the caller can inspect the ChainStatus
// and ChainElements properties and evaluate the failure reason against app-level
// policies. If the caller does not care about these policy violations, they can
// choose to ignore them and to treat chain building as successful.
//
// The other type of failure is that BuildChain simply can't build the chain at all.
// Perhaps something within the certificate is not valid or is unsupported, or perhaps
// there's an internal failure within the OS layer we're invoking, etc. Whatever the
// reason, we're not meaningfully able to initialize the ChainStatus property, which
// means callers may observe a non-empty list of policy violations. Depending on the
// caller's logic, they might incorrectly interpret this as there being no policy
// violations at all, which means they might treat this condition as success.
//
// To avoid callers misinterpeting this latter condition as success, we'll throw an
// exception, which matches general .NET API behavior when a method cannot complete
// its objective. A compat switch is provided to normalize this back to a 'false'
// return value for callers who cannot handle an exception here. If throwOnException
// is false, it means the caller explicitly wants to suppress exceptions and normalize
// them to a false return value.

if (!success
&& throwOnException
&& _pal?.ChainStatus is not { Length: > 0 }
&& LocalAppContextSwitches.X509ChainBuildThrowOnInternalError)
{
throw new CryptographicException(SR.Cryptography_X509_ChainBuildingFailed);
}

return verified.Value;
return success;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1269,6 +1269,58 @@ public static void BuildChainForSelfSignedSha3Certificate()
}
}

[Fact]
public static void BuildChainForSelfSignedCertificate_WithSha256RsaSignature()
{
using (ChainHolder chainHolder = new ChainHolder())
using (X509Certificate2 cert = new X509Certificate2(TestData.SelfSignedCertSha256RsaBytes))
{
X509Chain chain = chainHolder.Chain;
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
chain.ChainPolicy.VerificationTime = cert.NotBefore.AddHours(2);

// No custom root of trust store means that this self-signed cert will at
// minimum be marked UntrustedRoot.

Assert.False(chain.Build(cert));
AssertExtensions.HasFlag(X509ChainStatusFlags.UntrustedRoot, chain.AllStatusFlags());
}
}

[Fact]
public static void BuildChainForSelfSignedCertificate_WithUnknownOidSignature()
{
using (ChainHolder chainHolder = new ChainHolder())
using (X509Certificate2 cert = new X509Certificate2(TestData.SelfSignedCertDummyOidBytes))
{
X509Chain chain = chainHolder.Chain;
chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
chain.ChainPolicy.VerificationTime = cert.NotBefore.AddHours(2);

// This tests a self-signed cert whose signature block contains a garbage signing alg OID.
// Some platforms return NotSignatureValid to indicate that they cannot understand the
// signature block. Other platforms return PartialChain to indicate that they think the
// bad signature block might correspond to some unknown, untrusted signer. Yet other
// platforms simply fail the operation; e.g., Windows's CertGetCertificateChain API returns
// NTE_BAD_ALGID, which we bubble up as CryptographicException.

if (PlatformDetection.UsesAppleCrypto)
{
Assert.False(chain.Build(cert));
AssertExtensions.HasFlag(X509ChainStatusFlags.PartialChain, chain.AllStatusFlags());
}
else if (PlatformDetection.IsOpenSslSupported)
{
Assert.False(chain.Build(cert));
AssertExtensions.HasFlag(X509ChainStatusFlags.NotSignatureValid, chain.AllStatusFlags());
}
else
{
Assert.ThrowsAny<CryptographicException>(() => chain.Build(cert));
}
}
}

internal static X509ChainStatusFlags AllStatusFlags(this X509Chain chain)
{
return chain.ChainStatus.Aggregate(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4224,5 +4224,54 @@ internal static DSAParameters GetDSA1024Params()
"09463C6E50BCA36EB3F8BCB00D8A415D2D0DB5AE08303B301F300706052B0E03" +
"021A0414A57105D833610A6D07EBFBE51E5486CD3F8BCE0D0414DB32290CC077" +
"37E9D9446E37F104FA876C861C0102022710").HexToByteArray();

// Used for chain building tests (CN=Test chain building)
internal static readonly byte[] SelfSignedCertSha256RsaBytes = (
"308202BD308201A5A003020102020900EF79C10DFD657168300D06092A864886F70D0101" +
"0B0500301E311C301A060355040313135465737420636861696E206275696C64696E6730" +
"1E170D3231313031333231353735335A170D3232313031333231353735335A301E311C30" +
"1A060355040313135465737420636861696E206275696C64696E6730820122300D06092A" +
"864886F70D01010105000382010F003082010A0282010100E3B5BBF862313DEAA9172788" +
"278B26A3EAB61B9B0326F5CEA91B1A6C6DFD156836A2363BFAC5B0F4A78F4CFF5A11F35A" +
"831C6D7935D1DFD13DD81DA29AA0645CBA9F4D20BF991C625E6D61CF396C15914DEE41F6" +
"1190E97B52BFF7AE52B79FD0E2EEE3319EC23C30D27A52A2E8A963557B12BEC0664ADEF9" +
"3C520B587EC5DABFBC70980DB7473414B4B6BF982EA9AA0969F2A76AA085464AE78DFB2B" +
"F04BDE7192874679193119C2AABEC04D360F61925921660BF09A0489B7C53464F5FC35B8" +
"612F5B993D544475C20AC46CD350A34551FEA0ACBD138D8B72F79052BF0EB3BD794A426C" +
"0117CB77B4F311FFD1C628F8E438E5474509AD51FA035558771546310203010001300D06" +
"092A864886F70D01010B050003820101000A12CE2FC3DC854113D179725E9D9ADD013A42" +
"D66340CEA7A465D54EC357AD8FED1828862D8B5C32EB3D21FC8B26A7CFA9D9FB36F593CC" +
"6AD30C25C96E8100C3F07B1B51430245EE995864749C53B409260B4040705654710C236F" +
"D9B7DE3F3BE5E6E5047712C5E506419106A57C5290BB206A97F6A3FCC4B4C83E25C3FC6D" +
"2BAB03B941374086265EE08A90A8C72A63A4053044B9FA3ABD5ED5785CFDDB15A6A327BD" +
"C0CC2B115B9D33BD6E528E35670E5A6A8D9CF52199F8D693315C60D9ADAD54EF7FDCED36" +
"0C8C79E84D42AB5CB6355A70951B1ABF1F2B3FB8BEB7E3A8D6BA2293C0DB8C86B0BB060F" +
"0D6DB9939E88B998662A27F092634BBF21F58EEAAA").HexToByteArray();

// This is nearly identical to the cert in Pkcs7SelfSignedCertSha256RsaBytes,
// but we've replaced the OID (1.2.840.113549.1.1.11 sha256RSA) with a dummy OID
// 1.3.9999.1234.5678.1234. The cert should load properly into an X509Certificate2
// object but will cause chain building to fail.
internal static readonly byte[] SelfSignedCertDummyOidBytes = (
"308202BD308201A5A003020102020900EF79C10DFD657168300D06092A864886F70D0101" +
"0B0500301E311C301A060355040313135465737420636861696E206275696C64696E6730" +
"1E170D3231313031333231353735335A170D3232313031333231353735335A301E311C30" +
"1A060355040313135465737420636861696E206275696C64696E6730820122300D06092A" +
"864886F70D01010105000382010F003082010A0282010100E3B5BBF862313DEAA9172788" +
"278B26A3EAB61B9B0326F5CEA91B1A6C6DFD156836A2363BFAC5B0F4A78F4CFF5A11F35A" +
"831C6D7935D1DFD13DD81DA29AA0645CBA9F4D20BF991C625E6D61CF396C15914DEE41F6" +
"1190E97B52BFF7AE52B79FD0E2EEE3319EC23C30D27A52A2E8A963557B12BEC0664ADEF9" +
"3C520B587EC5DABFBC70980DB7473414B4B6BF982EA9AA0969F2A76AA085464AE78DFB2B" +
"F04BDE7192874679193119C2AABEC04D360F61925921660BF09A0489B7C53464F5FC35B8" +
"612F5B993D544475C20AC46CD350A34551FEA0ACBD138D8B72F79052BF0EB3BD794A426C" +
"0117CB77B4F311FFD1C628F8E438E5474509AD51FA035558771546310203010001300D06" +
"092BCE0F8952AC2E8952050003820101000A12CE2FC3DC854113D179725E9D9ADD013A42" +
"D66340CEA7A465D54EC357AD8FED1828862D8B5C32EB3D21FC8B26A7CFA9D9FB36F593CC" +
"6AD30C25C96E8100C3F07B1B51430245EE995864749C53B409260B4040705654710C236F" +
"D9B7DE3F3BE5E6E5047712C5E506419106A57C5290BB206A97F6A3FCC4B4C83E25C3FC6D" +
"2BAB03B941374086265EE08A90A8C72A63A4053044B9FA3ABD5ED5785CFDDB15A6A327BD" +
"C0CC2B115B9D33BD6E528E35670E5A6A8D9CF52199F8D693315C60D9ADAD54EF7FDCED36" +
"0C8C79E84D42AB5CB6355A70951B1ABF1F2B3FB8BEB7E3A8D6BA2293C0DB8C86B0BB060F" +
"0D6DB9939E88B998662A27F092634BBF21F58EEAAA").HexToByteArray();
}
}

0 comments on commit 72e5ae9

Please sign in to comment.