Only reuse a cached OCSP response that states a nextUpdate, as RFC 6960 sec. 4.2.2.1 has an absent one mean newer information is always available, and treat a response dated ahead of the validation time by more than the clock skew allowance as unreliable.

Author: dghgit

docs/releasenotes.html

@@ -114,6 +114,7 @@ <h3>2.1.2 Defects Fixed</h3>
 <li>X509RevocationChecker downloaded CRLs from a certificate's CRL Distribution Points extension whenever the CRLs it had been given could not answer for a certificate, with no way for a caller to prevent it - unlike the provider's CertPath validator, which has always required the org.bouncycastle.x509.enableCRLDP property to be set, and unlike what that property's own javadoc describes ("the BC CertPath validator and X509RevocationChecker will attempt to download CRLs ..."). The checker now honours the property, so the URI taken from a certificate is only dereferenced when a caller has asked for that. <b>This is a behavioural change for anyone relying on the previous automatic fetch</b>: with the property unset the checker now behaves exactly as an unproductive fetch always did, soft failing where setSoftFail was configured and reporting no CRL found where it was not, so such callers should set org.bouncycastle.x509.enableCRLDP to "true" to keep the old behaviour.</li>
 <li>OcspCache.getOcspResponse read an OCSP response up to the length the responder itself declared in its Content-Length header, applying its own 32K ceiling only when that header was absent, so a responder (or anything able to answer in its place) that declared and sent hundreds of megabytes was read into the caller's heap. The declared length may now only narrow the read, never widen it: it is used when present and no larger than the ceiling, which is otherwise applied. The ceiling is now configurable through the new org.bouncycastle.ocsp.max_response_size property and its default has been raised from 32K to 64K, still far above any real response; a configured value of zero or less is ignored so a mistyped value cannot turn the limit off. An over-long response fails the OCSP check the same way an unreachable responder does - a caller with CRLs configured falls back to those - and now says so, as Streams.readAllLimited reports the condition only as "Data Overflow", which is restated in terms of the limit and the property that sets it.</li>
 <li>ArmoredInputStream parses the OpenPGP ASCII armor headers as the stream is constructed, and bounded neither the length of a header line nor the number of them, so merely wrapping an untrusted stream could exhaust the heap before the caller had read a byte. Two shapes reached it: a "header line" that never arrives at a line terminator, accumulated without limit, and short header lines that never stop arriving, added to the header list without limit - the second is the companion case, reachable even where every line is well formed. Both are now capped, at 4096 bytes per header line and 64 headers (counting the armor header line itself), each configurable through the new org.bouncycastle.openpgp.max_armor_header_length and org.bouncycastle.openpgp.max_armor_headers properties; a value of zero or less is ignored, so a mistyped value cannot turn a limit off. Exceeding either raises ArmoredInputException, as the other malformed-header cases in that parser do. RFC 9580 sec. 6.2 bounds neither, but the headers it describes - Version, Comment and the like - are short, so the defaults sit far above any real armor. The equivalent accumulation in org.bouncycastle.util.io.pem.PemReader is deliberately left unbounded: a PEM body can legitimately be enormous - a CRL in the gigabyte range - so a default cap there would break working deployments.</li>
+<li>An OCSP response carrying no nextUpdate could be cached and reused as though it stated a validity interval, so a response could go on answering for a certificate after the responder had newer information about it - after a revocation, in particular. Any such reuse was bounded only by garbage collection rather than by an interval: OcspCache holds each responder's response map through a WeakReference, itself in a WeakHashMap, and nothing else refers to that map once the call returns, so entries last until the next collection. That is unpredictable rather than long - short on a busy JVM, potentially much longer on a large heap that collects rarely - and it is not a window the responder or the caller had any say in. RFC 6960 sec. 4.2.2.1 says the opposite of what that assumes: "if nextUpdate is not set, the responder is indicating that newer revocation information is available all the time", which is a statement that there is no interval to reuse the response over, not that it never expires. OcspCache now separates the two questions it had been asking with one method: a response arriving from the responder is accepted as before, whether or not it states a nextUpdate, while only a response that states one may be served from the cache afterwards. Nothing is rejected that was previously accepted - a responder that omits nextUpdate simply costs another request per validation, which is what "available all the time" asks for. Additionally, both the cached and the caller-supplied (stapled) paths now apply RFC 6960 sec. 4.2.2.1's other freshness rule, "responses whose thisUpdate time is later than the local system time SHOULD be considered unreliable", which neither had checked: a response dated ahead of the time being validated for by more than a 15 minute clock-skew allowance is treated as unreliable, raising "OCSP response not yet valid" on the stapled path.</li>
 </ul>
 
 <h3>2.1.3 Additional Features and Functionality</h3>

prov/src/main/java/org/bouncycastle/jce/provider/OcspCache.java

@@ -50,6 +50,12 @@ class OcspCache
     private static final int DEFAULT_TIMEOUT = 15000;
     private static final int DEFAULT_MAX_RESPONSE_SIZE = 64 * 1024;
 
+    /**
+     * Tolerance between our clock and the responder's when judging whether a response is dated in
+     * the future. 15 minutes, matching what the JDK's own OCSP client allows.
+     */
+    static final long MAX_CLOCK_SKEW_MS = 15 * 60 * 1000L;
+
     private static Map<URI, WeakReference<Map<CertID, OCSPResponse>>> cache
         = Collections.synchronizedMap(new WeakHashMap<URI, WeakReference<Map<CertID, OCSPResponse>>>());
 
@@ -75,7 +81,7 @@ static synchronized OCSPResponse getOcspResponse(
                 BasicOCSPResponse basicResp = BasicOCSPResponse.getInstance(
                     ASN1OctetString.getInstance(response.getResponseBytes().getResponse()).getOctets());
 
-                boolean matchFound = isCertIDFoundAndCurrent(basicResp, parameters.getValidDate(), certID);
+                boolean matchFound = isCertIDFoundAndReusable(basicResp, parameters.getValidDate(), certID);
                 if (matchFound)
                 {
                     return response;
@@ -262,7 +268,32 @@ static int getResponseSizeLimit(int contentLength)
         return contentLength;
     }
 
-    private static boolean isCertIDFoundAndCurrent(BasicOCSPResponse basicResp, Date validDate, CertID certID)
+    /**
+     * Whether the response answers for certID and is usable at validDate. Applied to a response as
+     * it arrives from the responder, so a missing nextUpdate is no objection - the responder is
+     * entitled not to state one.
+     */
+    static boolean isCertIDFoundAndCurrent(BasicOCSPResponse basicResp, Date validDate, CertID certID)
+    {
+        return findResponse(basicResp, validDate, certID, false);
+    }
+
+    /**
+     * Whether a response already held in the cache may answer for certID at validDate - the same
+     * question, plus the one the cache adds: does it state a validity interval to reuse it over?
+     * <p/>
+     * RFC 6960 sec. 4.2.2.1 reads "if nextUpdate is not set, the responder is indicating that newer
+     * revocation information is available all the time", so a response without one is never
+     * reusable. Nothing is rejected by this: such a response is still used for the check it arrived
+     * for, it just costs another request next time rather than being served from here.
+     */
+    static boolean isCertIDFoundAndReusable(BasicOCSPResponse basicResp, Date validDate, CertID certID)
+    {
+        return findResponse(basicResp, validDate, certID, true);
+    }
+
+    private static boolean findResponse(BasicOCSPResponse basicResp, Date validDate, CertID certID,
+        boolean requireNextUpdate)
     {
         ResponseData responseData = ResponseData.getInstance(basicResp.getTbsResponseData());
         ASN1Sequence s = responseData.getResponses();
@@ -274,12 +305,24 @@ private static boolean isCertIDFoundAndCurrent(BasicOCSPResponse basicResp, Date
             if (certID.equals(resp.getCertID()))
             {
                 ASN1GeneralizedTime nextUp = resp.getNextUpdate();
+                if (nextUp == null && requireNextUpdate)
+                {
+                    return false;
+                }
+
                 try
                 {
                     if (nextUp != null && validDate.after(nextUp.getDate()))
                     {
                         return false;
                     }
+
+                    // "Responses whose thisUpdate time is later than the local system time SHOULD
+                    // be considered unreliable" - RFC 6960 sec. 4.2.2.1, allowing for clock skew
+                    if (isFromTheFuture(resp.getThisUpdate(), validDate))
+                    {
+                        return false;
+                    }
                 }
                 catch (ParseException e)
                 {
@@ -293,4 +336,14 @@ private static boolean isCertIDFoundAndCurrent(BasicOCSPResponse basicResp, Date
 
         return false;
     }
+
+    /**
+     * Whether thisUpdate is later than the time being validated for, by more than the clock skew
+     * allowed between us and the responder.
+     */
+    static boolean isFromTheFuture(ASN1GeneralizedTime thisUpdate, Date validDate)
+        throws ParseException
+    {
+        return thisUpdate != null && thisUpdate.getDate().getTime() > (validDate.getTime() + MAX_CLOCK_SKEW_MS);
+    }
 }

prov/src/main/java/org/bouncycastle/jce/provider/ProvOcspRevocationChecker.java

@@ -279,6 +279,13 @@ public void check(Certificate certificate)
                                     {
                                         throw new CertPathValidatorException("OCSP response expired");
                                     }
+                                    // "Responses whose thisUpdate time is later than the local
+                                    // system time SHOULD be considered unreliable" - RFC 6960
+                                    // sec. 4.2.2.1, allowing for clock skew
+                                    if (OcspCache.isFromTheFuture(resp.getThisUpdate(), parameters.getValidDate()))
+                                    {
+                                        throw new CertPathValidatorException("OCSP response not yet valid");
+                                    }
                                     if (certID == null || !isEqualAlgId(certID.getHashAlgorithm(), resp.getCertID().getHashAlgorithm()))
                                     {
                                         org.bouncycastle.asn1.x509.Certificate issuer = extractCert();

prov/src/test/java/org/bouncycastle/jce/provider/OcspCacheTest.java

@@ -1,8 +1,26 @@
 package org.bouncycastle.jce.provider;
 
 import java.io.ByteArrayInputStream;
+import java.math.BigInteger;
+import java.util.Date;
 
 import junit.framework.TestCase;
+import org.bouncycastle.asn1.ASN1GeneralizedTime;
+import org.bouncycastle.asn1.ASN1Integer;
+import org.bouncycastle.asn1.ASN1Sequence;
+import org.bouncycastle.asn1.DERBitString;
+import org.bouncycastle.asn1.DEROctetString;
+import org.bouncycastle.asn1.DERSequence;
+import org.bouncycastle.asn1.nist.NISTObjectIdentifiers;
+import org.bouncycastle.asn1.ocsp.BasicOCSPResponse;
+import org.bouncycastle.asn1.ocsp.CertID;
+import org.bouncycastle.asn1.ocsp.CertStatus;
+import org.bouncycastle.asn1.ocsp.ResponderID;
+import org.bouncycastle.asn1.ocsp.ResponseData;
+import org.bouncycastle.asn1.ocsp.SingleResponse;
+import org.bouncycastle.asn1.x500.X500Name;
+import org.bouncycastle.asn1.x509.AlgorithmIdentifier;
+import org.bouncycastle.asn1.x509.Extensions;
 import org.bouncycastle.util.Arrays;
 import org.bouncycastle.util.Properties;
 import org.bouncycastle.util.io.StreamOverflowException;
@@ -90,4 +108,87 @@ public void testOverLongResponseNamesTheLimit()
 
         assertTrue(Arrays.areEqual(response, OcspCache.readResponse(new ByteArrayInputStream(response), 1024)));
     }
+    /**
+     * A cached response is only reusable while it states a validity interval covering the time
+     * being validated for. RFC 6960 sec. 4.2.2.1: "if nextUpdate is not set, the responder is
+     * indicating that newer revocation information is available all the time" - so there is no
+     * interval to reuse it over, and the cache must go back to the responder.
+     */
+    public void testResponseWithoutNextUpdateIsNeverCurrent()
+        throws Exception
+    {
+        Date now = new Date();
+        CertID certID = certID();
+
+        BasicOCSPResponse withNextUpdate = response(certID, minutesFromNow(now, -5), minutesFromNow(now, 60));
+        assertTrue("response inside its own validity interval was not current",
+            OcspCache.isCertIDFoundAndCurrent(withNextUpdate, now, certID));
+
+        BasicOCSPResponse expired = response(certID, minutesFromNow(now, -120), minutesFromNow(now, -60));
+        assertFalse("expired response was current", OcspCache.isCertIDFoundAndCurrent(expired, now, certID));
+
+        BasicOCSPResponse noNextUpdate = response(certID, minutesFromNow(now, -5), null);
+        assertFalse("response with no nextUpdate was reused from the cache",
+            OcspCache.isCertIDFoundAndReusable(noNextUpdate, now, certID));
+
+        // however old it is
+        BasicOCSPResponse ancient = response(certID, minutesFromNow(now, -60 * 24 * 365), null);
+        assertFalse("year-old response with no nextUpdate was reused from the cache",
+            OcspCache.isCertIDFoundAndReusable(ancient, now, certID));
+
+        // but nothing is rejected by that: a responder is entitled not to state a nextUpdate, and
+        // the response it just gave us is used for the check it arrived for
+        assertTrue("freshly fetched response with no nextUpdate was refused",
+            OcspCache.isCertIDFoundAndCurrent(noNextUpdate, now, certID));
+
+        // the interval is still honoured where one is stated
+        assertTrue("response inside its validity interval was not reusable",
+            OcspCache.isCertIDFoundAndReusable(withNextUpdate, now, certID));
+        assertFalse("expired response was reusable", OcspCache.isCertIDFoundAndReusable(expired, now, certID));
+    }
+
+    /**
+     * "Responses whose thisUpdate time is later than the local system time SHOULD be considered
+     * unreliable" - RFC 6960 sec. 4.2.2.1. Clock skew between us and the responder is allowed for.
+     */
+    public void testResponseDatedInTheFuture()
+        throws Exception
+    {
+        Date now = new Date();
+        CertID certID = certID();
+
+        BasicOCSPResponse withinSkew = response(certID, minutesFromNow(now, 5), minutesFromNow(now, 60));
+        assertTrue("response inside the clock skew allowance was rejected",
+            OcspCache.isCertIDFoundAndCurrent(withinSkew, now, certID));
+
+        BasicOCSPResponse fromTheFuture = response(certID, minutesFromNow(now, 60), minutesFromNow(now, 120));
+        assertFalse("response dated an hour ahead was current",
+            OcspCache.isCertIDFoundAndCurrent(fromTheFuture, now, certID));
+
+        assertFalse("absent thisUpdate treated as future", OcspCache.isFromTheFuture(null, now));
+    }
+
+    private static ASN1GeneralizedTime minutesFromNow(Date now, int minutes)
+    {
+        return new ASN1GeneralizedTime(new Date(now.getTime() + (minutes * 60 * 1000L)));
+    }
+
+    private static CertID certID()
+    {
+        return new CertID(new AlgorithmIdentifier(NISTObjectIdentifiers.id_sha256),
+            new DEROctetString(new byte[32]), new DEROctetString(new byte[32]), new ASN1Integer(BigInteger.ONE));
+    }
+
+    private static BasicOCSPResponse response(CertID certID, ASN1GeneralizedTime thisUpdate,
+        ASN1GeneralizedTime nextUpdate)
+    {
+        SingleResponse single = new SingleResponse(certID, new CertStatus(), thisUpdate, nextUpdate,
+            (Extensions)null);
+
+        ResponseData responseData = new ResponseData(new ResponderID(new X500Name("CN=Test Responder")),
+            thisUpdate, new DERSequence(single), (Extensions)null);
+
+        return new BasicOCSPResponse(responseData, new AlgorithmIdentifier(NISTObjectIdentifiers.id_sha256),
+            new DERBitString(new byte[]{ 1 }), (ASN1Sequence)null);
+    }
 }