381317 - Unauthorized OCSP response error

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

The checkin for Bug #338986 seems to have caused a regression in NSS 3.11.7 
It seems to have broken OCSP.  Apparently it wasn't tested adequately.  

A visit to https://www.startssl.com/ now gets error sec_error_ocsp_invalid_signing_cert

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.  

This is partly my fault.  Using CERT_VerifyCACertForUsage was my idea.
I believe calling CERT_VerifyCACertForUsage is the right thing to do.
But the function is working incorrectly for that usage.  That may be 
my fault, too.  It now appears that we have no QA test for this function.

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

We may want to respin 3.11.7 for this, if we can fix it quickly.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Julien, please double-check my analysis to confirm or refute that it is 
wrong for NSS to report this error for this test case.  I would be happy
to learn that NSS is properly diagnosing this error, but I don't think so.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Here is info on the server cert chain and the OCSP responder's cert.

SSL server cert:
Subject: "E=[e-mail],CN=*.startssl.com,OU=StartCom Trus
    ted Certificate Member,O=StartCom Ltd.,L=Eilat,C=IL"
Issuer: "E=[e-mail],CN=StartCom Class 3 Primary Intermediat
    e Free CA,OU=Secure Certificate Signing,O=StartCom Ltd.,ST=Israel
    ,C=IL"

Intermediate CA cert:
Subject: "E=[e-mail],CN=StartCom Class 3 Primary Intermedia
    te Free CA,OU=Secure Certificate Signing,O=StartCom Ltd.,ST=Israe
    l,C=IL"
Issuer: "E=[e-mail],CN=Free SSL Certification Authority,OU=
    CA Authority Dep.,O=StartCom Ltd.,L=Eilat,ST=Israel,C=IL"

Root:
Subject: "E=[e-mail],CN=Free SSL Certification Authority,OU
    =CA Authority Dep.,O=StartCom Ltd.,L=Eilat,ST=Israel,C=IL"
Issuer: "E=[e-mail],CN=Free SSL Certification Authority,OU=
    CA Authority Dep.,O=StartCom Ltd.,L=Eilat,ST=Israel,C=IL"

OCSP Responder:
Subject: "E=[e-mail],CN=StartCom Class3 Revocation Authorit
    y,O=StartCom Ltd.,C=IL"
Issuer: "E=[e-mail],CN=StartCom Class 3 Primary Intermediat
    e Free CA,OU=Secure Certificate Signing,O=StartCom Ltd.,ST=Israel
    ,C=IL"
Signed Extensions:
    Name: Certificate Basic Constraints
        Critical: True
        Data: Is a CA with no maximum path length.
    Name: Certificate Key Usage
        Usages: Digital Signature
                Non-Repudiation
                Key Encipherment
                Key Agreement
                Certificate Signing
                CRL Signing
    Name: Extended Key Usage
        OCSP Responder Certificate
        OCSP No Check Extension

So, we see that the OCSP responder's cert was issued by the CA that 
issued the SSL server's cert.  It is a delegated responder.  It has
the special EKU extension OID for delegated OCSP responderes, as it should.
The only thing unexpected about it is that it is a CA cert.  
Perhaps this is the first case we've encountered in which the OCSP responder
is a CA *AND* is a delegated responder, and not the issuer itself.

So, I'm convinced this is a bug.  Alexei and I need to jump on this Monday.
I have a patch forthcoming that seems to solve the immediate problem, but 
I fear that our OCSP logic for deciding if a responder is either:
(a) the cert's issuer, or (b) delegated by the cert's issuer or 
(c) the user's designated OCCSP responder, remains flawed, even though 
that was the whole point of Bug 338986 and Bug 58507 . :-(

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

This patch seems to fix the immediate problem.
It corrects the required cert type (EKU) for OCSP responders that area CAs
It may, however, not be the entire fix for this bug.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

In function CERT_VerifyOCSPResponseSignature, in file ocsp.c, we see the 
following code for deciding if the signer of the response is qualified.

3723     if (ocsp_CertIsOCSPDefaultResponder(handle, signerCert)) {
3724         rv = SECSuccess;
3725     } else {
3726         if (CERT_IsCACert(signerCert, NULL)) {
3727             rv = CERT_VerifyCACertForUsage(handle, signerCert, PR_TRUE,
3728                                            certUsageStatusResponder,
3729                                            producedAt, pwArg, NULL);
3730         } else {
3731             rv = CERT_VerifyCert(handle, signerCert, PR_TRUE,
3732                                  certUsageStatusResponder,
3733                                  producedAt, pwArg, NULL);
3734         }

What seems missing there is the logic that checks (when the responder is
not the user's designated "default" responder) to see that the responder
is either (a) the cert's issuer or (b) delegated by the cert's issuer.

I traced through ocspclnt -S , and also through vfyserv -o, and did not
find any logic (as I traced through) that seemed to check that the responder
either was the cert's issuer or its delegate.  This bug won't be fixed 
until it's clear that those checks are being done, and correctly.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

I now believe this patch is good enough for 3.11.7, but bigger changes
are needed for 3.12.  

Comment 7

[Kai Engert [:KaiE:]]

Do you propose to take this patch at the last minute for 3.11.7?

It would be good to ship 3.11.7 with the regression fixed, if we plan to deliver 3.11.7 into Firefox products. (But we can always do a 3.11.8 quickly afterwards.)

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

> Do you propose to take this patch at the last minute for 3.11.7?

Yes, I plan to fix this and produce another 3.11.7 Release Candidate, ASAP.
However, the next RC may not be available until next week.

Comment 9

[Alexei Volkov]

Attachment: another way of fixing it

proposing to add another type of CA for the purpose of identifying a CA with designated responder extended key usage.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 265708 [details] [diff] [review]
another way of fixing it

I don't think we have any need or desire to create separately specifiable
usages for delegated OCSP responders that are CAs vs those that are EEs.
In my view, EXT_KEY_USAGE_STATUS_RESPONDER simply needs to be implemented
correctly in both CERT_VerifyCert* and in CERT_VerifyCACert*

Perhaps I am missing some other important purpose/benefit of this 
proposal?

Comment 11

[Alexei Volkov]

In my opinion adding EXT_KEY_USAGE_STATUS_RESPONDER_CA straits things out. First, it is wrong to define NS_CERT_TYPE_CA to have EXT_KEY_USAGE_STATUS_RESPONDER, because there is a possibility to have designated responder cert that is not a CA. 
Second, cert_GetCertType checks if a cert has basicConstraint extension. I think been CA is important property of a cert and we should not disregard it.  

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 265465 [details] [diff] [review]
patch v1

I erred in suggesting that CERT_VerifyOCSPResponseSignature should call 
CERT_VerifyCACertForUsage instead of CERT_VerifyCert* when the responder's 
cert is a CA cert.  

CERT_VerifyCACertForUsage answers the wrong question.
The question we're trying to ask is: Is this cert (the responder's cert) 
a valid OCSP responder.  We're asking if the responder cert is valid to 
sign the response itself.  
That's not the question that CERT_VerifyCACertForUsage answers.

CERT_VerifyCACertForUsage answers the question: Is this CA cert a 
legitimate issuer of other certs that can be used for the specified purpose?  
CERT_VerifyCACertForUsage never asks if the CA cert can actually do the 
specified operation itself.  It asks if the CA can issue another cert 
that would be OK to do the specified operation.

The reason that I suggested using CERT_VerifyCACertForUsage, is that I 
thought CERT_VerifyCert* has a problem validating CA certs for purposes 
that we commonly regard as EE purposes.  I thought that CERT_VerifyCert* 
would always say: "No, this CA cert is not valid to sign OCSP responses."

Maybe CERT_VerifyCert* can evaluate the question of if a cert is valid 
to be an OCSP responder, whether the cert is a CA cert or an EE cert.
If that is so, then the better patch is just to always call 
CERT_VerifyCert* and never CERT_VerifyCACertForUsage.

I'm going to obsolete my own patch and give an explanatory comment.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 265708 [details] [diff] [review]
another way of fixing it

As I understand it, Alexei was concerned that NSS might decide a cert is 
a CA cert when it is merely a designated OCSP responder EE cert, because
NS_CERT_TYPE_CA has been defined to include EXT_KEY_USAGE_STATUS_RESPONDER.
I think that is a valid concern.  (I don't know if it's a real problem in
NSS, but it would be an error to trust an EE cert as a CA cert on that 
basis.)  I think it should be pursued for the next release of NSS in a 
separate bug.
 
But this patch isn't the right answer, because the real problem is that
CERT_VerifyOCSPResponseSignature calls CERT_VerifyCACertForUsage, which 
is my fault.

I will shortly attach a new patch that implements a solution that should 
in no case be worse than how NSS 3.11.6 behaves.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: don't call CERT_VerifyCACertForUsage

I think this is the right patch.  It makes the code work very much like
it did in 3.11.6, calling only CERT_VerifyCert* for the validation.
Please review ASAP.
We have to get this into a nighly build and through a nightly QA cycle
ASAP.

Comment 15

[Alexei Volkov]

Comment on attachment 265881 [details] [diff] [review]
don't call CERT_VerifyCACertForUsage

will fail to validate responses that was signed by issuer of a cert in question.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 265881 [details] [diff] [review]
don't call CERT_VerifyCACertForUsage

Agreed, this needs more study

Comment 17

[Alexei Volkov]

Attachment: regression resolution

all ocsp tests passed. Idea is provided by Nelson

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 266013 [details] [diff] [review]
regression resolution

r+ on the strength of the testing that Alexei did.  
Thanks, Alexei.

Comment 19

[Julien Pierre]

Comment on attachment 266013 [details] [diff] [review]
regression resolution

This looks OK, but we still need to fix CERT_VerifyCert for the certUsageStatusResponder case. It currently will pass even if a cert in the chain is explicitly untrusted. That should be a separate bug.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

>  It currently will pass even if a cert in the chain is explicitly untrusted.

Not sure what you mean by that.  Did you mean "the chain lacks a trusted cert"?

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Marking fixed.  
We need to file a separate bug about the certUsageStatusResponder issue.

Comment 22

[Kai Engert [:KaiE:]]

Attachment: Backport to NSS 3.11.5

This is an attempt to backport the fix to the 3.11.5 snapshot.
Not yet sure whether we'll have a way to use it, but when I applied this patch locally to a Firefox 2 with NSS 3.11.5, it fixed the test site mentioned in bug 419030 (https://ocsptest.comodoca.com).

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Kai, where do you propose to check this in? 
On the NSS_3_11_BRANCH for the next sequential NSS 3.11.x release?
I could support that.

But I object strenuously to any attempt to create yet another NSS branch.
I will not support another branch.  I think I speak for Sun. 
If this is an attempt to create 3.11.5.x, I do not and will not support it.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 318280 [details] [diff] [review]
Backport to NSS 3.11.5

We don't want to backport to 3.11.5.  We MAY want to backport to 3.11. branch 

If this patch also applies to 3.11.9 branch,  ask for review for it again. Thanks.