Closed
Bug 991898
Opened 10 years ago
Closed 10 years ago
(mozilla::pkix) sec_error_ocsp_malformed_response when mozilla::pkix on -- April 3 Nightly Build
Categories
(Core :: Security: PSM, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: kwilson, Assigned: keeler)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
11.39 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•10 years ago
|
||
(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.
Blocks: mozilla::pkix-beta
Updated•10 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla31
Assignee | ||
Comment 2•10 years ago
|
||
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).
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
(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?
Assignee | ||
Comment 8•10 years ago
|
||
Thanks, Brian. Carrying over r+.
Attachment #8408523 -
Attachment is obsolete: true
Attachment #8408616 -
Flags: review+
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2bd6f0ab761
Comment 10•10 years ago
|
||
(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.
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f2bd6f0ab761
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Reporter | ||
Comment 12•10 years ago
|
||
(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?
Assignee | ||
Comment 13•10 years ago
|
||
(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.
Description
•