408847 - pkix_OcspChecker_Check does not support specified responder (and given signercert)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert [:KaiE:]]

The comment in front of function pkix_OcspChecker_Check reads:

 * The OCSPChecker is created in an idle state, and remains in this state until
 * either (a) the default Responder has been set and enabled, and a Check
 * request is received with no responder specified, or (b) a Check request is
 * received with a specified responder.
 ...

I conclude this function should support both
(a) application wants to use ocsp responder listed in cert
(b) application wants to use a specific ocsp responder

The current code does not support (b).

It will call pkix_pl_OcspRequest_Create,
which calls CERT_GetOCSPAuthorityInfoAccessLocation.

If there is no AIA contained in the cert,
the function will return and indicate uriFound=PKIX_FALSE.

Back in pkix_OcspChecker_Check, we have this code:
  /* No uri to check is considered passing! */
  if (uriFound == PKIX_FALSE)
    passed = PKIX_TRUE;


This means, if I understand correctly, the implementation is not complete.
The code ignores it if the application globally requested a specific responder (and signer cert).

Worse, the implementation will make the application believe "OCSP got checked and it's good".

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Yup, that's a bug.  I have the impression that the use of "designated OCSP
responders" (ones chosen by the app/user, rather than from the cert's AIA),
is rather rare, so I think rather few users would be affected.  But the ones 
that do rely on this feature would be seriously impacted by this bug.

Comment 2

[Kai Engert [:KaiE:]]

I think this must block the NSS release/snapshot given to support EV in FF 3.

Users who explicitly configure a specific responder may get EV UI because of this false positive.

Comment 4

[Alexei Volkov]

Attachment: Patch v1 - check if ocsp context has default responder URI

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 327896 [details] [diff] [review]
Patch v1 - check if ocsp context has default responder URI

2 comments about this patch.

1. Let's use a more accurate word than the word "default".

>+ * FUNCTION: ocsp_GetResponderLocation
>+ *  Check ocspx context for default responder URI first. 

Let's not call it the "default" responder, because that value is not a 
"default" in any sense of that word.  It's the responder that the local 
user has told us to use.  When specified, we always use it, not merely 
"by default".   Let's call it the "user-designated" responder. 

2. This patch appears to ALSO remove the existing incomplete 
implementation of the "Service Locator" feature of OCSP.  

>@@ -268,9 +268,6 @@ pkix_pl_OcspRequest_RegisterSelf(void *p
>  *  "validity"
>  *     Address of the Date for which the Cert's validity is to be determined.
>  *     May be NULL.
>- *  "addServiceLocator"
>- *     Boolean value indicating whether the request should include the
>- *     AddServiceLocator extension
>  *  "signerCert"
>  *     Address of the Cert to be used, if present, in signing the request.
>  *     May be NULL.

>@@ -290,7 +287,6 @@ pkix_pl_OcspRequest_Create(
>         PKIX_PL_Cert *cert,
>         PKIX_PL_OcspCertID *cid,
>         PKIX_PL_Date *validity,
>-        PKIX_Boolean addServiceLocator,
>         PKIX_PL_Cert *signerCert,
>         PKIX_Boolean *pURIFound,
>         PKIX_PL_OcspRequest **pRequest,
>@@ -298,6 +294,7 @@ pkix_pl_OcspRequest_Create(

I wonder, do we really want to remove that?
Granted it's incomplete.  Maybe we should even assert that it's
not set (that the boolean is false) until it is implemented.
But if we take this out of the API now, then we'll have to change
the API signature again if we decide to complete the implementation
later.  

If we're certain that we never want to implement that feature, then
we can take it out.  Otherwise...

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 327896 [details] [diff] [review]
Patch v1 - check if ocsp context has default responder URI

r=nelson

Comment 7

[Alexei Volkov]

> 2. This patch appears to ALSO remove the existing incomplete 
> implementation of the "Service Locator" feature of OCSP.  
> 
> >@@ -290,7 +287,6 @@ pkix_pl_OcspRequest_Create(
> >         PKIX_PL_Cert *cert,
> >         PKIX_PL_OcspCertID *cid,
> >         PKIX_PL_Date *validity,
> >-        PKIX_Boolean addServiceLocator,
> >         PKIX_PL_Cert *signerCert,
> >         PKIX_Boolean *pURIFound,
> >         PKIX_PL_OcspRequest **pRequest,
> >@@ -298,6 +294,7 @@ pkix_pl_OcspRequest_Create(

This patch simplifies semantic of the function above since the argument is not used outside of the function. It gets defined, calculated and used internally.

Patch is integrated with a change to the ocsp_GetResponderLocation function comment.