338986 - Unauthorized OCSP response error from user's default OCSP responder

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Tomas Heredia]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

# openssl ocsp -issuer ca-sgp.pem -cert arcert.pem -url
http://192.168.147.166:2560  -CAfile work/RootCA/RootCAcert.pem -VAfile
work/Tokens/ocsp4cert.pem
Response verify OK
arcert.pem: good
        This Update: May 22 20:30:00 2006 GMT
        Next Update: May 23 14:23:28 2006 GMT
---
_BUT_ when trying to verify the cert uing Firefox, it responds "Error trying to validate certificate from www.domain.com using OCSP - unauthorized response" (no numerical error).

All certificates (Site, Site CA, OCSP CA and OCSP responder) are in Firefox's repository, and all are set to be trusted.

"security.OCSP.enabled" == 2
When configuring "Responder signer" in Firefox, only CAs appear in the list, and not the OCSP responder itself.

Reproducible: Always

Comment 1

[Tomas Heredia]

Attachment: testcase certificates and sample request and response

Comment 2

[Tomas Heredia]

related to bug 234129?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

You got error SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE 

The error message you cited suggests that the built-in client sent the OCSP
request and received the response, and then concluded that the response was
not from a server that is truly authorized to be the OCSP responder for the
CA that issued the cert being checked, or that the response was for a
different cert than the cert for which the request was made..

In order to be valid, the OCSP response has to be signed, and the signature
verifiable from the CA that issued the cert being checked, or a duly
authorized surrogate, and finally the response has to be spefically about
the cert for which the request was made, and not for some other cert.

When NSS gets the OCSP response, it also needs the certificate of the
responder server.  You can see the code that does this work at
http://lxr.mozilla.org/security/source/security/nss/lib/certhigh/ocsp.c#2976

(I'm not claiming that the steps described below are all necessarily
correct.  I'm only documenting what NSS is now doing.)

NSS checks to see if the responder's certificate passes any of the following
3 tests (in this order):

1. Is the responder's certificate one that has been locally designated
in the OCSP client's configuration as THE "default" OCSP responder cert.
If the local sysadmin/user can configured his OCSP client to trust a
"default" OCSP responder, then NSS will honor OCSP responses from that
responder.  (I doubt that you have configured a default OCSP responder.)

2. Is the responder an officially "designated" responder for the CA that
issued the cert being checked?  (more on that below) and if so, was the
responder's cert issued by the issuer of the cert whose revocation is 
being checked?

3. If not a designated responder, then is the responder cert actually the
issuer cert for the cert being checked?

If the answer to all 3 of those questions is no, you will get
SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE.

If the answer to any of those questions is yes, then NSS will check
that the response contains the hash of the cert being checked, the same
hash that the OCSP client sent to the OCSP responder.  If the hashes
don't match, then you get SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE.

On being an officially "designated" responder:
The test for being a designated responder starts by checking the responder's
cert for an "Extended Key Usage" extension that contains the "Object ID"
(OID) for being an OCSP responder.  That OID is 1.3.6.1.5.5.7.3.9
(See http://www.alvestrand.no/objectid/1.3.6.1.5.5.7.3.9.html )

This OID is only to be used in designated responders.  So, if it is present,
and if the rest of the designate responder test fails, NSS does not go
ahead with the third question:
("Is the responder cert actually the issuer cert?")

If the responder cert has the designated responder OID in its Extended Key
Usage extension, then NSS looks for the issuer of this designated responder.
The issuer of the designated responder MUST be the issuer of the cert being
checked.  If the designated responder cert is not issued by the cert's own
issuer, you get SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE.

I think that when the OCSP responder's cert is also the issuer cert for
the cert being checked, it (the responder cert) should NOT have the OID
for designated responder.  It's not designated by the CA, it IS the CA,
the designator!  NSS is pretty exacting about that.

My guess is that your CA cert has the EKU OID for being a designated OCSP
responder, even though it is the cert's issuer.  NSS wants to see that OID
only in a "designated responder" cert, not in the issuer cert.

Julien, is it proper for NSS to require the signer cert to NOT be the 
issuer cert (but be issued by the issuer cert) when this OID is present?

Comment 4

[Julien Pierre]

Nelson,

Per RFC2560, section 4.2.2.2 "Authorized responder" :

   They MUST reject the
   response if the certificate required to validate the signature on the
   response fails to meet at least one of the following criteria:

   1. Matches a local configuration of OCSP signing authority for the
   certificate in question; or

   2. Is the certificate of the CA that issued the certificate in
   question; or

   3. Includes a value of id-ad-ocspSigning in an ExtendedKeyUsage
   extension and is issued by the CA that issued the certificate in
   question."

My reading of the above is that even if the cert fails the 3rd test by having the EKU and not being the issuer of the OCSP signing cert, we must still consider the other 2 cases - that is, that the OCSP signing certificate is locally configured, or that it is the issuer of the certificate in question.

Comment 5

[Philip Withnall (unavailable)]

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

Comment 6

[Philip Withnall (unavailable)]

(In addition to comment #5)
> *** Bug 337678 has been marked as a duplicate of this bug. ***
> 

I've duped the older bug to this one because this has got more information. You might want to look at bug #337678 though, because it does have _some_ details.

Comment 7

[Tomas Heredia]

First of all, I´d like to clarify the scenario. In the case we are working, there are:
- OCSP client (Firefox)
- OCSP Server (locally designated responder)
- CA1 issuer of the OCSP server´s certificate (local trusted CA)

- HTTPS server (external)
- CA2 issuer of the HTTPS Server´s certificate (also trusted).


(In reply to comment #3)
> [...]
> When NSS gets the OCSP response, it also needs the certificate of the
> responder server.  You can see the code that does this work at
> http://lxr.mozilla.org/security/source/security/nss/lib/certhigh/ocsp.c#2976

Ok. It´s stored in the firefox´s repository, and viewable in the "web sites" tab.

> [...]
> 1. Is the responder's certificate one that has been locally designated
> in the OCSP client's configuration as THE "default" OCSP responder cert.
> If the local sysadmin/user can configured his OCSP client to trust a
> "default" OCSP responder, then NSS will honor OCSP responses from that
> responder.  (I doubt that you have configured a default OCSP responder.)

The actual setup is:
user_pref("security.OCSP.URL", "http://192.168.147.166:2560/");
user_pref("security.OCSP.enabled", 2);
user_pref("security.OCSP.signingCA", "CA - HMSYS");

Note thay "CA - HMSYS" is the CA isuing the _locally designated responder´s_ certificate, not the responder´s itself. The drop down menu doesn´t allow to select any certificates but those stored as CAs, nor putting it manually in prefs.js works.

I think this may be an important symptom of the problem.

> [...]
> If the answer to all 3 of those questions is no, you will get
> SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE.

I the setup we are working, number 1 answer is the one expected to be YES.

> [...]
> On being an officially "designated" responder:
> The test for being a designated responder starts by checking the responder's
> cert for an "Extended Key Usage" extension that contains the "Object ID"
> (OID) for being an OCSP responder.  That OID is 1.3.6.1.5.5.7.3.9
> (See http://www.alvestrand.no/objectid/1.3.6.1.5.5.7.3.9.html )

The responder is not the "officialy designated" one, but it´s the "locally designated" responder.
It has the 1.3.6.1.5.5.7.3.9 OID, as stated in various documents external to Mozilla.

It also has the 1.3.6.1.5.5.7.48.1.5 (id-pkix-ocsp-nocheck), anyway it doesn´t work neither with or without it, or with the responder responding for the ocsp responder´s CA too or not.

> This OID is only to be used in designated responders.  So, if it is present,
> and if the rest of the designate responder test fails, NSS does not go
> ahead with the third question:
> ("Is the responder cert actually the issuer cert?")

No. We are working in case 1 (locally designated responder). It doesn´t work without it neither. If the OID is not present, I get an -8048 error (SEC_ERROR_OCSP_INVALID_SIGNING_CERT).

 
> If the responder cert has the designated responder OID in its Extended Key
> Usage extension, then NSS looks for the issuer of this designated responder.
> The issuer of the designated responder MUST be the issuer of the cert being
> checked.  If the designated responder cert is not issued by the cert's own
> issuer, you get SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE.

May be THIS is the error instance I´m getting? I think this chek is invalid if we are in "case 1"

> [...]
> My guess is that your CA cert has the EKU OID for being a designated OCSP
> responder, even though it is the cert's issuer.  NSS wants to see that OID
> only in a "designated responder" cert, not in the issuer cert.

No, It´s not the cert issuer, it´s an external responder ("locally designated responder")

> [...]

Hope this Helps,
Tomas

Comment 8

[Tomas Heredia]

The problem here is that on Firefox on Windows Platform, the OCSP signer´s certificate is silently discarded when trying to load it in the CA´s tab, but it´s loaded OK as a "web site". This can be reproduced using "ocsp4cert.pem" from testcase attachment.
So THAT was why the OCSP responder´s certificate couldn´t be used as "response signer".
I´ve confirmed that mozilla browser (1.7.8) on Linux platform works OK.

As the OCSP responder is nor a CA neither a Web Site, I think there are two possible problems here:
- OCSP Signer´s certificate silently discarded when importing it as a CA (may be related to bug 335636, bug 230301, or bug 197316?)
- CertIsDefaultResponderForCertID function (http://lxr.mozilla.org/security/source/security/nss/lib/certhigh/ocsp.c#2965, or possibly the configuration framework itself should recognize "web site" certificates as possible "Response Signers".

Comment 9

[Tomas Heredia]

Confirmed to work OK on Mozilla 1.7.13 on Windows platform. Setting Product to Firefox.
Firefox 1.0.4 also works OK on Linux platform, so it appears to be a  platform specific issue. Leaving Hardware: PC and OS: Windows (XP).
Also setting Severity to Normal, as it doesn´t affect ALL products and platforms (Is this ok?)

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Thomas,  all OCSP code is in NSS.  If you assign this bug to another product, 
the NSS developers will not look at it.  That's counter productive.

The reported observation that OCSP works with this responder in old mozilla 
browsers and not in newer FireFox ones suggests that it may be a regression.  

The Certificate Manager dialog tab in which the cert appears is 
not relevant to the OCSP issue.  It is a mere display issue.
NSS does not have separate certificate stores for each tab.  
The choice of tab in which a cert appears is merely a display time 
issue.  An OCSP responder is a server.  If its cert is not a CA cert
then the tab for server certs seems like exactly the right place for
that cert to be displayed.  

Instructions on how to reproduce this would be helpful.
Is your default OCSP repsonder publicly accessible?
Can you provide a URL with which the problem may be reproduced by others?

Comment 11

[Tomas Heredia]

(In reply to comment #10)
> Thomas,  all OCSP code is in NSS.  If you assign this bug to another product, 
> the NSS developers will not look at it.  That's counter productive.

Thanks for your explanation. I really thought it could be a product speciffic poroblem

> [...]
> The Certificate Manager dialog tab in which the cert appears is 
> not relevant to the OCSP issue.  It is a mere display issue.
> NSS does not have separate certificate stores for each tab.  
> The choice of tab in which a cert appears is merely a display time 
> issue.  An OCSP responder is a server.  If its cert is not a CA cert
> then the tab for server certs seems like exactly the right place for
> that cert to be displayed.

_BUT_, On Mozilla (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060414),
if I load de OCSP responder cert in the "web sites", tab, it doesn´t appear any more in the "response Signer" drop down list. I can only select it as a "Response Signer" if it was loaded via de CA´s tab.
And on Firefox (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3)
I can´t load de certificate in the CA´s tab, but only in the web servers one. As on Mozilla, It doesn´t appear on the "Response Signers" drop down list.
At least on Firefox (didn´t try on mozilla) setting it by hand on prefs.js ,while the OCSP Signer´s cert was loaded as web server, doesn´t help neither (it just disables OCSP). So I think there IS some kind of difference... may be some kind of tag in the store?

> Instructions on how to reproduce this would be helpful.

Sure.

> Is your default OCSP repsonder publicly accessible?

I´m afraid not by now.

> Can you provide a URL with which the problem may be reproduced by others?
Not now, but if it´s enough to solve the problem you could reproduce most of the problem like this on Firefox (version stated above):
1. Load RootCAcert.pem (from testcase attached to bug). No need to trust it
2. Try to load ocsp4cert.pem from the "Authorities" tab. Note that it silently fails.
3. Load ocsp4cert.pem from "Web Sites" tab. Verify it worked OK.
4. Look at "Response Signer" drop down list: this cert (HMSYS - ocsp4) doesn't appear, nor is it usable if configured on prefs.js

If you need more info, clarifications or help, please let me know (I´m pretty sure I can´t help with code itself, but I could provide complete setup instructions and testcase to reproduce the hole problem)

Comment 12

[Tomas Heredia]

Well, It´s driving me crazy!
I´v been trying different Firefox versions from scratch, and here are some facts about the problem. I think there are 2 different problems here.
1. - OCSP Signer certificate is ONLY usable if it appears on the CA tab. As Nelson said, it´s not a CA (in this case), but neither is it a web site... I think there should be a new bug filed for this problem. I´ve just verified this behaviour in Firefox 1.0.7, 1.5 and 1.5.3.
2. - At least from Firefox 1.5, Non CA certificates cannot be loaded in the CA tab, so they cannot be used as OSCP Signers. Firefox 1.0.7 allows to load them OK, and they remain in the CA tab if Firefox is upgraded to 1.5 and/or 1.5.3.
Versions used:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

I think I now have a much better understanding of comment 11 and comment 12.
Apparently, some older version of PSM (and/or NSS) would set the "valid CA"
trust flag on OCSP responder certs, even though they are (and were) *NOT* CAs.
This trust flag told NSS to give those certs the full capabilities of CA certs.

That was a bug (IMO).  Only CA certs should show up as CA certs in the CA 
certs tab of cert manager (IMO).  This bug caused those non-CA certs to show 
up in the cert manager tab for CA certs.  AND it apparently allowed those 
certs to be listed in the drop down list of OCSP responder certs in the cert 
validation preferences UI.  It is suggested that PSM's UI for selecting OCSP
responder certs depended on that bug (I'm not entirely convinced of that yet).

Apparently, when the bug was fixed, such that OCSP responder certs were no 
longer marked as being CA certs, OCSP responder certs loaded into the DB
thereafter did not appear in the drop down list of OCSP responder certs in 
PSM's UI, and therefore could not be selected.

Now, I think that perhaps the intended purpose of some of the text boxes in
the preferences dialog for OCSP responder selection may differ from the 
apparent purpose, and I'm looking into that before saying more about it.

I think it may also be the case that the UI designers assumed that a 
locally designated "default" OCSP responder would be a valid normal 
(not locally designated default) responder for SOME CA somewhere. 
Perhaps that was a false assumption.  

Thomas, I would not assume that PSM "silently fails" to import a cert just
because that cert doesn't appear in the tab from which the import was 
initiated.  One of the problems with the design of PSM's cert manager UI
is that it sets false expectations that a cert will always be imported into
the tab from which the import was initiated.  But that's not how it works.

When a cert is imported, it is imported into the cert store.  It is 
subsequently displayed in whichever tab seems most appropriate for that 
cert, REGARDLESS of the tab in which the "import" button was pressed.  

That's a UI design failure.  Users naturally think the cert was not imported
and that it "silently failed", when it MAY be the case that the cert WAS 
imported, but showed up in another tab.  

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #12)

> [...] Firefox 1.5, Non CA certificates cannot be loaded in the CA tab, 

That's not a bug.  That's correct behavior.  The previous behavior was a bug.

> so they cannot be used as OSCP Signers. 

Or rather, they do not appear in the preference dialog's drop-down list of 
OCSP responder certs (or whatever it is a list of).  That's not quite the
same as saying they cannot be used a OCSP responder certs, but only that the
UI doesn't make it easy or apparent how to do so.  
The solution is not to put OCSP responder certs back in the CA cert tab, but
rather is to make the appropriate certs selectable in the OCSP preference 
panel's drop down list.  But I'm not entirely sure there's a bug there yet.

Must do more research.  Unfortunately, this may be rather difficult without
the ability to actually test with the live responer.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

BTW, I've added numerous screen shots and comments about the OCSP responder selection preference UI to bug 209837. 

Comment 16

[Tomas Heredia]

(In reply to comment #13)
> [..]
> Thomas, I would not assume that PSM "silently fails" to import a cert just
> because that cert doesn't appear in the tab from which the import was 
> initiated.  One of the problems with the design of PSM's cert manager UI
> is that it sets false expectations that a cert will always be imported into
> the tab from which the import was initiated.  But that's not how it works.

I verified tha the certificate included in the testcase file attached  (ocsp4cert.pem) doesn´t appear in any tab when trying to load it from the CA tab. May be it´s in the store but doesn´t appear in any tab?

> [...]

(In reply to comment #14)
> [...]
> Or rather, they do not appear in the preference dialog's drop-down list of 
> OCSP responder certs (or whatever it is a list of).  That's not quite the
> same as saying they cannot be used a OCSP responder certs, but only that the
> UI doesn't make it easy or apparent how to do so.

They can´t be used configuring it in the prefs.js file neither. (I tried using the standard name: "DN - CADN", like
user_pref("security.OCSP.signingCA", "ocsp4 - HMSYS");
In this case, no OCSP request appears in network captures, but the page is loaded OK. I assume in this case the Web Site certificate is accepted without doing the OCSP check (May be there´s even another new bug? OCSP silently disabled when prefs.js missconfigured?)
  
> The solution is not to put OCSP responder certs back in the CA cert tab, but
> rather is to make the appropriate certs selectable in the OCSP preference 
> panel's drop down list.

Totally agree.

> But I'm not entirely sure there's a bug there yet.

See above. Certificate included in testcase (ocsp4cert.pem) is a valid OCSP responder cert, and can´t be used as such in firefox.

> Must do more research.  Unfortunately, this may be rather difficult without
> the ability to actually test with the live responer.

I can´t help with a live responder by now, but I have the needed infrastructure in VM´s, so let me help with it if you want.

Comment 17

[Tomas Heredia]

Confirmed same behaviour in Forefox 1.5.0.4 on Linux, so changing OS and Hardware to ALL

Comment 18

[Massimiliano Pala]

Most of the problems (besides the display of the certificate and the selection to act as an OCSP responder) would be solved if a new checkbox could be added to the UI:

   [] Act as a Trusted Responder

meaning that (if checked) as long as the responder certificate is trusted, the responder can provide responses also for certificates issued by 3rd party CAs. Thus enabling a trusted OCSP responder to act as a gateway for multiple CAs.

This behavior is what most of the users (and OCSP administrators) expect when selecting a fixed OCSP server in the configuration.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Alexei and I discussed this today.  

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

After our discussion, I've concluded that 

1) ocsp_AuthorizedResponderForCertID does the wrong thing.  It checks for the 
presence of the id-ad-ocspSigning OID in an ExtendedKeyUsage extension RATHER
than checking to see if the cert is the issuer cert.  

2) ocsp_CheckSignature calls CERT_VerifyCert to ask if the cert is a valid
OCSP responder, regardless of whether the cert being checked is the issuer 
or is a "designated responder", but CERT_VerifyCert interprets certUsageStatusResponder as meaning it should check ONLY for being a 
designated responder, not also for being the issuer.  

There are several ways we could fix this bug, including:

a) Change Cert_VerifyCert(ificate) to not check for the presence of the 
id-ad-ocspSigning OID for certUsageStatusResponder
b) Change ocsp_CheckSignature to first determine if the response was signed
by the cert's issuer or by a designated responder, and then perform separate
CERT_Verify* calls for each case.  

Comment 21

[Alexei Volkov]

Attachment: changes in OCSP response verification procedure

* makes use of CERT_VerifyCACertForUsage function for cert usage verification if issuer of the response is CA cert

* ocsp_AuthorizedResponderForCertID function now checks if signer cert is the issuer. Only in case of comparison failure the function tries to find if the signer cert is the CA Designated Responder.

* some trivial function were eliminated as they no longer used.

* ocspclnt cert ref leak

Comment 22

[Alexei Volkov]

ocsp_AuthorizedResponderForCertID tries to identify if a response was signed by an authorized responder. The patch in attachment 247887 [details] [diff] [review] calculates pub key hash of the cert that signed the response and compares this hash with previously calculated pub key hash of an issuer of the cert in question. Here is the reason why it will work:

   The primary reason to use the hash of the CA's public key in addition
   to the hash of the CA's name, to identify the issuer, is that it is
   possible that two CAs may choose to use the same Name (uniqueness in
   the Name is a recommendation that cannot be enforced). Two CAs will
   never, however, have the same public key unless the CAs either
   explicitly decided to share their private key, or the key of one of
   the CAs was compromised.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 247887 [details] [diff] [review]
changes in OCSP response verification procedure

Several issues with this patch.

1. There are lots of lines in this patch that are longer than 80 columns
Here is one of many.  Please wrap them.

>+    ocspResponseData *tbsData = ocsp_GetResponseData(response); /* this is what is signed */
>+    ocspSignature *signature = ocsp_GetResponseSignature(response);
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

2. The above two lines of code return pointers that are not checked for
NULL before we use them.  

3. The block of code below doesn't conform to our indentation style.
Please fix it to do so.

>+    PORT_Assert(tbsData->responderID != NULL);
>+    switch (tbsData->responderID->responderIDType) {
>+      case ocspResponderID_byName:
>+	lookupByName = PR_TRUE;
>+	certIndex = &tbsData->derResponderID;
>+	break;
>+      case ocspResponderID_byKey:
>+	lookupByName = PR_FALSE;
>+	certIndex = &tbsData->responderID->responderIDValue.keyHash;
>+	break;
>+      case ocspResponderID_other:
>+      default:
>+	PORT_Assert(0);
>+	PORT_SetError(SEC_ERROR_OCSP_MALFORMED_RESPONSE);
>+	return SECFailure;
>+    }

4. more lines too long.

>+        rv = CERT_VerifyCACertForUsage(handle, signerCert, PR_TRUE, certUsageStatusResponder,
>+        rv = CERT_VerifyCertificate(handle, signerCert, PR_TRUE, certUsageStatusResponder,
>+    encodedTBS = SEC_ASN1EncodeItem(NULL, NULL, tbsData, ocsp_ResponseDataTemplate);
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

5. Is there an issue here with recusion?  
If CERT_VerifyCertificate calls OCSP and OCSP calls CERT_VerifyCertificate...
I think Julien actually filed a bug on this.  Please check.

>+     * First, lets check if signer of the response is the acctual issuer
>+     * of the cert. For that we will use signer cert key hash and will
>+     * compare it with already calculated issuer key hash. The hash algorithm
>+     * is picked from response certID hash to avoid second hash calculation.

The RFC you quoted in comment 22 says to " use the hash of the CA's public key 
*in addition to* the hash of the CA's name, to identify the issuer", so I think
the patch should check both.

stValueForCert(NULL, signerCert, hashAlg, NULL);
>+    if (keyHash != NULL) {
>+        PRBool hashEQ =
>+            (SECITEM_CompareItem(keyHash,
>+                                 &certID->issuerKeyHash) == SECEqual) ? PR_TRUE :
>+                                                                        PR_FALSE;
..12345678901234567890123456789012345678901234567890123456789012345678901234567890
too long

>+    if (keyHash != NULL) {
>+        PRBool hashEQ = 
>+            (SECITEM_CompareItem(keyHash,
>+                                 &certID->issuerKeyHash) != SECEqual) ? PR_TRUE :
>+                                                                        PR_FALSE;

too long

Comment 24

[Alexei Volkov]

> 5. Is there an issue here with recusion?  
> If CERT_VerifyCertificate calls OCSP and OCSP calls CERT_VerifyCertificate...
> I think Julien actually filed a bug on this.  Please check.

Didn't find any bugs related to this matter. Recursion may happen if OCSP is enabled, trusted responder is set and CERT_VerifyCertificate is used to validate signer of OCSP response cert to any cert usage, but certificateUsageStatusResponder. The function will not recur if 
certificateUsageStatusResponder flag is precent in requiredUsages argument.

Comment 25

[Alexei Volkov]

Attachment: patch + comments implementation

Current OCSP library implementation will fail to validate any ocsp responses from a responder that uses a trusted responder cert to sign responses provided that the cert does not have OCSPSigning eku extensions. The reason for a failure will be reported as invalid signature on an OCSP responses. The real reason for the failure is false result received from validation of cert-signer(trusted responder cert).

To overcome trusted responder cert validation problem, I propose to validate trusted responder cert only at the time library enables default responder. We should not allowed to set trusted responder to invalid cert. Signer cert of OCSP response will not be validated if this cert is trusted responder cert.

Attached patch implements the proposal.

Comments?

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Alexei, 
Your second patch does not patch all the files that the first patch did.
The first patch modified ocspclnt.c, but the second patch does not.
So, it appears that the second patch is incomplete.  
Did the patch to ocspclnt already get committed?  or?

Comment 27

[Alexei Volkov]

patch to ocspclnt is already committed as a part of a fix for bug 363480

Comment 28

[Kai Engert (:KaiE:)]

Please ask me for review once you got r+ from Nelson.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 252659 [details] [diff] [review]
patch + comments implementation

Here are some additional changes to the ocsp code (and your patch) that
I want you to make.

Please rename the following symbols (function names, variables names)
throughout the ocsp code (and in your patch).

SEC_ERROR_OCSP_DEFAULT_RESPONDER_CERT_INVALID -> 
        SEC_ERROR_OCSP_RESPONDER_CERT_INVALID

CERT_SNDigestValueForCert  -> cert_GetSubjectNameDigest
CERT_SPKDigestValueForCert -> cert_GetSPKIDigest 
                        Note the new I   ^

Please move those two new functions up near the top of ocsp.c 
so that there won't need to be any forward declarations like this one:

>+extern SECItem *
>+CERT_SNDigestValueForCert(PRArenaPool *arena, CERTCertificate *cert,
>+                          SECOidTag digestAlg, SECItem *fill);
>+

Please change the type of the second argument for each of those two
functions from "CERTCertificate *" to "const CERTCertificate *"

If those functions are only used in one source file, please make them static.

Please move any necessary forward static function declarations up to the top
of the source file, so code readers won't have to search the entire file
to find them.

At http://lxr.mozilla.org/security/source/security/nss/lib/certdb/certt.h#496
add a line like this:

#define certificateUsageCheckAllUsages       0

and use that symbol, rather than (SECCertificateUsage)0 in this line:

+ rv = CERT_VerifyCertificateNow(handle, cert, PR_TRUE, (SECCertificateUsage)0,

>+    int certCount = 0;
>+    PRBool lookupByName;
>+    void *certIndex;
>+    int64 producedAt;
>+    ocspResponseData *tbsData = ocsp_GetResponseData(response,
>+                                                     &tbsResponseDataDER);
>+    ocspSignature *signature = ocsp_GetResponseSignature(response);

If there is ANY WAY that ocsp_GetResponseData or ocsp_GetResponseSignature
then the code will crash here (just below) referencing the pointers 
tbsData or "signature".  Unless it is impossible, please add code here that
checks those pointers for NULL before proceeding.  If it is impossible,
please add a comment here, asserting that.


>+	int headerLen = 2 + ((crIndex->data[1] & 0x80) ?
>+                             crIndex->data[1] & 0x7f : 0);

Questions about the above code:
a) can crIndex->data be NULL?
b) can crIndex->len be less than 2 ?
c) can crIndex->len be less than headerLen ?

>+    rv = DER_GeneralizedTimeToTime(&producedAt, &tbsData->producedAt);
>+    if (rv != SECSuccess)
>+	return rv;

That will leak "certs", among other things.  Goto finish instead.

>+    if (ocsp_CertIsOCSPDefaultResponder(handle, signerCert) == PR_TRUE) {

Please don't check for == PR_TRUE. Please either check for != PR_FALSE, or
just skip the comparison, e.g. 
>+    if (ocsp_CertIsOCSPDefaultResponder(handle, signerCert)) {

While reviewing this patch, I found some problems that appear to also 
exist in the existing OCSP code.  So, I am going to send off the review
comments I've collected so far, and go review that code in more depth.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

In comment 29, I wrote:
> If there is ANY WAY that ocsp_GetResponseData or ocsp_GetResponseSignature
> then the code will crash here

I meant to write:
> If there is ANY WAY that ocsp_GetResponseData or ocsp_GetResponseSignature
> can be NULL here, then the code will crash here

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

My comment about not checking for == PR_TRUE, but rather checking for != PR_FALSE
was meant to apply to the whole patch, not just that one place.

There are two places where we have two cert pointers and we want to ask if
they point to the same cert.  The test being done is a mere equality test
of the two pointer values.  That's not enough.  It's possible that the two
pointers will not be equal, but the certs will be equal.  If the two pointers
are equal, then the certs are certainly equal.  But if the pointers are not 
equal then the next thing to do is to call 
  SECITEM_CompareItem(&cert1->derCert, &cert2->derCert)

We should have a function that compares two certs that way, and use it 
rather than repeating the implementation of that test in multiple places.
Maybe we DO have such a function.

The ocsp code is guilty of having many threads all share a single common
reference to a cert, rather than each using its own reference.  The shared
reference is stored in statusContext->defaultResponderCert.
Consequently if any thread destroys that one reference by calling CERT_DisableOCSPDefaultResponder while the other threads are doing OCSP 
processing, a crash is likely to occur.  This could happen in the browser
if the user changed his OCSP responder setting while a window or tab was
still trying to access an https URL.  
And while we're at it, CERT_DisableOCSPDefaultResponder destroys the 
shared reference while the global pointer still points to it.  The code is:

    if (statusContext->defaultResponderCert != NULL) {
	CERT_DestroyCertificate(statusContext->defaultResponderCert);
	statusContext->defaultResponderCert = NULL;
    }
It should be something like
    if ((temp = statusContext->defaultResponderCert) != NULL) {
	statusContext->defaultResponderCert = NULL;
	CERT_DestroyCertificate(temp);
    }

Comment 32

[Alexei Volkov]

Attachment: patch + comments implementation (v2)

patch also fixes two cases in ocspclnt where cert references were freed twice.

Comment 33

[Kai Engert (:KaiE:)]

Nelson, I assume it makes sense you review first.

Comment 34

[Kai Engert (:KaiE:)]

Comment on attachment 254077 [details] [diff] [review]
patch + comments implementation (v2)

I am happy to report, theses changes do NOT conflict with the patch I have in bug 205406. I was able to compile NSS trunk with both patches applied.

(The attached patch does not apply, because a -u10 was used, but a patch with default context size applies mostly fine.)

Comment 35

[Alexei Volkov]

Great to hear this. Thanks Kai for trying it out.

Alexei

Comment 36

[Kai Engert (:KaiE:)]

Comment on attachment 254077 [details] [diff] [review]
patch + comments implementation (v2)

Please note that I did not review your changes to the tests. Do you need me to?

I think I found some issues - but at many points I am just asking questions. If you believe your code is correct in those places, a short explanation will be fine.


     certs = CERT_NewCertList();
-    if (certs == NULL)
+    cert = CERT_DupCertificate(cert);
+    if (certs == NULL || cert == NULL)
        goto loser;

### I guess it's higly unlikely that certs allocation fails,
### and cert dupping works?
### This case would leak a cert reference.
### We probably do not want a cert destroy call after loser,
### so I recomment you use 2 gotos.
### certs = ...; if certs null goto; cert = ...; if cert null goto

     certs = CERT_NewCertList();
-    if (certs == NULL)
+    cert = CERT_DupCertificate(cert);
+    if (certs == NULL || cert == NULL)
        goto loser;

### same here

 
+static SECItem *
+ocsp_DigestValue(PRArenaPool *arena, SECOidTag digestAlg, 
+                 SECItem *fill, SECItem *src)

### is it possible to declare src as const?
### you are playing risky games with the src parameter when calling
### this function, and it would be good if it were assured this
### function will not modify src.


+    if ((fill == NULL) || (fill->data == NULL)) {
+       result = SECITEM_AllocItem(arena, fill, digestObject->length);
+       if ( result == NULL ) {
+          goto loser;
+       }
+       fill = result;
+    }

### And now you lose the information, whether fill was originaly NULL!
### If the following code jumps to loser, you will leak.
### Can you use an additional variable to store the correct value 
### for the second parameter when you later call SECITEM_FreeItem?


+    /*
+     * Now digest the value, using the specified algorithm.
+     */
+    if (PK11_HashBuf(digestAlg, fill->data,
+                     src->data, src->len) != SECSuccess) {

### the original code in CERT_SPKDigestValueForCert (which you removed)
### used a series of operations on digestObject-> and you replaced
### it with a single call to PK11_HashBuf. I have no idea whether
### this is a correct change. Please ask someone who understand this
### code to confirm this is correct.

 
-    tempItem = SEC_ASN1EncodeItem(NULL, NULL, &issuerCert->subject,
-                                 CERT_NameTemplate);
-    if (tempItem == NULL) {
-       goto loser;
-    }
-
-    if (SECITEM_AllocItem(arena, &(certID->issuerNameHash),
-                         SHA1_LENGTH) == NULL) {
-       goto loser;
-    }
-    rv = PK11_HashBuf(SEC_OID_SHA1, certID->issuerNameHash.data,
-                     tempItem->data, tempItem->len);
-    if (rv != SECSuccess) {
+    if (cert_GetSubjectNameDigest(arena, issuerCert, SEC_OID_SHA1,
+                                  &(certID->issuerNameHash)) == NULL) {

### Is SEC_ASN1EncodeItem(cert->subject, CERT_NameTemplate) the same thing 
### as cert->derSubject (which is used in the new function) ?
### Is cert->derSubject always guaranteed to be available?
### I do not know the answer.
### I'm just asking myself, because I wonder, why did the old code
### this additional encoding, when it could have just used derSubject?


-       for (; signature->derCerts[certCount] != NULL; certCount++) {
-           /* just counting */
-           /*IMPORT CERT TO SPKI TABLE */
-       }
-    }
-    rv = CERT_ImportCerts(handle, certUsage, certCount,
+       for (; signature->derCerts[certCount] != NULL; certCount++);

### The code is ok.
### But it took me a moment to see it's an one-line-empty-loop.
### I think the old code using { /* just counting */ }
### was easier to read. Or maybe you could put the semicolon 
### on a separate line to make it more obvious.
### Your decision.


     if (lookupByName) {
-       SECItem *encodedName;
+       SECItem *crIndex = (SECItem*)certIndex;
+       SECItem encodedName;
 
-       encodedName = SEC_ASN1EncodeItem(NULL, NULL, certIndex,
-                                        CERT_NameTemplate);
-       if (encodedName == NULL)
-           goto finish;
+       if (crIndex->len > 1) {        
### do you need to handle "else"?

+
+           int headerLen = 2 + ((crIndex->data[1] & 0x80) ?
+                                 crIndex->data[1] & 0x7f : 0);
### Can you please explain what this does?
### Could you add a comment?
### Does this work for any item size?

+            
+           if (headerLen < crIndex->len) {
### do you need to handle "else"?

+               encodedName.data  = crIndex->data + headerLen;
+               encodedName.len = crIndex->len - headerLen;
### why does this trick work?
### Maybe you save an allocation by using this trick,
### but it seems the old code (that used 
###   certIndex = &tbsData->responderID->responderIDValue.name;
### ) is easier to understand.


-    rv = CERT_VerifyCert(handle, signerCert, PR_TRUE, certUsage, checkTime,
-                        pwArg, NULL);
+    if (ocsp_CertIsOCSPDefaultResponder(handle, signerCert)) {
+        rv = SECSuccess;
+    } else {

### I'm surprised, we really do NOT need to verify that a default OCSP 
### responder cert is (still) valid?

 
     /*
      * Check first for a trusted responder, which overrides everything else.
      */
-    if (ocsp_CertIsDefaultResponderForCertID(handle, signerCert, certID))
+    if ((defRespCert = ocsp_CertGetDefaultResponder(handle, certID)) &&
+        CERT_CompareCerts(defRespCert, signerCert)) {

### I personally think avoding assignments in if statements is a good idea,
### so I would prefer the assignment on a separate line before the if,
### but the code is correct. Your decision.
 

+        keyHashEQ =
+            (SECITEM_CompareItem(keyHash,
+                                 &certID->issuerKeyHash) == SECEqual) ?
+                                                    PR_TRUE : PR_FALSE;

### you could omit ?PR_TRUE:PR_FALSE


+    if (keyHashEQ == PR_TRUE &&

### you might just use: if (keyHashEQ &&


+        nameHashEQ =
+            (SECITEM_CompareItem(nameHash,
+                                 &certID->issuerNameHash) == SECEqual) ?
+                                                    PR_TRUE : PR_FALSE;

### you could omit ?PR_TRUE:PR_FALSE


+        if (nameHashEQ == PR_TRUE) {

### you might just use: if (nameHashEQ) {

 
+        keyHashEQ = 
+            (SECITEM_CompareItem(keyHash,
+                                 &certID->issuerKeyHash) == SECEqual) ?
+                                                    PR_TRUE : PR_FALSE;

### you could omit ?PR_TRUE:PR_FALSE


+        nameHashEQ =
+            (SECITEM_CompareItem(nameHash,
+                                 &certID->issuerNameHash) == SECEqual) ?
+                                                    PR_TRUE : PR_FALSE;

### you could omit ?PR_TRUE:PR_FALSE


+    if (keyHashEQ == PR_TRUE && nameHashEQ == PR_TRUE) {

### if (keyHashEQ && nameHashEQ) {

 
+    rv = CERT_VerifyCertificateNow(handle, cert, PR_TRUE,
+                                   certificateUsageCheckAllUsages,
+                                   NULL, &usage);
+    if (rv != SECSuccess || (usage && (certificateUsageSSLClient |

### are you sure you want a && here?
### I suspect you want the bitwise & operator?


+                                       certificateUsageSSLServer |
+                                       certificateUsageSSLServerWithStepUp |
+                                       certificateUsageEmailSigner |
+                                       certificateUsageObjectSigner |
+                                       certificateUsageStatusResponder |
+                                       certificateUsageSSLCA)) == 0) {
+       PORT_SetError(SEC_ERROR_OCSP_RESPONDER_CERT_INVALID);

### you are checking, does the cert's verified usages include at least one 
### from this given list of usages. 
### If there is no usage in common, you say it's a failure.
### Why is that?
### Why not the other usages?
### Could you please add a comment?


-    if (statusContext->defaultResponderCert != NULL) {
-       CERT_DestroyCertificate(statusContext->defaultResponderCert);
+    if ((tmpCert = statusContext->defaultResponderCert) != NULL) {

### I'd prefer:
###   tmpCert = ...
###   if (tmpCert) {

 
-SECItem *
-CERT_SPKDigestValueForCert(PRArenaPool *arena, CERTCertificate *cert,
-                           SECOidTag digestAlg, SECItem *fill)

### So your intention is to complete remove function CERT_SPKDigestValueForCert ?
### Two questions:
### - if you really want to remove it completely, you should also remove it from
###   the header cert.h
### 
### but I believe you must not remove it!
###
### - there is another place in the code that seems to call this
###   function, in file 
###   mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c
###   so you probably can't remove it?

Comment 37

[Alexei Volkov]

Comment on attachment 254077 [details] [diff] [review]
patch + comments implementation (v2)

looking for two reviews out of free requests

Comment 38

[Robert Relyea]

Comment on attachment 254077 [details] [diff] [review]
patch + comments implementation (v2)

r- for 3 issues. (rest of the patch looks good to me). The nit is not required, but should be easy.

>    certs = CERT_NewCertList();
>
>    -if (certs == NULL)
>    +cert = CERT_DupCertificate(cert);
>    +if (certs == NULL || cert == NULL)
>	goto loser;

Kaie's comments about leaking cert if CERT_NewCertList fails is valid. CERT_DupCertificate just returns a reference to the original cert and is highly unlikely to fail, even if you can't get memory.

Also if CERT_AddCertToListTail fails, we leak the cert.

Best fix: Create a new variable, (CERTCertificate myCert = NULL), assign my sert at the Dup stage, change all occurances of cert to myCert in the function, free myCert at the end if it's not NULL.

>    if ((fill == NULL) || (fill->data == NULL)) {
>	result = SECITEM_AllocItem(arena, fill, digestObject->length);
>	if ( result == NULL ) {
>	   goto loser;
>	}
>	fill = result;
>   }

Again Kaie is correct, the clean up of this code needs extra work. This code is called with at lease these allocation cases:
1) arena is not null (mark and release is sufficient).
2) arena and fill are both NULL.
3) arena is null and fill is provided with a static buffer.

In addition, it may be possible that the following is also called:
3) fill is provided and arena is NULL, fill->data may are may not be set.

result will have different values depending on those inputs. If fill is provided, arena is NULL and fill->data is not NULL,
then if the Hash fails, we will try to free result, which will be equal to the static fill buffer we have passed in (bad things happen).

Also, if fill is provided with data and no arena (case 3), and digestObject->length > fill->len, then PK11_HashBuff will write passed the end of the fill buffer. We should check for this condition and return an error.

>+	SECItem *crIndex = (SECItem*)certIndex;
>	SECItem encodedName;
>
>-	encodedName = SEC_ASN1EncodeItem(NULL, NULL, certIndex,
>-					 CERT_NameTemplate);
>-         if (encodedName == NULL)
>-	    goto finish;
>-         signerCert = CERT_FindCertByName(handle, encodedName);
>-         SECITEM_FreeItem(encodedName, PR_TRUE);
>
>
>+	if (crIndex->len > 1) {        
>+	    int headerLen = 2 + ((crIndex->data[1] & 0x80) ?
>+	                          crIndex->data[1] & 0x7f : 0);
>+	    if (headerLen < crIndex->len) {
>+	 	encodedName.data  = crIndex->data + headerLen;
>+		encodedName.len = crIndex->len - headerLen;
>+		signerCert = CERT_FindCertByName(handle, &encodedName);
>+	    }
>+	}

The first set of code is creating an ASN1 data structure. The second set of code is decoding it (by hand).
It looks to me like the encode was found to be empirically wrong, and the new code was added
to match the data that was actually seen. Please figure out what the correct ASN1 template is
and use one of our many asn1 decoders (quick decoder may be a good match here). Please do not
try to decode the data by hand (NOTE, this code will fail if the Subject name we are looking for is greater then
128 bytes.... another reason to use already tested tools).


One nit... in a comment...
>  First, lets check if signer of the response is the acctual issuer

acctual -> actual


bob

Comment 39

[Alexei Volkov]

Kai, thanks for looking into the patch. I've noticed your comments to the patch after I asked bob for additional review. Please find my answer below:

> 
>      certs = CERT_NewCertList();
> -    if (certs == NULL)
> +    cert = CERT_DupCertificate(cert);
> +    if (certs == NULL || cert == NULL)
>         goto loser;

Missed the fact that new cert reference will not be "attached" to cert list
and will not be freed by CERT_DestoryCertList. This place and the other similar place in function dump_response will be fixed.


> +static SECItem *
> +ocsp_DigestValue(PRArenaPool *arena, SECOidTag digestAlg, 
> +                 SECItem *fill, SECItem *src)
> 
> ### is it possible to declare src as const?
Will gladly do so.

> +    if ((fill == NULL) || (fill->data == NULL)) {
> +       result = SECITEM_AllocItem(arena, fill, digestObject->length);
> +       if ( result == NULL ) {
> +          goto loser;
> +       }
> +       fill = result;
> +    }

This is an old code that I should have reviewed my self before using it. Will be fixed.


> 
> +    /*
> +     * Now digest the value, using the specified algorithm.
> +     */
> +    if (PK11_HashBuf(digestAlg, fill->data,
> +                     src->data, src->len) != SECSuccess) {
> 
> ### the original code in CERT_SPKDigestValueForCert (which you removed)
> ### used a series of operations on digestObject-> and you replaced
> ### it with a single call to PK11_HashBuf. I have no idea whether
> ### this is a correct change. Please ask someone who understand this
> ### code to confirm this is correct.
> 

PK11_HashBuf does exactly same thing that was done in three or more lines.
Could be a bit slower then original code. 

> -    tempItem = SEC_ASN1EncodeItem(NULL, NULL, &issuerCert->subject,
> -                                 CERT_NameTemplate);
> -    if (tempItem == NULL) {
> -       goto loser;
> -    }
> -
> -    if (SECITEM_AllocItem(arena, &(certID->issuerNameHash),
> -                         SHA1_LENGTH) == NULL) {
> -       goto loser;
> -    }
> -    rv = PK11_HashBuf(SEC_OID_SHA1, certID->issuerNameHash.data,
> -                     tempItem->data, tempItem->len);
> -    if (rv != SECSuccess) {
> +    if (cert_GetSubjectNameDigest(arena, issuerCert, SEC_OID_SHA1,
> +                                  &(certID->issuerNameHash)) == NULL) {
> 
> ### Is SEC_ASN1EncodeItem(cert->subject, CERT_NameTemplate) the same thing 
> ### as cert->derSubject (which is used in the new function) ?
Yes, that was unnecessary operation. We keep der encoded certs in db and
decode them when extracting from db using CERT_CertificateTemplate template.
This template instruct decoder to save derSubject, but decode it into "subject"
structure which is a member of CERTCertificate.

> ### Is cert->derSubject always guaranteed to be available?
> ### I do not know the answer.
> ### I'm just asking myself, because I wonder, why did the old code
> ### this additional encoding, when it could have just used derSubject?
I was also wondering about it, but unfortunately, I don't know the history on NSS well enough to answer this question.


> -       for (; signature->derCerts[certCount] != NULL; certCount++) {
> -           /* just counting */
> -           /*IMPORT CERT TO SPKI TABLE */
> -       }
> -    }
> -    rv = CERT_ImportCerts(handle, certUsage, certCount,
> +       for (; signature->derCerts[certCount] != NULL; certCount++);

Yes, it is not well readable. Will add a line.


>      if (lookupByName) {

The following code it used to avoid unnecessary encoding step. Extra precautions  steps are taken to avoid running out of the allocated buffer.
   
> +       SECItem *crIndex = (SECItem*)certIndex;
> +       SECItem encodedName;
> +       if (crIndex->len > 1) {        
> ### do you need to handle "else"?

No, because main functional load of the block is to find response issuer certificate by encoded subject name. If issuer is not found,  response verification process will fail later. If encoded structure length of subject name is less then 1 and OCSP responded instruct us to find ocspResponder cert by name, then we have problem with response encoding and we should fail verification test.

> 
> +
> +           int headerLen = 2 + ((crIndex->data[1] & 0x80) ?
> +                                 crIndex->data[1] & 0x7f : 0);
> ### Can you please explain what this does?
derResponderId is encoded as choice of encodings of subject name or key id of the issuer. First byte of excoded structure is choice metadata byte. Second defines length if length is encoded in a single byte. If length is encoded in more then one byte, first byte should have highest bit set(0x80) and the lowest seven bits define a number of bytes, following this one, that are used for length encoding.

As Bob suggested, I'll use a template to decode encoded choice. This will simplify code reading.

> ### Could you add a comment?
> ### Does this work for any item size?
> 
> +            
> +           if (headerLen < crIndex->len) {
> ### do you need to handle "else"?

Same as before. It is incorrectly encoded structure if header length is grater then the size of encoding.
> 
> +               encodedName.data  = crIndex->data + headerLen;
> +               encodedName.len = crIndex->len - headerLen;
> ### why does this trick work?
> ### Maybe you save an allocation by using this trick,
> ### but it seems the old code (that used 
> ###   certIndex = &tbsData->responderID->responderIDValue.name;
> ### ) is easier to understand.

No dough, it is easier to understand. But we already ran into a problem ones, then we tried to encode previously decoded data (see 340779). Now I change the code to use original encoding that came from responder.

> 
> 
> -    rv = CERT_VerifyCert(handle, signerCert, PR_TRUE, certUsage, checkTime,
> -                        pwArg, NULL);
> +    if (ocsp_CertIsOCSPDefaultResponder(handle, signerCert)) {
> +        rv = SECSuccess;
> +    } else {
> 
> ### I'm surprised, we really do NOT need to verify that a default OCSP 
> ### responder cert is (still) valid?

At this point, OCSP library has already checked default responder cert. It is redundant to check it on every response. 


> +    rv = CERT_VerifyCertificateNow(handle, cert, PR_TRUE,
> +                                   certificateUsageCheckAllUsages,
> +                                   NULL, &usage);
> +    if (rv != SECSuccess || (usage && (certificateUsageSSLClient |
> 
> ### are you sure you want a && here?
> ### I suspect you want the bitwise & operator?

We want to validate cert to have at least one of the following usages. We are fine if it has any of set, but we need at least one. 
> 
> 
> +                                       certificateUsageSSLServer |
> +                                       certificateUsageSSLServerWithStepUp |
> +                                       certificateUsageEmailSigner |
> +                                       certificateUsageObjectSigner |
> +                                       certificateUsageStatusResponder |
> +                                       certificateUsageSSLCA)) == 0) {
> +       PORT_SetError(SEC_ERROR_OCSP_RESPONDER_CERT_INVALID);
> 
> ### you are checking, does the cert's verified usages include at least one 
> ### from this given list of usages. 
> ### If there is no usage in common, you say it's a failure.
> ### Why is that?
> ### Why not the other usages?
> ### Could you please add a comment?
The cert that use supplies will be used as a trusted responder cert. The cert should at least have  a signing capability in order for us to use it as a trusted responder cert. Ability to sign is guarantied if cert is validated to have any set of the usages above.

> -SECItem *
> -CERT_SPKDigestValueForCert(PRArenaPool *arena, CERTCertificate *cert,
> -                           SECOidTag digestAlg, SECItem *fill)
> 
> ### So your intention is to complete remove function CERT_SPKDigestValueForCert
> ?
> ### Two questions:
> ### - if you really want to remove it completely, you should also remove it
> from
> ###   the header cert.h
> ### 
> ### but I believe you must not remove it!
> ###
> ### - there is another place in the code that seems to call this
> ###   function, in file 
> ###   mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c
> ###   so you probably can't remove it?
> 
Thx for noticing this. I'll check it out and do appropriate changes.

Comment 40

[Alexei Volkov]

(In reply to comment #36)
> (From update of attachment 254077 [details] [diff] [review])
> Please note that I did not review your changes to the tests. Do you need me to?

Yes, if you would like to. But these changes were attach to this patch by accident. The complete test patch is attached to 377288.

> CERT_SPKDigestValueForCert
> ?
> ### Two questions:
> ### - if you really want to remove it completely, you should also remove it
> from
> ###   the header cert.h
> ### 
> ### but I believe you must not remove it!
> ###
> ### - there is another place in the code that seems to call this
> ###   function, in file 
> ###   mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c
> ###   so you probably can't remove it?

This patch is for 3.11.6 only: changes need to be made in header file only. I'll modify this patch for 3.12 and get these references fixed in PKIX library.

Comment 41

[Alexei Volkov]

Attachment: patch for 3.11 branch

Patch for 3.11 branch. No changes for ocspclnt.c are needed. The patch includes changes suggested in Kay and Bob reviews.

Comment 42

[Kai Engert (:KaiE:)]

Comment on attachment 254887 [details] [diff] [review]
patch for 3.11 branch

I found at least two issues with the patch yet - not yet completed looking at the patch:

1) In ocsp_DigestValue, if a "fill" was passed to the function, you always return NULL, because you don't set "result" in that case.

2) I understand you want this patch on the 3.11 branch. But I *suspect* the NSS policy is, all new code that goes to branch must go to trunk, too. So: You are renaming function CERT_SPKDigestValueForCert to cert_GetSPKIDigest, but on the trunk there are still references to the old function name.

Comment 43

[Kai Engert (:KaiE:)]

> +    if (rv != SECSuccess || (usage && (certificateUsageSSLClient |
> ### are you sure you want a && here?
> ### I suspect you want the bitwise & operator?

Neither you nor Bob responded to my question, so I just talked to Bob in a chat. 
We both believe using a && is wrong. You want the bitwise &.


> The cert that use supplies will be used as a trusted responder cert. The cert
> should at least have  a signing capability in order for us to use it as a
> trusted responder cert. Ability to sign is guarantied if cert is validated to
> have any set of the usages above.

Thanks for the explanation.
Could you please add this as a comment to the code?

Comment 44

[Alexei Volkov]

Attachment: patch for 3.11 branch(return NULL result and &&->& fixes)

Comment 45

[Robert Relyea]

Comment on attachment 255010 [details] [diff] [review]
patch for 3.11 branch(return NULL result and &&->& fixes)

r+ = rrelyea.

I would have preferred a new variable in the ocsp_DigestValue to make it easier to understand what is going on, but this patch is correct.

bob

Comment 46

[Kai Engert (:KaiE:)]

Comment on attachment 255010 [details] [diff] [review]
patch for 3.11 branch(return NULL result and &&->& fixes)

r=kengert

Comment 47

[Alexei Volkov]

attachment 255010 [details] [diff] [review] is checked into 3.11 branch:

/cvsroot/mozilla/security/nss/cmd/lib/SECerrs.h,v  <--  SECerrs.h
new revision: 1.11.24.2; previous revision: 1.11.24.1
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.53.28.1; previous revision: 1.53
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.33.2.2; previous revision: 1.33.2.1
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.21.2.14; previous revision: 1.21.2.13
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.18.28.2; previous revision: 1.18.28.1

Comment 48

[Alexei Volkov]

Attachment: patch for 3.12

in addition to changes in attachment 255010 [details] [diff] [review] for 3.11 branch, this patch has fixes for ocspclnt.c file, the cert ref count leak, and pkix code where CERT_SPKDigestValueForCert function was renamed to CERT_GetSPKIDigest.

Comment 49

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 256088 [details] [diff] [review]
patch for 3.12

r += nelson
Please fix one spelling error.  s/guarantied/guaranteed/

Comment 50

[Alexei Volkov]

/cvsroot/mozilla/security/nss/cmd/lib/SECerrs.h,v  <--  SECerrs.h
new revision: 1.14; previous revision: 1.13
/cvsroot/mozilla/security/nss/cmd/ocspclnt/ocspclnt.c,v  <--  ocspclnt.c
new revision: 1.9; previous revision: 1.8
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.55; previous revision: 1.54
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.35; previous revision: 1.34
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.36; previous revision: 1.35
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c,v  <--  pkix_pl_ocspresponse.c
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.20; previous revision: 1.19

Leaving the bug open and re-targeting it to 3.12: need to verify that PKIX ocsp code has appropriate changes.

Comment 51

[Nelson Bolyard (seldom reads bugmail)]

Alexei, I believe we want to fix this on the 3.11 branch if possible.
Is there more work to be done  for the trunk? 
or is this now fixed on the trunk?

Comment 52

[Nelson Bolyard (seldom reads bugmail)]

P1, must have for 3.12

Comment 53

[Nelson Bolyard (seldom reads bugmail)]

I am changing the target to 3.11.7 and marking this bug resolved, 
because the patch did get committed on the 3.11 branch in time for 3.11.7.  

Unfortunately, it seems to have broken OCSP.  This is a regression.  
Apparently this wasn't tested adequately.  The test case on which the 
failure is demonstrated is to visit the page https://www.startssl.com/

The regression is due to the new call to CERT_VerifyCACertForUsage in 
CERT_VerifyOCSPResponseSignature.  The problem is: CERT_VerifyCACertForUsage 
apparently produces the wrong answer for certUsageStatusResponder.
Because CERT_VerifyCACertForUsage says the CA is not verified, we report
that the signature on the OCSP response is invalid.  

I believe calling CERT_VerifyCACertForUsage is the right thing to do.
But the function is working incorrectly for that usage.  

The error occurs at this code in CERT_VerifyCACertForUsage:

1087  	if (!isca || (cert->nsCertType & NS_CERT_TYPE_CA)) {
1088        isca = (cert->nsCertType & caCertType) ? PR_TRUE : PR_FALSE;
1089  	}

When it gets to that line, cert->nsCertType is 0x4000 and caCertTYPE IS 7
which causes the code to decide that the cert is not a valid CA for this 
usage.  caCertType got its value from CERT_KeyUsageAndTypeForCertUsage at 
line 966.

I will file a separate bug about this.

Comment 54

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 256088 [details] [diff] [review]
patch for 3.12

This patch added the following error string:

>Index: cmd/lib/SECerrs.h

>+ER3(SEC_ERROR_OCSP_RESPONDER_CERT_INVALID,  	(SEC_ERROR_BASE + 156),
>+"OCSP Trusted Responder Cert is invalid.")

I should have caught that in my review and didn't.
Saying the cert is Trusted and invalid is a contradiction.
It cannot be both.

Perhaps the word should be Delegated, instead of Trusted. (?)
Alexei, did you mean that the cert from a Delegated Responder
was invalid?  Or does this message apply to all cases where
the responder's cert is invalid?  Or does this apply only to 
user-configured OCSP responders? or ?

Comment 55

[Nelson Bolyard (seldom reads bugmail)]

Alexei tells me that this error applies only to user-configured OCSP 
responders.  The word "trusted" should be "User-configured" or 
"configured".  So I think the error message should be changed to

"Configured OCSP responder's certificate is invalid."