158141 - NSS should have a slop time for OCSP responses in the future.

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Stephane Saux]

If the clock on the local machine is slow, and the OCSP responder is fast, an
OCSP response will appear to be in the future. NSS rejects all responses in the
future.

NSS should have a built-in slop time.

Ideally, NSS should let applications set the slop time for both future and old
OCSP responses.

Finally, (but this may be a separate RFE), the application would benefit from an
API which would return how far in the future/past (from the current local time)
the response was.

Comment 1

[Wan-Teh Chang]

Julien, could you take a look at this?  Thanks.

Comment 2

[Julien Pierre]

What should the default slop time be, for both future and past responses ?
It won't be hard to add new APIs to set the default, I expect to have a patch soon.

However, I'm going to pass on the support for returning how far off the response
is. The current API isn't suited for it as it only returns SECStatus throughout
many function calls. If this is a hard requirement for you, please file another
enhancement request, but I'm not sure that I can implement this in the 3.6
timeframe given all the other CRL items I still have.

Comment 3

[Robert Relyea]

Nelson commented in bug 159591 about this.

We should use the same slop time set for Cert processing. We already do some
future slop calculations for certs. we should do the same for OCSP.

Comment 4

[Julien Pierre]

Attachment: allow slop time based on CERT_GetSlopTime()

This patch will allow future OCSP response up to n + sloptime where sloptime is
set by CERT_GetSlopTime(), an existing API which was used for the same purpose
to allow certs with a future validity date.

Comment 5

[Wan-Teh Chang]

Comment on attachment 93639 [details] [diff] [review]
allow slop time based on CERT_GetSlopTime()

>+    LL_UI2L(tmstamp, CERT_GetSlopTime()); /* get slop time in seconds */
>+    LL_I2L(tmp, PR_USEC_PER_SEC);

Nit: use LL_I2L for CERT_GetSlopTime() (which returns PRInt32)
and LL_UI2L for PR_USEC_PER_SEC (which is an unsigned constant).

Other than that, your patch is correct.

Looking forward to the day when all compilers support long long.

Comment 6

[Terry Hayes]

I don't think it's appropriate to use the same grace interval for both 
certificates/CRLs and OCSP. The reason for detecting a response that is in the 
future is to generate an error when the local clock is slow by a significant 
amount. In situations where the clock is slow, a certificate or OCSP response 
will appear to be valid for a longer period than intended.  For certificates 
(which typically have 6 month validity time), accepting the certificate for an 
additional day is not a huge security problem.  However, OCSP response are 
supposed to be more current. Adding 24 hours to the valid time period is a 
significant change.

I recommmend that we set the OCSP grace period to 5 minutes.  Clients that use 
OCSP (which is optional) will need to have the clock set within this bound.
 

Comment 7

[Julien Pierre]

Wan-Teh,

I modeled my code after the one in CERT_CheckCertValidTimes - it turns out it
was doing the wrong thing macro-wise.

Per Terry's comments, we are going to use a different slop time, which I am
making unsigned.

Comment 8

[Julien Pierre]

Attachment: patch with separate slop time for OCSP, defaulting to 5 minutes

Comment 9

[Julien Pierre]

Looks like bugmail got lost - there was no notification when I made the patch
yesterday at 17:34 . Hopefully this update will send a mail.

Comment 10

[Wan-Teh Chang]

Comment on attachment 93648 [details] [diff] [review]
patch with separate slop time for OCSP, defaulting to 5 minutes

1. Nit: use PRInt32 as the type of the OCSP slop time
and have CERT_SetOCSPSlotTime return SECStatus, to be
consistent with CERT_GetSlopTime and CERT_SetSlopTime.
With this change, you'll need to use LL_I2L with
ocspsloptime.

2. I am not convinced that it's necessary to allow the
OCSP slop time to be configurable.  Either delete the
get/set functions, or declare them in ocsp.h and add
them to lib/nss/nss.def.

Comment 11

[Julien Pierre]

1. I chose a PRUint32 because logically it's an interval that can only be
positive. I know the existing function works differently, but I think it was a
flaw in the prototype that I didn't want to continue.

2. I did have a patch for nss.def, I just didn't include it because I have other
functions in it as well.

Comment 12

[Julien Pierre]

Attachment: patch to hard-code OCSP slop time to 5 minutes

Comment 13

[Wan-Teh Chang]

Comment on attachment 93797 [details] [diff] [review]
patch to hard-code OCSP slop time to 5 minutes

r=wtc.

Comment 14

[Julien Pierre]

Checked in to the NSS 3.6 tip.

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

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Why was the slop time reduced to 5 minutes?  

One of the reasons the cert slop time is so large is that it solves problems
caused by having the local system's time zone setting be wrong, even by
several time zones.  5 minutes slop time won't allow any correction for 
time zone mistakes.

Comment 16

[Terry Hayes]

OCSP is supposed to provide timely status information about certificates. It's 
possible to deploy systems where OCSP can indicate that a certificate has been 
revoked within minutes of the status change. In order to allow for mis-setting 
time zones a slop of 12-24 hours would be necessary.  This would allow the 
system clock to be slow by that amount, which would make it impossible provide 
status information that is accurate within a few minutes or hours.

While most applications won't need this level of response, the (single) slop 
value that we choose should support it.  Installations where OCSP is in use 
will need to make sure the system clocks are set correctly.

Comment 17

[Kai Engert [:KaiE:]]

A 5 minutes default is bad for average users. It would probably make the browser
be harder to user for those users, if OCSP gets enabled by default, as being
requested by bug 110161.

But I agree that 5 minutes is a good choice for deployments where security is
critical.

I suggest the slop time should be configurable by the application.

Comment 18

[Scott A. Colcord]

*** Bug 165338 has been marked as a duplicate of this bug. ***