Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 11bd62de authored by Tom Chan's avatar Tom Chan Committed by Android (Google) Code Review
Browse files

Merge changes Ie68b7c90,I614f0ed1 into main

* changes:
  Do not fetch remote CRL if all certificates are recently checked
  Do not check revocation status of leaf certificate
parents 3709fb6a 8ba8fbd0
Loading
Loading
Loading
Loading
+5 −1
Original line number Diff line number Diff line
@@ -303,7 +303,11 @@ class AttestationVerificationPeerDeviceVerifier {
        if (mRevocationEnabled) {
            // Checks Revocation Status List based on
            // https://developer.android.com/training/articles/security-key-attestation#certificate_status
            mCertificateRevocationStatusManager.checkRevocationStatus(certificates);
            // The first certificate is the leaf, which is generated at runtime with the attestation
            // attributes such as the challenge. It is specific to this attestation instance and
            // does not need to be checked for revocation.
            mCertificateRevocationStatusManager.checkRevocationStatus(
                    new ArrayList<>(certificates.subList(1, certificates.size())));
        }
    }

+33 −12
Original line number Diff line number Diff line
@@ -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.
@@ -126,6 +129,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<>();
@@ -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);
            }
        }
    }

    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().minusDays(MAX_DAYS_SINCE_LAST_CHECK))) {
                    throw new CertPathValidatorException(
                            "Unable to verify the revocation status of certificate "
                                    + serialNumber);
                }
                            .isBefore(LocalDateTime.now().minus(lastCheckedWithin))) {
                return false;
            }
        }
        return true;
    }

    private static boolean needToCheckRevocationStatus(
+66 −0
Original line number Diff line number Diff line
@@ -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));