Fetch CRLDistributionPoints in FidoMetadataDownloader

Author: emlun

webauthn-server-attestation/build.gradle.kts

@@ -48,7 +48,12 @@ dependencies {
   testImplementation("org.scalatestplus:junit-4-13_2.13")
   testImplementation("org.scalatestplus:scalacheck-1-16_2.13")
 
-  testImplementation("org.slf4j:slf4j-api")
+  testImplementation("org.slf4j:slf4j-api") {
+    version {
+      strictly("[1.7.25,1.8-a)") // Pre-1.8 version required by slf4j-test
+    }
+  }
+  testRuntimeOnly("uk.org.lidalia:slf4j-test")
 }
 
 val integrationTest = task<Test>("integrationTest") {
@@ -58,9 +63,6 @@ val integrationTest = task<Test>("integrationTest") {
   testClassesDirs = sourceSets["integrationTest"].output.classesDirs
   classpath = sourceSets["integrationTest"].runtimeClasspath
   shouldRunAfter(tasks.test)
-
-  // Required for processing CRL distribution points extension
-  systemProperty("com.sun.security.enableCRLDP", "true")
 }
 tasks["check"].dependsOn(integrationTest)
 

webauthn-server-attestation/src/integrationTest/scala/com/yubico/fido/metadata/FidoMetadataDownloaderIntegrationTest.scala

@@ -1,5 +1,7 @@
 package com.yubico.fido.metadata
 
+import com.yubico.internal.util.CertificateParser
+import com.yubico.webauthn.data.ByteArray
 import org.junit.runner.RunWith
 import org.scalatest.BeforeAndAfter
 import org.scalatest.funspec.AnyFunSpec
@@ -8,7 +10,8 @@ import org.scalatest.tags.Network
 import org.scalatest.tags.Slow
 import org.scalatestplus.junit.JUnitRunner
 
-import java.util.Optional
+import scala.jdk.CollectionConverters.ListHasAsScala
+import scala.jdk.OptionConverters.RichOption
 import scala.util.Success
 import scala.util.Try
 
@@ -21,24 +24,56 @@ class FidoMetadataDownloaderIntegrationTest
     with BeforeAndAfter {
 
   describe("FidoMetadataDownloader with default settings") {
+    // Cache downloaded items to avoid cause unnecessary load on remote servers
+    var trustRootCache: Option[ByteArray] = None
+    var blobCache: Option[ByteArray] = None
     val downloader =
       FidoMetadataDownloader
         .builder()
         .expectLegalHeader(
           "Retrieval and use of this BLOB indicates acceptance of the appropriate agreement located at https://fidoalliance.org/metadata/metadata-legal-terms/"
         )
         .useDefaultTrustRoot()
-        .useTrustRootCache(() => Optional.empty(), _ => {})
+        .useTrustRootCache(
+          () => trustRootCache.toJava,
+          trustRoot => { trustRootCache = Some(trustRoot) },
+        )
         .useDefaultBlob()
-        .useBlobCache(() => Optional.empty(), _ => {})
+        .useBlobCache(
+          () => blobCache.toJava,
+          blob => { blobCache = Some(blob) },
+        )
         .build()
 
     it("downloads and verifies the root cert and BLOB successfully.") {
-      // This test requires the system property com.sun.security.enableCRLDP=true
       val blob = Try(downloader.loadCachedBlob)
       blob shouldBe a[Success[_]]
       blob.get should not be null
     }
+
+    it(
+      "does not encounter any CRLDistributionPoints entries in unknown format."
+    ) {
+      val blob = Try(downloader.loadCachedBlob)
+      blob shouldBe a[Success[_]]
+      val trustRootCert =
+        CertificateParser.parseDer(trustRootCache.get.getBytes)
+      val certChain = downloader
+        .fetchHeaderCertChain(
+          trustRootCert,
+          FidoMetadataDownloader.parseBlob(blobCache.get).getBlob.getHeader,
+        )
+        .asScala :+ trustRootCert
+      for { cert <- certChain } {
+        withClue(
+          s"Unknown CRLDistributionPoints structure in cert [${cert.getSubjectX500Principal}] : ${new ByteArray(cert.getEncoded)}"
+        ) {
+          CertificateParser
+            .parseCrlDistributionPointsExtension(cert)
+            .isAnyDistributionPointUnsupported should be(false)
+        }
+      }
+    }
   }
 
 }

webauthn-server-attestation/src/main/java/com/yubico/fido/metadata/FidoMetadataDownloader.java

@@ -29,6 +29,7 @@
 import com.yubico.fido.metadata.FidoMetadataDownloaderException.Reason;
 import com.yubico.internal.util.BinaryUtil;
 import com.yubico.internal.util.CertificateParser;
+import com.yubico.internal.util.OptionalUtil;
 import com.yubico.webauthn.data.ByteArray;
 import com.yubico.webauthn.data.exception.Base64UrlException;
 import com.yubico.webauthn.data.exception.HexException;
@@ -54,6 +55,7 @@
 import java.security.Signature;
 import java.security.SignatureException;
 import java.security.cert.CRL;
+import java.security.cert.CRLException;
 import java.security.cert.CertPath;
 import java.security.cert.CertPathValidator;
 import java.security.cert.CertPathValidatorException;
@@ -1131,13 +1133,18 @@ private MetadataBLOB verifyBlob(ParseResult parseResult, X509Certificate trustRo
     if (certStore != null) {
       pathParams.addCertStore(certStore);
     }
+
+    // Parse CRLDistributionPoints ourselves so users don't have to set the
+    // `com.sun.security.enableCRLDP=true` system property
+    fetchCrlDistributionPoints(certChain, certFactory).ifPresent(pathParams::addCertStore);
+
     pathParams.setDate(Date.from(clock.instant()));
     cpv.validate(blobCertPath, pathParams);
 
     return parseResult.blob;
   }
 
-  private static ParseResult parseBlob(ByteArray jwt) throws IOException, Base64UrlException {
+  static ParseResult parseBlob(ByteArray jwt) throws IOException, Base64UrlException {
     Scanner s = new Scanner(new ByteArrayInputStream(jwt.getBytes())).useDelimiter("\\.");
     final ByteArray jwtHeader = ByteArray.fromBase64Url(s.next());
     final ByteArray jwtPayload = ByteArray.fromBase64Url(s.next());
@@ -1176,7 +1183,7 @@ private static ByteArray verifyHash(ByteArray contents, Set<ByteArray> acceptedC
   }
 
   @Value
-  private static class ParseResult {
+  static class ParseResult {
     private MetadataBLOB blob;
     private ByteArray jwtHeader;
     private ByteArray jwtPayload;
@@ -1213,4 +1220,68 @@ List<X509Certificate> fetchHeaderCertChain(
       return Collections.singletonList(trustRootCertificate);
     }
   }
+
+  /**
+   * Parse the CRLDistributionPoints extension of each certificate, fetch each distribution point
+   * and assemble them into a {@link CertStore} ready to be injected into {@link
+   * PKIXParameters#addCertStore(CertStore)} to provide CRLs for the verification procedure.
+   *
+   * <p>We do this ourselves so that users don't have to set the `com.sun.security.enableCRLDP=true`
+   * system property. This is required by the default SUN provider in order to enable
+   * CRLDistributionPoints resolution.
+   *
+   * <p>Any CRLDistributionPoints entries in unknown format are ignored and log a warning.
+   */
+  private Optional<CertStore> fetchCrlDistributionPoints(
+      List<X509Certificate> certChain, CertificateFactory certFactory)
+      throws InvalidAlgorithmParameterException, NoSuchAlgorithmException {
+    final List<URL> crlDistributionPointUrls =
+        certChain.stream()
+            .flatMap(
+                cert -> {
+                  log.debug(
+                      "Attempting to parse CRLDistributionPoints extension of cert: {}",
+                      cert.getSubjectX500Principal());
+                  try {
+                    return CertificateParser.parseCrlDistributionPointsExtension(cert)
+                        .getDistributionPoints()
+                        .stream();
+                  } catch (Exception e) {
+                    log.warn(
+                        "Failed to parse CRLDistributionPoints extension of cert: {}",
+                        cert.getSubjectX500Principal(),
+                        e);
+                    return Stream.empty();
+                  }
+                })
+            .collect(Collectors.toList());
+
+    if (crlDistributionPointUrls.isEmpty()) {
+      return Optional.empty();
+
+    } else {
+      final List<CRL> crldpCrls =
+          crlDistributionPointUrls.stream()
+              .map(
+                  crldpUrl -> {
+                    log.debug("Attempting to download CRL distribution point: {}", crldpUrl);
+                    try {
+                      return Optional.of(
+                          certFactory.generateCRL(
+                              new ByteArrayInputStream(download(crldpUrl).getBytes())));
+                    } catch (CRLException e) {
+                      log.warn("Failed to import CRL from distribution point: {}", crldpUrl, e);
+                      return Optional.<CRL>empty();
+                    } catch (Exception e) {
+                      log.warn("Failed to download CRL distribution point: {}", crldpUrl, e);
+                      return Optional.<CRL>empty();
+                    }
+                  })
+              .flatMap(OptionalUtil::stream)
+              .collect(Collectors.toList());
+
+      return Optional.of(
+          CertStore.getInstance("Collection", new CollectionCertStoreParameters(crldpCrls)));
+    }
+  }
 }