342461 - verify signature on an OCSP response without intermediate decoding and encoding

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P3)

Description

[Alexei Volkov]

Intermediate decoding and encoding of a response can potentially create problem due to ASN1 encoder/decoder incompatibility or errors on either peer's sides. 

Check bug 340776 for more info.

Comment 1

[Alexei Volkov]

Attachment: skip reencoding of response structure

Instead extract response TBS from response DER and verify it's signature.

Comment 2

[Alexei Volkov]

Attachment: decode tbs der using SEC_ASN1_SAVE flag (checked in)

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 245053 [details] [diff] [review]
decode tbs der using SEC_ASN1_SAVE flag (checked in)

r=nelson

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 244837 [details] [diff] [review]
skip reencoding of response structure 

I prefer the second patch.

Comment 5

[Alexei Volkov]

/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.33; previous revision: 1.32
/cvsroot/mozilla/security/nss/lib/certhigh/ocspti.h,v  <--  ocspti.h
new revision: 1.6; previous revision: 1.5

Comment 6

[Alexei Volkov]

Comment on attachment 245053 [details] [diff] [review]
decode tbs der using SEC_ASN1_SAVE flag (checked in)

Propose this bug fix for 3.11.5: It fixes the reason of escalation we had 9 month ago from the department of defense. See bug 340776. They had openssl based responder, that was sending us incorrectly encoded response. We choke on verifying response signature because we re-encoded response before calculating its hash.

Reopen the bug. Asking Wan-teh for this patch super review.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

This is a case where we were not being "generous in what you accept".
We were receiving a message with (IIRC) an optional item that was being 
encoded with zero length, rather than being omitted.  That isn't strictly 
correct, but we didn't have any real trouble decoding it or understanding it.
The problem was that, before verifying the signature, we were re-encoding the
message from its decoded values, and our reencoding omitted the zero length
optional part, producing a correct result that didn't match the original,
so the signature test failed.  

The answer is to stop re-encoding the decoded message and instead use the 
original undecoded message in the signature test.  That's what this fix does.
I would like to see us get this fix into 3.11.5.

Comment 8

[Wan-Teh Chang]

Comment on attachment 245053 [details] [diff] [review]
decode tbs der using SEC_ASN1_SAVE flag (checked in)

r=wtc.  Good patch.  I have some suggested changes.

> static ocspResponseData *
>-ocsp_GetResponseData(CERTOCSPResponse *response)
>+ocsp_GetResponseData(CERTOCSPResponse *response, SECItem **tbsResponseDataDER)

It would be nice to document the new argument tbsResponseDataDER in
the block comment before this function.

I'd probably use the type SECItem * for tbsResponseDataDER.

>+    if (tbsResponseDataDER) {
>+        *tbsResponseDataDER = &basic->tbsResponseDataDER;
>+    }

I wonder if we should add these assertions after the assignment:
          PORT_Assert((*tbsResponseDataDER)->data != NULL);
          PORT_Assert((*tbsResponseDataDER)->len != 0);
since this function has a lot of assertions.

>+ocsp_CheckSignature(ocspSignature *signature, SECItem *encodedTBS,
> 		    const SEC_ASN1Template *encodeTemplate,

Please remove the now unused argument encodeTemplate.

> struct ocspBasicOCSPResponseStr {
>     ocspResponseData *tbsResponseData;	/* "tbs" == To Be Signed */
>     ocspSignature responseSignature;
>+    SECItem tbsResponseDataDER;
> };

It would be better to make tbsResponseDataDER the first member of the
structure because tbsResponseDataDER is before tbsResponseData in
ocsp_BasicOCSPResponseTemplate.  Alternatively, can we move
    { SEC_ASN1_ANY | SEC_ASN1_SAVE,
	offsetof(ocspBasicOCSPResponse, tbsResponseDataDER) },
to the end of ocsp_BasicOCSPResponseTemplate?

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 245053 [details] [diff] [review]
decode tbs der using SEC_ASN1_SAVE flag (checked in)

Alexei committed this patch on the trunk, and Wan-Teh committed it on the 3.11 branch. 
I think we should consider making additional changes in response to Wan-Teh's review comments, so I will leave this bug open for now.

Comment 10

[Wan-Teh Chang]

Nelson, I didn't check in any patch in this bug.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh wrote:
> Nelson, I didn't check in any patch in this bug.

Hmm.  I see now that you checked in a patch for a different bug,
but the cvs log message for your checkin was completely identical 
to Alexei's checkin message on the trunk.  So, judging solely on the 
basis of the checkin message, it appeared that you checked Alexei's 
patch on the branch.

Alexei, please commit this fix on the branch, too, since it is
targetted at 3.11.5 and has two reviews.

Comment 12

[Wan-Teh Chang]

Nelson, yes, I checked in a patch for a different bug with the
wrong CVS log message referencing this bug.  Sorry.  I have just
backed out that checkin and checked it in again with the right
CVS log message.

Comment 13

[Alexei Volkov]

Attachment: additional fix(review comments) to the integrated patch

Comment 14

[Alexei Volkov]

Integrating changes suggested by Wan-Teh(attachment 250925 [details] [diff] [review]):
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.34; previous revision: 1.33
/cvsroot/mozilla/security/nss/lib/certhigh/ocspti.h,v  <--  ocspti.h
new revision: 1.7; previous revision: 1.6

Comment 15

[Alexei Volkov]

Attachment: fix assertions location

/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.35; previous revision: 1.34

Comment 16

[Alexei Volkov]

committing to the branch:

/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.21.2.13; previous revision: 1.21.2.12
/cvsroot/mozilla/security/nss/lib/certhigh/ocspti.h,v  <--  ocspti.h
new revision: 1.5.28.1; previous revision: 1.5

Comment 17

[Alexei Volkov]

libpkix has the same problem of re-encoding the responseDataTBS structure. See http://lxr.mozilla.org/security/source/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c#1190

Reopen and retargeting to 3.12.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Wait.  If this is fixed in 3.11.6 then it should remain targetted at 3.11.6 
and remain resolved fixed, so that people can find that it was fixed in that
release.  Probably should file a separate bug for the libPKIX code for 3.12.

Comment 19

[Alexei Volkov]

bug 373367 created as a dup for this one targeted to be fixed in pkix library.