489694 - OCSP nextFetchAttemptTime incorrectly (?) computed in some cases

Status: NEW

(NSS :: Libraries, defect, P5)

Description

[Nagendra Modadugu]

From security/nss/lib/certhigh/ocsp.c:

716     if (cacheItem->haveThisUpdate) {
717         OCSP_TRACE_TIME("thisUpdate:", cacheItem->thisUpdate);
718         latestTimeWhenResponseIsConsideredFresh = cacheItem->thisUpdate +
719             OCSP_Global.maximumSecondsToNextFetchAttempt * 
720                 MICROSECONDS_PER_SECOND;
721         OCSP_TRACE_TIME("latestTimeWhenResponseIsConsideredFresh:", 
722                         latestTimeWhenResponseIsConsideredFresh);
723     } else {
724         latestTimeWhenResponseIsConsideredFresh = now +
725             OCSP_Global.minimumSecondsToNextFetchAttempt *
726                 MICROSECONDS_PER_SECOND;
727         OCSP_TRACE_TIME("no thisUpdate, "
728                         "latestTimeWhenResponseIsConsideredFresh:", 
729                         latestTimeWhenResponseIsConsideredFresh);
730     }

...
750     if (latestTimeWhenResponseIsConsideredFresh < 
751         earliestAllowedNextFetchAttemptTime) {
752         latestTimeWhenResponseIsConsideredFresh = 
753             earliestAllowedNextFetchAttemptTime;
754         OCSP_TRACE_TIME("latest < earliest, setting latest to:", 
755                         latestTimeWhenResponseIsConsideredFresh);
756     }


latestTimeWhenResponseIsConsideredFresh is computed based on when the OCSP response was generated, rather then when the lookup is performed.  This means that 24hrs (i.e. maximumSecondsToNextFetchAttempt) later than when the response was been generated, the response will get cached for no more than (1hr) minimumSecondsToNextFetchAttempt seconds.  Is this the expected behavior?

There's a minor bug in a logging statement:
732     if (cacheItem->haveNextUpdate) {
733         OCSP_TRACE_TIME("have nextUpdate:", cacheItem->thisUpdate);

cacheItem->thisUpdate ---> cacheItem->nextUpdate

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Good to hear from you, Nagendra.

We'll have to decide on the "expected behavior" question, I think.

Comment 2

[Julien Pierre]

Attachment: Fix logging (checked in)

Comment 3

[Julien Pierre]

Regarding the question of the expected behavior, I believe the answering is yes, the freshness should be based on the thisUpdate of the response, so the behavior is correct.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 377027 [details] [diff] [review]
Fix logging (checked in)

This patch is fine, and a good thing, but I am not sure that 
the behavior Nagendra described is desired behavior.  
We should discuss it further, preferably by phone or in person.

Comment 5

[Julien Pierre]

I checked in the fix for the logging.

Checking in ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.59; previous revision: 1.58
done

Comment 6

[Kai Engert (:KaiE:)]

Bugs that are currently assigned to Julien => assigning to nobody.
Search for 20100628-kaie-jp