357197 - OCSP response code fails to match CERTIds

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Steve Parkinson]

A customer is running into interop problems with their OCSP responders.

Their OCSP responder is setting the 'parameters' field of the algorithm ID to an empty array. This is what they send:

(gdb) print *certID2
$34 = {hashAlgorithm = {algorithm = {type = siBuffer,
      data = 0x92e081d "+\016\003\002\032\004\024&#65533;q&#65533;&#65533;\021\n&#65533;&#65533;\t&#65533;1A&#65533;&&#65533;`\004<&#65533;\001\004\024&#65533;&#65533;)\214&#65533;w&#65533;\034(\bNwO&#65533;&#65533;\023\021cA3\002\003\020&#65533;\220\200", len = 5},
    parameters = {type = siBuffer, data = 0x0, len = 0}},



Our code in  is calling SECOID_CompareAlgorithmID(), and expecting to match this against what we have calculated:

(gdb) print *certID1
$33 = {hashAlgorithm = {algorithm = {type = siDEROID,
      data = 0x92d1290 "+\016\003\002\032&#65533;&#65533;&#65533;", len = 5}, parameters = {
      type = siBuffer, data = 0x92d12a0 "\005", len = 2}},

As you can see, we set 'parameters' to a 2-byte field.


The code then falls through to the switch statement below. But the function exits early because foundHash is never set:

ocsp.c: ocsp_CertIDsMatch()

 SECItem *foundHash = NULL;

...
   if (foundHash == NULL) {
        goto done;
    }
..

Comment 1

[Steve Parkinson]

The upshot of all this is that the CERT_VerifyCertificate fails with
SEC_ERROR_OCSP_UNKNOWN_CERT

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Steve, is this a duplicate of bug 234129 ?

In comment 0, you wrote:
> Our code in [invalid character] is calling SECOID_CompareAlgorithmID()

Please add comments to this bug (or to bug 234129, it this is a duplicate)
showing the call stack for the affected call to SECOID_CompareAlgorithmID.

Comment 3

[Steve Parkinson]

Attachment: fixes fallthrough case

Yes, this bug is related to bug 234129.

I have attached a patch which addresses the fall-through case. I have tested
it against a responder which does not include the algorithm parameters.
The fallthrough code does correctly match the certID, and the function returns
properly.

Comment 4

[Steve Parkinson]

Stack trace as requested:

#0  SECOID_CompareAlgorithmID (a=0x908b208, b=0x909b7c0) at secalgid.c:171
#1  0x08060bf9 in ocsp_CertIDsMatch (handle=0x9077b88, certID1=0x908b208,
    certID2=0x909b7c0) at ocsp.c:2650
#2  0x08060da7 in ocsp_GetSingleResponseForCertID (responses=0x909b198,
    handle=0x9077b88, certID=0x908b208) at ocsp.c:2718
#3  0x080619c5 in CERT_GetOCSPStatusForCertID (handle=0x9077b88,
    response=0x9092a98, certID=0x908b208, signerCert=0x909e228,
    time=1161280433498756) at ocsp.c:3457
#4  0x080618b8 in CERT_CheckOCSPStatus (handle=0x9077b88, cert=0x9084108,
    time=1161280433498756, pwArg=0x0) at ocsp.c:3406
#5  0x08065f25 in CERT_VerifyCert (handle=0x9077b88, cert=0x9084108,
    checkSig=1, certUsage=certUsageSSLClient, t=1161280433498756, wincx=0x0,
    log=0x0) at certvfy.c:1635
#6  0x0804f23b in verify_cert (out_file=0x5625c0, handle=0x9077b88,
    cert_name=0xbff90960 "testcert", cert_usage=certUsageSSLClient,
    verify_time=1161280433498756) at ocspclnt.c:453
#7  0x08050887 in main (argc=7, argv=0xbff20d74) at ocspclnt.c:1197

Comment 5

[Steve Parkinson]

One last thing - the mystery algorithm parameters that we're setting
is a SEC_ASN1_NULL put in by this function: SECOID_SetAlgorithmID()

I think this was put in to work around various bugs in other libraries
which crash if this optional param is missing.

Comment 6

[Robert Relyea]

We should check the RFC. It may say that SHA1 must have an explicit NULL parameter value for backward compatibility.

If it does then we should also notify the vendor.
In any case there should be no security issue in ignoring the parameters for those hash functions we have explicitly in the switch table.

bob

Comment 7

[Robert Relyea]

Comment on attachment 242780 [details] [diff] [review]
fixes fallthrough case

Since I suggested the fix, I think others should review it.

bob

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 242780 [details] [diff] [review]
fixes fallthrough case

>     SECOidTag hashAlg;
>     SECItem *keyHash = NULL;
>     SECItem *nameHash = NULL;
>@@ -2684,11 +2683,12 @@ ocsp_CertIDsMatch(CERTCertDBHandle *hand
> 	nameHash = &certID1->issuerMD2NameHash;
> 	break;
>     default:
>-	foundHash = NULL;
>+	keyHash = NULL;
>+	nameHash = NULL;

keyHash and nameHash are already NULL here, so I see no reason to set 
them to NULL again.  Why not just return PR_FALSE Here?

> 	break;
>     }
> 
>-    if (foundHash == NULL) {
>+    if (keyHash == NULL || nameHash == NULL) {

In the above switch cases, the values assignd to keyHash and nameHash
CANNOT be NULL, except in the default case where both will be null.
For example, Even if certID1 is NULL, &certID1->issuerMD2NameHash is not.
So testing both for NULL seems pointless.  Testing one is sufficient.
But why not goto done, or return PR_FALSE in the default case above?

> 	goto done;
>     }
>     PORT_Assert(keyHash && nameHash);

This old assert is silly now and should go.

Final question.  Do we care about other hashes, such as "SHA 2" hashes?

Comment 9

[Steve Parkinson]

Attachment: New patch addressing Nelson's comments

Thanks for your comments. This new patch gets rid of default case and the assertion.

Comment 10

[Wan-Teh Chang]

Comment on attachment 242890 [details] [diff] [review]
New patch addressing Nelson's comments

Steve, in NSS it's okay to use goto statements for
error exits.

Therefore, I suggest that you just change

    if (foundHash == NULL) {
	goto done;
    }

to

    if (keyHash == NULL) {
	goto done;
    }

You can keep a default case that does nothing but
a break statement.  (Some coding styles require
every switch statement to have a default case.)

Please ask Julien "Mr. OCSP" to review new patches.
Someone should also look into the NULL parameters
issue.

The switch statement doesn't need to handle the
SHA-2 algorithms because struct CERTOCSPCertIDStr
doesn't have any members for those hash algorithms.

Comment 11

[Wan-Teh Chang]

*** This bug has been marked as a duplicate of 234129 ***

Comment 12

[Robert Relyea]

Since the patch and the action are in this bug, Let's close the old one as a dup.

Also the target for this should be 3.11.4

Comment 13

[Robert Relyea]

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

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Bug 234129 should not be BOTH a duplicate of this bug, and a dependent of 
this bug.  So I'm remving the dependency.  Hopefully, as a result, bugzilla
will stop sending me so many duplicate emails. :)

Comment 15

[Steve Parkinson]

Attachment: Additional parens around expression

In reference to wtc's comment 10, I think my patch is cleaner than having the goto: in there, so I would rather keep it as-is.

I did incorporate another of wtc's suggestions - adding parens to match the style of the rest of the expression.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 242928 [details] [diff] [review]
Additional parens around expression

r=nelson

Comment 17

[Julien Pierre]

RFC2560 doesn't specify the format of the parameters for AlgorithmIdentifier. It refers to RFC2459 which was superceded by RFC3280. 

RFC3280 defines AlgorithmIdentifier as :

AlgorithmIdentifier  ::=  SEQUENCE  {
     algorithm               OBJECT IDENTIFIER,
     parameters              ANY DEFINED BY algorithm OPTIONAL  }
                                -- contains a value of the type
                                -- registered for use with the
                                -- algorithm object identifier value

Unfortunately, RFC3280 doesn't go into more specifics about the parameters value either. It refers to RFC3279 for that . It speaks about hash functions MD2, MD5 and SHA-1 only. But it doesn't speak about the format of the parameters at all for these; whether it should be left out or a NULL value. My reading is that nothing should be encoded because the RFC is completely silent about the value, and the component is optional. It's possible someone else made a different reading and decided to encode a NULL. But I couldn't find anything that supported encoding a NULL value here.

RFC3279 speaks at length about signature algorithms (hash with encryption) and the format of the parameters for those algorithms. But that's not what we are concerned with here.

So, IMO, the responder is in error in encoding a NULL, and it's a bug on their part to encode it, not a bug on NSS' part to reject the OCSP response.

Comment 18

[Steve Parkinson]

Julien - you got it backwards, NSS is encoding a NULL (in SECOID_SetAlgorithmID). The responder is not.

Another fix for this would be to change SECOID_SetAlgorithmID to not encode the parameters, but that will have ramifications throughout every protocol NSS supports. We'd be trading the interop problem mentioned in this bug for potentially many more. If you feel strongly about that, I would suggest opening a different bug, because it affects so much more than just OCSP.

Can we treat this bug about being more flexible with what NSS accepts, for OCSP only?

Comment 19

[Robert Relyea]

So just to be clear, the responder encoding NULL is the only one that currently works.

I know that SHA-1 has some weirdness where the natural reading would lead one to believe that the parameters should not exist, but many implementations has an explicit NULL (I believe we had lots of problems with signatures going back to when I first joined the group).

So the upshot, I think, is that there is nothing wrong with the empty parameters, and there is nothing  else to do beyond fixing the NSS bug where it doesn't accept  empty parameters for SHA1.

Comment 20

[Robert Relyea]

Steve, I think we should continue to encode the NULL on sending. That has become the defacto standard for the best compatibility. 

bob

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

The whole NULL-vs-empty debate was going on just before I joined Netscape in
September '96.  I wish I had some document(s) to cite for the decisions, but
I've never seen any (to my knowledge).

Comment 22

[Wan-Teh Chang]

We can handle the algorithm parameters similar to the way
the callers of SGN_DecodeDigestInfo handle that field.
http://lxr.mozilla.org/security/ident?i=SGN_DecodeDigestInfo

Replace the SECOID_CompareAlgorithmID(...) == SECEqual test by 

  (SECITEM_CompareItem(&certID1->hashAlgorithm.algorithm,
      &certID2->hashAlgorithm.algorithm) == SECEqual) &&
      (certID2->hashAlgorithm.algorithm.parameters.len <= 2)

You can also specifically test for a zero-length or NULL
(SEC_ASN1_NULL) 'parameters'.

Comment 23

[Rob Crittenden]

The latest patch was tested with an otherwise vanilla NSS 3.11.3 and mod_nss (tip) on Solaris 9 and the back OCSP behavior went away.

Comment 24

[Wan-Teh Chang]

Attachment: Fix hash algorithm identifier comparison

Steve, Rob, could you please test this patch, *without* Steve's
patch (attachment 242928 [details] [diff] [review])?  Thanks.

Comment 25

[Rob Crittenden]

The hash algorithm patch works as well.

Comment 26

[Wan-Teh Chang]

Comment on attachment 242928 [details] [diff] [review]
Additional parens around expression

I checked in Steve's patch on the NSS trunk (NSS 3.12) and the
NSS_3_11_BRANCH (NSS 3.11.4).

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

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

Comment 27

[Wan-Teh Chang]

Comment on attachment 243708 [details] [diff] [review]
Fix hash algorithm identifier comparison

Let me know if you want me to not test certID1->hashAlgorithm.parameters,
which comes from NSS and therefore is known to be NULL (two bytes 0x05 0x00).

I can also define a macro for the certID2->hashAlgorithm.parameters.len <= 2
test so that we can use it in lib/softoken/pkcs11c.c and lib/cryptohi/secvfy.c
as well (search for "> 2" in those two files).  Something like this:

  /*
   * For all the supported hash algorithms, 'parameters' is NULL (two
   * bytes 0x05 0x00), but we allow it to be missing (zero length).
   */
  #define IS_VALID_HASH_ALG_PARAMS(params) ((params)->len <= 2)

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 243708 [details] [diff] [review]
Fix hash algorithm identifier comparison

r=nelson

Comment 29

[Wan-Teh Chang]

Attachment: Fix hash algorithm identifier comparison (more strict)

I found that we never examine certID2->hashAlgorithm.parameters,
other than comparing it with certID1->hashAlgorithm.parameters.
It seems that we should reject certID2 if certID2->hashAlgorithm.parameters
is bogus.  This more strict patch does that.  Please let me know
if you prefer this patch to the previous patch (attachment 243708 [details] [diff] [review]).
With the previous patch, certID2 with a bogus hashAlgorithm.parameters
still gets a second chance in the fallthrough code (the switch
statement).

Comment 30

[Robert Relyea]

Comment on attachment 243708 [details] [diff] [review]
Fix hash algorithm identifier comparison

r+=relyea

Comment 31

[Wan-Teh Chang]

Comment on attachment 243708 [details] [diff] [review]
Fix hash algorithm identifier comparison

I checked in this patch "fix hash algorithm identifier comparison"
on the NSS trunk (NSS 3.12) and the NSS_3_11_BRANCH (NSS 3.11.5).

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

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

Comment 32

[Wan-Teh Chang]

Attachment: Reject hash algorithm identifier with bogus 'parameters'

I explained this patch in comment 29.  I think we should reject
a hash algorithm identifier that contains a bogus 'parameters', not
allowing it to fall through to the switch statement below.  Agree?

Recall that the switch statement didn't work before.  Now that Steve's
patch fixed it, we should also check in this patch to match the previous
behavior (hash algid with bogus parameters didn't work before).

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 247616 [details] [diff] [review]
Reject hash algorithm identifier with bogus 'parameters'

On its surface, function ocsp_CertIDsMatch appears to take two similar 
objects and compare them.  The names of the two CertID arguments do not 
suggest that one is fundamentally different from the other, and the block 
comment also does not suggest it.

But the code treats the two CertID argument values asymmetrically.
The code "knows" that certID1 is a value on which we have previously 
computed 3 different hashes, and that certID2 is a value returned 
in an OCSP response, and contains only one remotely computed hash value.  
So, it finds the locally computed hash value for certID1 that was computed 
using the same hash used in certID2, and compares those two values.  

This patch further increases the asymmetry.  It performs a validity 
check only on certID2, that it does not also perform on certID1. 
This is apparently due to a (probably reasonable) assumption that we 
properly encoded the hash parameter in certID1.  

I think that, at a minimum, there should be a block comment explaining
this, so that future readers of this code are given some explanation 
for the asymmetric treatment.  I would go further and suggest that the
two pointers be renamed from certID1 and certID2 to "local" and "remote"
or "request" and "response", (or "reference" and "suspected" :) to 
further clarify this difference.  

I am a little concerned that this patch will cause failures to occur in
cases where they previously were not occuring, and may be seen as a 
failure of backwards binary compatibility.  If some broken OCSP responder
is sending out bad parameters, we might have been blissfully ignoring
them before, and now we will fail.  We will have to explain to customers
that our product doesn't work any more because of a problem that some
other product has had all along, and the question will be why did we 
break backwards compatibility.  I am weary of those questions.  
Unless we can show that there is a vulnerability being actively thwarted
by this change, I am reluctant to approve it.

Comment 34

[Wan-Teh Chang]

Attachment: Reject hash algorithm identifier with bogus 'parameters' v2

Nelson, I improved the comment and renamed the certID1 and certID2
parameters as you suggested to make the asymmetry between the two
parameters clear.  The longer parameter names required me to reformat
some code to wrap long lines.

This patch does not break backward compatibility because a response
certID with bogus hash algorithm parameters never worked before.
Let me explain this subtle point.  In NSS releases <= 3.11.3, such
a response certID

  - fails the binary comparison with the request certID
  - falls through to the broken switch statement, and fails again

Steve's hot fix fixed the broken switch statement, so in NSS 3.11.4, it

  - fails the binary comparison with the request certID
  - falls through to the switch statement, and succeeds

This patch will make it fail again.  It will

  - fail the "parameters" test
  - NOT fall through to the switch statement

So this patch only breaks backward compatibility with NSS 3.11.4.
It is backward compatible with all NSS releases <= 3.11.3.

Comment 35

[Wan-Teh Chang]

Just to make one point clear -- Steve's hot fix allows a response
certID with *missing* hash algorithm parameters, but it has the subtle,
unintended side effect of also allowing a response certID with *bogus*
hash algorithm parameters.  Since this side effect is unintended, we
should remove it.  I shouldn't need to show it's a vulnerability.

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 247920 [details] [diff] [review]
Reject hash algorithm identifier with bogus 'parameters' v2

r=nelson
Thanks, Wan-Teh.  
This is a big improvement!

Comment 37

[Wan-Teh Chang]

Comment on attachment 247920 [details] [diff] [review]
Reject hash algorithm identifier with bogus 'parameters' v2

I checked in the patch "reject hash algorithm identifier with
bogus 'parameters' v2" on the NSS trunk (NSS 3.12).

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

Comment 38

[Robert Relyea]

Comment on attachment 247920 [details] [diff] [review]
Reject hash algorithm identifier with bogus 'parameters' v2

r+ = relyea.

I'm a little worried about the hardcoded param > 2. It's fine for all existing hash algorithms, but future ones could include valid parameters which are greater than 2 (I can't conceive of one right now, though).

The patch is OK, though because a) it's no worse than the existing code it corrects, and b) we would need to rework this section of OCSP if we need to support hashes otther than MD5, MD2 or SHA1 (supporting SHA256 and SHA512 would require changes to the OSCP code, even though NSS already supports these hashes, for example).

bob

Comment 39

[Wan-Teh Chang]

The hardcoded param > 2 was copied from existing code in
lib/cryptohi/secvfy.c and lib/softoken/pkcs11c.c.

I checked in the patch "reject hash algorithm identifier with
bogus 'parameters' v2" on the NSS_3_11_BRANCH (NSS 3.11.5).

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