Correctly handle a cached CRL with no NextUpdate on Linux
authorKevin Jones <kevin@vcsjones.com>
Wed, 12 Aug 2020 22:33:24 +0000 (18:33 -0400)
committerGitHub <noreply@github.com>
Wed, 12 Aug 2020 22:33:24 +0000 (15:33 -0700)
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.

src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs
src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/CrlCache.cs
src/libraries/System.Security.Cryptography.X509Certificates/tests/RevocationTests/DynamicRevocationTests.cs

index 84a2607..a2227dd 100644 (file)
@@ -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)
index 83d2e75..16e2dc5 100644 (file)
@@ -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).
index 514698a..0455619 100644 (file)
@@ -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,