The default bug view has changed. See this FAQ.

pkix_OcspChecker_Check does not support specified responder (and given signercert)

RESOLVED FIXED in 3.12.1

Status

NSS
Libraries
P1
major
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: kaie, Assigned: Alexei Volkov)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
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".
(Reporter)

Updated

9 years ago
Whiteboard: PKIX
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.
(Reporter)

Comment 2

9 years ago
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.
Severity: normal → major
Priority: -- → P1
Target Milestone: --- → 3.12

Updated

9 years ago
Target Milestone: 3.12 → 3.12.1

Updated

9 years ago
Assignee: nobody → alexei.volkov.bugs
(Assignee)

Updated

9 years ago
Target Milestone: 3.12.1 → 3.12
(Assignee)

Updated

9 years ago
Target Milestone: 3.12 → 3.12.1
Duplicate of this bug: 403844
(Assignee)

Comment 4

9 years ago
Created attachment 327896 [details] [diff] [review]
Patch v1 - check if ocsp context has default responder URI
Attachment #327896 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #327896 - Flags: review? → review?(nelson)
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 on attachment 327896 [details] [diff] [review]
Patch v1 - check if ocsp context has default responder URI

r=nelson
Attachment #327896 - Flags: review?(nelson) → review+
(Assignee)

Comment 7

9 years ago
> 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.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.