From 553e96e9c5e6460c5abfb1dd17a3bc27be5a7f9a Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Fri, 12 Jul 2019 18:55:27 -0700 Subject: [PATCH] Fix assert in MapVerifyErrorToChainStatus from DigiCert CRLs * Define all of the X509_V_ERR* values form OpenSSL 1.0/1.1 * Exempt X509_V_ERR_DIFFERENT_CRL_SCOPE from being reported, as it is redundant to X509_V_ERR_UNABLE_TO_GET_CRL * The remaining new X509_V_ERR* codes will still trip the assert, largely because they should be behind option flags that we don't enable. This change also makes the CRL cache entry also consider the specific CDP URL: DigiCert currently has at least two distinct CRLs running for the same CA (C = US, O = DigiCert Inc, CN = DigiCert SHA2 Secure Server CA) * URI:http://crl3.digicert.com/ssca-sha2-g5.crl * URI:http://crl3.digicert.com/ssca-sha2-g6.crl Using the current cache name strategy, just a hash of the CA name, some of the certs get meaningful revocation, and others do not. Now the file name will be {ca name hash}.{url hash}.crl, enabling the DigiCert scenario. Upgrading across this change will cause CRLs to be re-downloaded, but that's a thing that happens over time anyways. Commit migrated from https://github.com/dotnet/corefx/commit/95bbbcd9f85aecd29df0e47b2aa3b65f0fac71a2 --- .../Interop.X509.cs | 32 +++++++++++ .../src/Internal/Cryptography/Pal.Unix/CrlCache.cs | 63 ++++++++++++++++------ .../Pal.Unix/OpenSslX509ChainProcessor.cs | 11 ++-- 3 files changed, 86 insertions(+), 20 deletions(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.X509.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.X509.cs index 38615e0..8ffc70a 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.X509.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.X509.cs @@ -278,6 +278,38 @@ internal static partial class Interop 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, } } } diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/CrlCache.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/CrlCache.cs index e1c8262..26299bd 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/CrlCache.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/CrlCache.cs @@ -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; @@ -28,6 +28,9 @@ namespace Internal.Cryptography.Pal 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, @@ -42,7 +45,16 @@ namespace Internal.Cryptography.Pal 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; } @@ -54,12 +66,12 @@ namespace Internal.Cryptography.Pal 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")) { @@ -119,17 +131,11 @@ namespace Internal.Cryptography.Pal } 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)) @@ -155,7 +161,7 @@ namespace Internal.Cryptography.Pal // 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")) { @@ -177,7 +183,7 @@ namespace Internal.Cryptography.Pal 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. @@ -189,10 +195,35 @@ namespace Internal.Cryptography.Pal uint persistentHash = unchecked((uint)persistentHashLong); + if (ts_urlHash == null) + { + ts_urlHash = SHA256.Create(); + } + + Span 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 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(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); diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs index 314cc3e..d28286f 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs @@ -975,10 +975,13 @@ namespace Internal.Cryptography.Pal 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 && + errorCode != Interop.Crypto.X509VerifyStatusCode.X509_V_ERR_DIFFERENT_CRL_SCOPE) { if (_errors == null) { @@ -1015,7 +1018,7 @@ namespace Internal.Cryptography.Pal 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; -- 2.7.4