Closed Bug 991898 Opened 6 years ago Closed 6 years ago

(mozilla::pkix) sec_error_ocsp_malformed_response when mozilla::pkix on -- April 3 Nightly Build

Categories

(Core :: Security: PSM, defect)

31 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: kwilson, Assigned: keeler)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

With OCSP hard-fail set and mozilla::pkix on (FF31.0a1, 4/3/2014), get sec_error_ocsp_malformed_response. Don't get the error when mozilla::pkix is off (FF29).
 
https://test1.a-trust.at/
https://root-ca2.test.telesec.de
(In reply to Kathleen Wilson from comment #0)
> With OCSP hard-fail set and mozilla::pkix on (FF31.0a1, 4/3/2014), get
> sec_error_ocsp_malformed_response. Don't get the error when mozilla::pkix is
> off (FF29).
>  
> https://test1.a-trust.at/

This one WFM.

> https://root-ca2.test.telesec.de

I reproduced the sec_error_ocsp_malformed_response for this one.
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla31
I was able to reproduce the problem for both sites. In both cases it looks like the responder is including a responseExtensions consisting of an empty SEQUENCE (so A2 02 30 00 - see http://tools.ietf.org/html/rfc6960#section-4.2.1 under ResponseData for reference). rfc 3280 (http://www.ietf.org/rfc/rfc3280.txt) defines Extensions as SEQUENCE SIZE (1..MAX) OF Extension, so I don't think this is a valid encoding (the proper way would be to just omit the responseExtensions in the ResponseData).
(In reply to David Keeler (:keeler) from comment #2)
> I was able to reproduce the problem for both sites.

Actually, I just rechecked and I was able to reproduce the problem for both sites too. That means there are at least two different root CAs that have this problem. We should add this to the list of things that CAs should stop doing, write a patch to allow empty Extensions, and file a follow-up bug to undo that patch in the future.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8408523 - Flags: review?(brian)
Comment on attachment 8408523 [details] [diff] [review]
patch

Review of attachment 8408523 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +508,5 @@
> +    SECItem* responseExtensions = Extensions(context);
> +    if (!responseExtensions) {
> +      return nullptr;
> +    }
> +    responseExtensionsNested = EncodeNested(context.arena,

These changes aren't strictly related to this issue - I can file a separate bug if that would be better.
Comment on attachment 8408523 [details] [diff] [review]
patch

Review of attachment 8408523 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/lib/pkixocsp.cpp
@@ +868,2 @@
>    return der::NestedOf(input, der::SEQUENCE, der::SEQUENCE,
> +                       der::MayBeEmpty, CheckExtensionForCriticality);

Nit: Perhaps we should just change the implementation of der::NestedOf to treat der::MustNotBeEmpty the same as der::MayBeEmpty? It seems like if we have a problem here we probably have a problem everywhere. Up to you.

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +508,5 @@
> +    SECItem* responseExtensions = Extensions(context);
> +    if (!responseExtensions) {
> +      return nullptr;
> +    }
> +    responseExtensionsNested = EncodeNested(context.arena,

Nit: I think it would be better to undo this change. In particular, I just rebased my patch for bug 916629 on top of your earlier changes that added Extensions() so that we can create Extensions for both certificates and OCSP responses, and IMO it is better to have this EncodeNested call in Extensions() because it is less duplicate code.
Attachment #8408523 - Flags: review?(brian) → review+
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #6) 
> Nit: Perhaps we should just change the implementation of der::NestedOf to
> treat der::MustNotBeEmpty the same as der::MayBeEmpty? It seems like if we
> have a problem here we probably have a problem everywhere. Up to you.

I would rather be conservative in the sense that if the same thing is going on elsewhere, it would be better to know so we can fix the broken implementations that are generating these encodings.

> ::: security/pkix/test/lib/pkixtestutil.cpp
> @@ +508,5 @@
> > +    SECItem* responseExtensions = Extensions(context);
> > +    if (!responseExtensions) {
> > +      return nullptr;
> > +    }
> > +    responseExtensionsNested = EncodeNested(context.arena,
> 
> Nit: I think it would be better to undo this change. In particular, I just
> rebased my patch for bug 916629 on top of your earlier changes that added
> Extensions() so that we can create Extensions for both certificates and OCSP
> responses, and IMO it is better to have this EncodeNested call in
> Extensions() because it is less duplicate code.

Ok - are Extensions in both cases wrapped by a (CONSTRUCTED | CONTEXT_SPECIFIC | 1) tag?
Thanks, Brian. Carrying over r+.
Attachment #8408523 - Attachment is obsolete: true
Attachment #8408616 - Flags: review+
(In reply to David Keeler (:keeler) from comment #7)
> I would rather be conservative in the sense that if the same thing is going
> on elsewhere, it would be better to know so we can fix the broken
> implementations that are generating these encodings.

Seems reasonable. The main issue with this approach is that we're likely to not notice a lot of incompatibilities due to our strictness for DER decoding of OCSP responses since we only really notice when the EV indicator goes away for a site that is supposedly EV.

> Ok - are Extensions in both cases wrapped by a (CONSTRUCTED |
> CONTEXT_SPECIFIC | 1) tag?

For TBSCertificate it is [3] instead of [1], but otherwise the same.
https://hg.mozilla.org/mozilla-central/rev/f2bd6f0ab761
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #3)
> We should add this to the list of things that CAs should stop doing, 

Added to https://wiki.mozilla.org/SecurityEngineering/mozpkix-testing#Things_for_CAs_to_Fix

> write a patch to allow empty Extensions, and file a follow-up bug to
> undo that patch in the future.

I cannot find the follow-up bug to undo this in the future. Did it get created?
(In reply to Kathleen Wilson from comment #12)
> I cannot find the follow-up bug to undo this in the future. Did it get
> created?

It's bug 997994.
You need to log in before you can comment on or make changes to this bug.