Do not fetch remote CRL if all certificates are recently checked

This change limits remote fetches to at most once every 24 hours and
reduces potentially metered network use for repeated attestations
from the same device.

Test: Manually, also atest AttestationVerificationTest:com.android.server.security.CertificateRevocationStatusManagerTest
Bug: 389088384
Flag: EXEMPT bug fix
Change-Id: Ie68b7c904b5a75145e75654f173f44655dae6d31

2 files changed, 99 insertions, 12 deletions

diff --git a/services/core/java/com/android/server/security/CertificateRevocationStatusManager.java b/services/core/java/com/android/server/security/CertificateRevocationStatusManager.java
index d36d9f5f6636..4cd4b3b84910 100644
--- a/services/core/java/com/android/server/security/CertificateRevocationStatusManager.java
+++ b/services/core/java/com/android/server/security/CertificateRevocationStatusManager.java
@@ -42,6 +42,7 @@ import java.net.MalformedURLException;
 import java.net.URL;
 import java.security.cert.CertPathValidatorException;
 import java.security.cert.X509Certificate;
+import java.time.Duration;
 import java.time.LocalDate;
 import java.time.LocalDateTime;
 import java.time.ZoneId;
@@ -67,6 +68,8 @@ class CertificateRevocationStatusManager {
      */
     @VisibleForTesting static final int MAX_DAYS_SINCE_LAST_CHECK = 30;
 
+    @VisibleForTesting static final int NUM_HOURS_BEFORE_NEXT_CHECK = 24;
+
     /**
      * The number of days since issue date for an intermediary certificate to be considered fresh
      * and not require a revocation list check.
@@ -127,6 +130,17 @@ class CertificateRevocationStatusManager {
             serialNumbers.add(serialNumber);
         }
         try {
+            if (isLastCheckedWithin(Duration.ofHours(NUM_HOURS_BEFORE_NEXT_CHECK), serialNumbers)) {
+                Slog.d(
+                        TAG,
+                        "All certificates have been checked for revocation recently. No need to"
+                                + " check this time.");
+                return;
+            }
+        } catch (IOException ignored) {
+            // Proceed to check the revocation status
+        }
+        try {
             JSONObject revocationList = fetchRemoteRevocationList();
             Map<String, Boolean> areCertificatesRevoked = new HashMap<>();
             for (String serialNumber : serialNumbers) {
@@ -151,25 +165,32 @@ class CertificateRevocationStatusManager {
                     serialNumbers.remove(serialNumber);
                 }
             }
-            Map<String, LocalDateTime> lastRevocationCheckData;
             try {
-                lastRevocationCheckData = getLastRevocationCheckData();
+                if (!isLastCheckedWithin(
+                        Duration.ofDays(MAX_DAYS_SINCE_LAST_CHECK), serialNumbers)) {
+                    throw new CertPathValidatorException(
+                            "Unable to verify the revocation status of one of the certificates "
+                                    + serialNumbers);
+                }
             } catch (IOException ex2) {
                 throw new CertPathValidatorException(
                         "Unable to load stored revocation status", ex2);
             }
-            for (String serialNumber : serialNumbers) {
-                if (!lastRevocationCheckData.containsKey(serialNumber)
-                        || lastRevocationCheckData
-                                .get(serialNumber)
-                                .isBefore(
-                                        LocalDateTime.now().minusDays(MAX_DAYS_SINCE_LAST_CHECK))) {
-                    throw new CertPathValidatorException(
-                            "Unable to verify the revocation status of certificate "
-                                    + serialNumber);
-                }
+        }
+    }
+
+    private boolean isLastCheckedWithin(Duration lastCheckedWithin, List<String> serialNumbers)
+            throws IOException {
+        Map<String, LocalDateTime> lastRevocationCheckData = getLastRevocationCheckData();
+        for (String serialNumber : serialNumbers) {
+            if (!lastRevocationCheckData.containsKey(serialNumber)
+                    || lastRevocationCheckData
+                            .get(serialNumber)
+                            .isBefore(LocalDateTime.now().minus(lastCheckedWithin))) {
+                return false;
             }
         }
+        return true;
     }
 
     private static boolean needToCheckRevocationStatus(
diff --git a/tests/AttestationVerificationTest/src/com/android/server/security/CertificateRevocationStatusManagerTest.java b/tests/AttestationVerificationTest/src/com/android/server/security/CertificateRevocationStatusManagerTest.java
index c38517ace5e6..586bb76388f6 100644
--- a/tests/AttestationVerificationTest/src/com/android/server/security/CertificateRevocationStatusManagerTest.java
+++ b/tests/AttestationVerificationTest/src/com/android/server/security/CertificateRevocationStatusManagerTest.java
@@ -277,6 +277,72 @@ public class CertificateRevocationStatusManagerTest {
         }
     }
 
+    @Test
+    public void checkRevocationStatus_allCertificatesRecentlyChecked_doesNotFetchRemoteCrl()
+            throws Exception {
+        copyFromAssetToFile(
+                REVOCATION_LIST_WITHOUT_CERTIFICATES_USED_IN_THIS_TEST, mRevocationListFile);
+        mCertificateRevocationStatusManager =
+                new CertificateRevocationStatusManager(
+                        mContext, mRevocationListUrl, mRevocationStatusFile, false);
+        mCertificateRevocationStatusManager.checkRevocationStatus(mCertificates1);
+        // indirectly verifies the remote list is not fetched by simulating a remote revocation
+        copyFromAssetToFile(
+                REVOCATION_LIST_WITH_CERTIFICATES_USED_IN_THIS_TEST, mRevocationListFile);
+
+        // no exception
+        mCertificateRevocationStatusManager.checkRevocationStatus(mCertificates1);
+    }
+
+    @Test
+    public void checkRevocationStatus_allCertificatesBarelyRecentlyChecked_doesNotFetchRemoteCrl()
+            throws Exception {
+        copyFromAssetToFile(
+                REVOCATION_LIST_WITH_CERTIFICATES_USED_IN_THIS_TEST, mRevocationListFile);
+        mCertificateRevocationStatusManager =
+                new CertificateRevocationStatusManager(
+                        mContext, mRevocationListUrl, mRevocationStatusFile, false);
+        Map<String, LocalDateTime> lastCheckedDates = new HashMap<>();
+        LocalDateTime barelyRecently =
+                LocalDateTime.now()
+                        .minusHours(
+                                CertificateRevocationStatusManager.NUM_HOURS_BEFORE_NEXT_CHECK - 1);
+        for (X509Certificate certificate : mCertificates1) {
+            lastCheckedDates.put(getSerialNumber(certificate), barelyRecently);
+        }
+        mCertificateRevocationStatusManager.storeLastRevocationCheckData(lastCheckedDates);
+
+        // Indirectly verify the remote CRL is not checked by checking there is no exception despite
+        // a certificate being revoked. This test differs from the next only in the lastCheckedDate,
+        // one before the NUM_HOURS_BEFORE_NEXT_CHECK cutoff and one after
+        mCertificateRevocationStatusManager.checkRevocationStatus(mCertificates1);
+    }
+
+    @Test
+    public void checkRevocationStatus_certificatesRevokedAfterCheck_throwsException()
+            throws Exception {
+        copyFromAssetToFile(
+                REVOCATION_LIST_WITH_CERTIFICATES_USED_IN_THIS_TEST, mRevocationListFile);
+        mCertificateRevocationStatusManager =
+                new CertificateRevocationStatusManager(
+                        mContext, mRevocationListUrl, mRevocationStatusFile, false);
+        Map<String, LocalDateTime> lastCheckedDates = new HashMap<>();
+        // To save network use, we do not check the remote CRL if all the certificates are recently
+        // checked, so we set the lastCheckDate to some time not recent.
+        LocalDateTime notRecently =
+                LocalDateTime.now()
+                        .minusHours(
+                                CertificateRevocationStatusManager.NUM_HOURS_BEFORE_NEXT_CHECK + 1);
+        for (X509Certificate certificate : mCertificates1) {
+            lastCheckedDates.put(getSerialNumber(certificate), notRecently);
+        }
+        mCertificateRevocationStatusManager.storeLastRevocationCheckData(lastCheckedDates);
+
+        assertThrows(
+                CertPathValidatorException.class,
+                () -> mCertificateRevocationStatusManager.checkRevocationStatus(mCertificates1));
+    }
+
     private List<X509Certificate> getCertificateChain(String fileName) throws Exception {
         Collection<? extends Certificate> certificates =
                 mFactory.generateCertificates(mContext.getResources().getAssets().open(fileName));