From f9dffea57540192174060bbeff0a9c499e1d71af Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Wed, 12 Aug 2020 18:33:24 -0400 Subject: [PATCH] Correctly handle a cached CRL with no NextUpdate on Linux Treat a CRL with no nextUpdate as cachable for 3 days, then we'll recheck it. This only seems to (legitimately) happen when the associated CA is nearing end-of-validity. Either macOS doesn't really support this, or it's just part of their general downplay of CRL, but the tests are disabled there. --- .../X509Certificates/CertificateAuthority.cs | 11 ++++- .../src/Internal/Cryptography/Pal.Unix/CrlCache.cs | 26 ++++++++++- .../RevocationTests/DynamicRevocationTests.cs | 50 ++++++++++++++++++++++ 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs index 84a2607..a2227dd 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs @@ -96,6 +96,7 @@ namespace System.Security.Cryptography.X509Certificates.Tests.Common internal bool CorruptRevocationSignature { get; set; } internal DateTimeOffset? RevocationExpiration { get; set; } internal bool CorruptRevocationIssuerName { get; set; } + internal bool OmitNextUpdateInCrl { get; set; } // All keys created in this method are smaller than recommended, // but they only live for a few seconds (at most), @@ -343,7 +344,10 @@ namespace System.Security.Cryptography.X509Certificates.Tests.Common writer.WriteUtcTime(_cert.NotBefore); // nextUpdate - writer.WriteUtcTime(RevocationExpiration.Value); + if (!OmitNextUpdateInCrl) + { + writer.WriteUtcTime(RevocationExpiration.Value); + } } else { @@ -351,7 +355,10 @@ namespace System.Security.Cryptography.X509Certificates.Tests.Common writer.WriteUtcTime(now); // nextUpdate - writer.WriteUtcTime(newExpiry); + if (!OmitNextUpdateInCrl) + { + writer.WriteUtcTime(newExpiry); + } } // revokedCertificates (don't write down if empty) 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 83d2e75..16e2dc5 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 @@ -96,8 +96,30 @@ namespace Internal.Cryptography.Pal // at least it can fail without using the network. // // If crl.NextUpdate is in the past, try downloading a newer version. - DateTime nextUpdate = OpenSslX509CertificateReader.ExtractValidityDateTime( - Interop.Crypto.GetX509CrlNextUpdate(crl)); + IntPtr nextUpdatePtr = Interop.Crypto.GetX509CrlNextUpdate(crl); + DateTime nextUpdate; + + // If there is no crl.NextUpdate, this indicates that the CA is not providing + // any more updates to the CRL, or they made a mistake not providing a NextUpdate. + // We'll cache it for a few days to cover the case it was a mistake. + if (nextUpdatePtr == IntPtr.Zero) + { + try + { + nextUpdate = File.GetLastWriteTime(crlFile).AddDays(3); + } + catch + { + // We couldn't determine when the CRL was last written to, + // so consider it expired. + Debug.Fail("Failed to get the last write time of the CRL file"); + return false; + } + } + else + { + nextUpdate = OpenSslX509CertificateReader.ExtractValidityDateTime(nextUpdatePtr); + } // OpenSSL is going to convert our input time to universal, so we should be in Local or // Unspecified (local-assumed). diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs index 514698a..0455619 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs @@ -940,6 +940,56 @@ namespace System.Security.Cryptography.X509Certificates.Tests.RevocationTests }); } + [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/31249", TestPlatforms.OSX)] + public static void TestRevocationWithNoNextUpdate_NotRevoked() + { + SimpleTest( + PkiOptions.CrlEverywhere, + (root, intermediate, endEntity, holder) => + { + intermediate.OmitNextUpdateInCrl = true; + + // Build a chain once to get the no NextUpdate in the CRL + // cache. We don't care about the build result. + holder.Chain.Build(endEntity); + + SimpleRevocationBody( + holder, + endEntity, + rootRevoked: false, + issrRevoked: false, + leafRevoked: false); + }); + } + + [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/31249", TestPlatforms.OSX)] + public static void TestRevocationWithNoNextUpdate_Revoked() + { + SimpleTest( + PkiOptions.CrlEverywhere, + (root, intermediate, endEntity, holder) => + { + intermediate.OmitNextUpdateInCrl = true; + + DateTimeOffset now = DateTimeOffset.UtcNow; + intermediate.Revoke(endEntity, now); + holder.Chain.ChainPolicy.VerificationTime = now.AddSeconds(1).UtcDateTime; + + // Build a chain once to get the no NextUpdate in the CRL + // cache. We don't care about the build result. + holder.Chain.Build(endEntity); + + SimpleRevocationBody( + holder, + endEntity, + rootRevoked: false, + issrRevoked: false, + leafRevoked: true); + }); + } + private static void RevokeEndEntityWithInvalidRevocation( ChainHolder holder, CertificateAuthority intermediate, -- 2.7.4