Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix assert in MapVerifyErrorToChainStatus from DigiCert CRLs #39430

Merged
bartonjs merged 2 commits into from
Jul 13, 2019
Merged

Fix assert in MapVerifyErrorToChainStatus from DigiCert CRLs #39430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
923ee08
Fix assert in MapVerifyErrorToChainStatus from DigiCert CRLs
bartonjs Jul 12, 2019
4f3b75e
Make the CRL cache entry also consider the specific CDP URL
bartonjs Jul 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.X509.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,38 @@ internal enum X509VerifyStatusCode : int
X509_V_ERR_INVALID_EXTENSION = 41,
X509_V_ERR_INVALID_POLICY_EXTENSION = 42,
X509_V_ERR_NO_EXPLICIT_POLICY = 43,
X509_V_ERR_DIFFERENT_CRL_SCOPE = 44,
X509_V_ERR_UNSUPPORTED_EXTENSION_FEATURE = 45,
X509_V_ERR_UNNESTED_RESOURCE = 46,
X509_V_ERR_PERMITTED_VIOLATION = 47,
X509_V_ERR_EXCLUDED_VIOLATION = 48,
X509_V_ERR_SUBTREE_MINMAX = 49,
X509_V_ERR_APPLICATION_VERIFICATION = 50,
X509_V_ERR_UNSUPPORTED_CONSTRAINT_TYPE = 51,
X509_V_ERR_UNSUPPORTED_CONSTRAINT_SYNTAX = 52,
X509_V_ERR_UNSUPPORTED_NAME_SYNTAX = 53,
X509_V_ERR_CRL_PATH_VALIDATION_ERROR = 54,
X509_V_ERR_PATH_LOOP = 55,
X509_V_ERR_SUITE_B_INVALID_VERSION = 56,
X509_V_ERR_SUITE_B_INVALID_ALGORITHM = 57,
X509_V_ERR_SUITE_B_INVALID_CURVE = 58,
X509_V_ERR_SUITE_B_INVALID_SIGNATURE_ALGORITHM = 59,
X509_V_ERR_SUITE_B_LOS_NOT_ALLOWED = 60,
X509_V_ERR_SUITE_B_CANNOT_SIGN_P_384_WITH_P_256 = 61,
X509_V_ERR_HOSTNAME_MISMATCH = 62,
X509_V_ERR_EMAIL_MISMATCH = 63,
X509_V_ERR_IP_ADDRESS_MISMATCH = 64,
X509_V_ERR_DANE_NO_MATCH = 65,
X509_V_ERR_EE_KEY_TOO_SMALL = 66,
X509_V_ERR_CA_KEY_TOO_SMALL = 67,
X509_V_ERR_CA_MD_TOO_WEAK = 68,
X509_V_ERR_INVALID_CALL = 69,
X509_V_ERR_STORE_LOOKUP = 70,
X509_V_ERR_NO_VALID_SCTS = 71,
X509_V_ERR_PROXY_SUBJECT_NAME_VIOLATION = 72,
X509_V_ERR_OCSP_VERIFY_NEEDED = 73,
X509_V_ERR_OCSP_VERIFY_FAILED = 74,
X509_V_ERR_OCSP_CERT_UNKNOWN = 75,
}
}
}
63 changes: 47 additions & 16 deletions ...tem.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/CrlCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Buffers;
using System.Diagnostics;
using System.IO;
using System.Runtime.InteropServices;
using System.Security.Cryptography;
using System.Security.Cryptography.Asn1;
using System.Security.Cryptography.X509Certificates;
Expand All @@ -28,6 +28,9 @@ internal static class CrlCache

private const ulong X509_R_CERT_ALREADY_IN_HASH_TABLE = 0x0B07D065;

[ThreadStatic]
private static HashAlgorithm ts_urlHash;

public static void AddCrlForCertificate(
SafeX509Handle cert,
SafeX509StoreHandle store,
Expand All @@ -42,7 +45,16 @@ public static void AddCrlForCertificate(
verificationTime = DateTime.MinValue;
}

if (AddCachedCrl(cert, store, verificationTime))
string url = GetCdpUrl(cert);

if (url == null)
{
return;
}

string crlFileName = GetCrlFileName(cert, url);

if (AddCachedCrl(crlFileName, store, verificationTime))
{
return;
}
Expand All @@ -54,12 +66,12 @@ public static void AddCrlForCertificate(
return;
}

DownloadAndAddCrl(cert, store, ref remainingDownloadTime);
DownloadAndAddCrl(url, crlFileName, store, ref remainingDownloadTime);
}

private static bool AddCachedCrl(SafeX509Handle cert, SafeX509StoreHandle store, DateTime verificationTime)
private static bool AddCachedCrl(string crlFileName, SafeX509StoreHandle store, DateTime verificationTime)
{
string crlFile = GetCachedCrlPath(cert);
string crlFile = GetCachedCrlPath(crlFileName);

using (SafeBioHandle bio = Interop.Crypto.BioNewFile(crlFile, "rb"))
{
Expand Down Expand Up @@ -119,17 +131,11 @@ private static bool AddCachedCrl(SafeX509Handle cert, SafeX509StoreHandle store,
}

private static void DownloadAndAddCrl(
SafeX509Handle cert,
string url,
string crlFileName,
SafeX509StoreHandle store,
ref TimeSpan remainingDownloadTime)
{
string url = GetCdpUrl(cert);

if (url == null)
{
return;
}

// X509_STORE_add_crl will increase the refcount on the CRL object, so we should still
// dispose our copy.
using (SafeX509CrlHandle crl = CertificateAssetDownloader.DownloadCrl(url, ref remainingDownloadTime))
Expand All @@ -155,7 +161,7 @@ private static void DownloadAndAddCrl(
// the chain as invalid.
try
{
string crlFile = GetCachedCrlPath(cert, mkDir: true);
string crlFile = GetCachedCrlPath(crlFileName, mkDir: true);

using (SafeBioHandle bio = Interop.Crypto.BioNewFile(crlFile, "wb"))
{
Expand All @@ -177,7 +183,7 @@ internal static string GetCachedOcspResponseDirectory()
return s_ocspDir;
}

private static string GetCachedCrlPath(SafeX509Handle cert, bool mkDir=false)
private static string GetCrlFileName(SafeX509Handle cert, string crlUrl)
{
// X509_issuer_name_hash returns "unsigned long", which is marshalled as ulong.
// But it only sets 32 bits worth of data, so force it down to uint just... in case.
Expand All @@ -189,10 +195,35 @@ private static string GetCachedCrlPath(SafeX509Handle cert, bool mkDir=false)

uint persistentHash = unchecked((uint)persistentHashLong);

if (ts_urlHash == null)
{
ts_urlHash = SHA256.Create();
}

Span<byte> hash = stackalloc byte[256 >> 3];

// Endianness isn't important, it just needs to be consistent.
// (Even if the same storage was used for two different endianness systems it'd stabilize at two files).
ReadOnlySpan<byte> utf16Url = MemoryMarshal.AsBytes(crlUrl.AsSpan());

if (!ts_urlHash.TryComputeHash(utf16Url, hash, out int written) || written != hash.Length)
{
Debug.Fail("TryComputeHash failed or produced an incorrect length output");
throw new CryptographicException();
}

uint urlHash = MemoryMarshal.Read<uint>(hash);

// OpenSSL's hashed filename algorithm is the 8-character hex version of the 32-bit value
// of X509_issuer_name_hash (or X509_subject_name_hash, depending on the context).
string localFileName = persistentHash.ToString("x8") + ".crl";
//
// We mix in an 8-character hex version of the "left"-most bytes of a hash of the URL to
// disambiguate when one Issuing Authority separates their revocation across independent CRLs.
return $"{persistentHash:x8}.{urlHash:x8}.crl";
}

private static string GetCachedCrlPath(string localFileName, bool mkDir=false)
{
if (mkDir)
{
Directory.CreateDirectory(s_crlDir);
Expand Down
11 changes: 7 additions & 4 deletions ...tography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,10 +975,13 @@ internal int VerifyCallback(int ok, IntPtr ctx)
return 0;
}

// We don't report "OK" as an error.
// For compatibility with Windows / .NET Framework, do not report X509_V_CRL_NOT_YET_VALID.
// * We don't report "OK" as an error.
// * For compatibility with Windows / .NET Framework, do not report X509_V_CRL_NOT_YET_VALID.
// * X509_V_ERR_DIFFERENT_CRL_SCOPE will result in X509_V_ERR_UNABLE_TO_GET_CRL
// which will trigger OCSP, so is ignorable.
if (errorCode != Interop.Crypto.X509VerifyStatusCode.X509_V_OK &&
errorCode != Interop.Crypto.X509VerifyStatusCode.X509_V_ERR_CRL_NOT_YET_VALID)
errorCode != Interop.Crypto.X509VerifyStatusCode.X509_V_ERR_CRL_NOT_YET_VALID &&

@krwq krwq Jul 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

X509_V_ERR_PERMITTED_VIOLATION also seems like something which could be ignored

@bartonjs bartonjs Jul 12, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's "A name constraint violation occurred in the permitted subtrees." which might mean "this certificate has name constraints that exceed the bounds of the parent"... which probably isn't something we'd want to ignore.

I don't really want to ignore things until we can see it, and understand that we're mapping it the same as on Windows 😄.

errorCode != Interop.Crypto.X509VerifyStatusCode.X509_V_ERR_DIFFERENT_CRL_SCOPE)
{
if (_errors == null)
{
Expand Down Expand Up @@ -1015,7 +1018,7 @@ internal int VerifyCallback(int ok, IntPtr ctx)

private unsafe struct ErrorCollection
{
// As of OpenSSL 1.1.1 there are 74 defined X509_V_ERR values,
// As of OpenSSL 1.1.1 there are 75 defined X509_V_ERR values,
// therefore it fits in a bitvector backed by 3 ints (96 bits available).
private const int BucketCount = 3;
private const int OverflowValue = BucketCount * sizeof(int) * 8 - 1;
Expand Down