Closed Bug 1060929 Opened 5 years ago Closed 5 years ago

Firefox 33 can't verify a certificate chain, even though it conforms the new pkix validation "behavior changes" (bad DER encoding of default value)

Categories

(Core :: Security: PSM, defect)

33 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox31 --- unaffected
firefox32 --- unaffected
firefox33 + fixed
firefox34 + fixed
firefox35 + fixed

People

(Reporter: alexeyv, Assigned: keeler)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached file chainforfirefox.zip
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.143 Safari/537.36

Steps to reproduce:

Browse to google.com with Check Point Https Inspection turned on.

I have the chain attached, it is not a chain of actual google certificate, but the fake one used for https inspection. It is signed by an imported trusted CA. The chain conforms to the changes listed on mozpkix-testing page. It is accepted in FF31


Actual results:

The following message appeared:
"Secure Connection Failed

An error occurred during a connection to www.google.com. security library: improperly formatted DER-encoded message. (Error code: sec_error_bad_der)

    The page you are trying to view cannot be shown because the authenticity of the received data could not be verified.
    Please contact the website owners to inform them of this problem."


Expected results:

Google.com page should've appeared.
Component: Untriaged → Security: PSM
Product: Firefox → Core
What version of Firefox is this - 33 as in the Version field? You've only clarified that it works on 31...

Note that as of Firefox 32, and with expanded scopes in later versions, Firefox supports certificate pinning, so at a minimum you'd want to turn that off if you wanted to test your cert architecture on google.com. See https://wiki.mozilla.org/SecurityEngineering/Public_Key_Pinning#Implementation_status for more details.
Flags: needinfo?(alexeyv)
(also, if this breaks in 33, and works in 31, have you checked what happens in 32?)
It is 33, as in the Version field.

The same issue reproduces for all https sites that I've tried, (Facebook, for example, which isn't in the list of pinned certificates).

Isn't there a separate message for issues related to pinning?

It does work well in 32.
Flags: needinfo?(alexeyv)
(In reply to alexeyv from comment #3)
> It is 33, as in the Version field.
> 
> The same issue reproduces for all https sites that I've tried, (Facebook,
> for example, which isn't in the list of pinned certificates).

OK, so it's not that - in any case, AFAICT from that wikipage it shouldn't change anything for user-imported certificates.

> Isn't there a separate message for issues related to pinning?

I should think so - I've not checked - but it was worth making sure it didn't have anything to do with that.

> It does work well in 32.

This is good to know... it might also be helpful to check against 34 (current nightly, https://nightly.mozilla.org )...

dkeeler, could you have a look?
Flags: needinfo?(dkeeler)
34 is affected by this too.
Two extensions in the google-for-mozilla.der.cer certificate (in the attached zip) explicitly encode the default value of false for the 'critical' boolean. According to the ASN.1 specification, this is not valid for DER encodings.
Flags: needinfo?(dkeeler)
Thanks for the analysis.
Could you link me to the relevant place in specification?

As far as I understand, critical boolean is part of PKIX specification, and not ASN.1.

(Also, wasn't it checked in previous versions? Should it appear on "behavior changes" list in the wiki?)
Flags: needinfo?(dkeeler)
Sure - from ITU-T X.690 ( http://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf ) section 11.5: "The encoding of a set value or sequence value shall not include an encoding for any component value which is equal to its default value."

RFC 5280 defines Extension as:

   Extension  ::=  SEQUENCE  {
        extnID      OBJECT IDENTIFIER,
        critical    BOOLEAN DEFAULT FALSE,
        extnValue   OCTET STRING
                    -- contains the DER encoding of an ASN.1 value
                    -- corresponding to the extension type identified
                    -- by extnID
        }

So, when encoding the field "critical", if it is the default value of false, it must not be included. See also item 2 in https://wiki.mozilla.org/SecurityEngineering/mozpkix-testing#Things_for_CAs_to_Fix.

As far as I can tell, DER is not strictly enforced by NSS in general, so this wouldn't have been checked prior to Firefox using mozilla::pkix to decode certificate extensions.
Flags: needinfo?(dkeeler)
Brian, I was wondering if you had an opinion on special-casing this like we did for bug 988633 / bug 989516. Thanks.
Flags: needinfo?(brian)
(In reply to David Keeler (:keeler) [use needinfo?] from comment #9)
> Brian, I was wondering if you had an opinion on special-casing this like we
> did for bug 988633 / bug 989516. Thanks.

I think that if we're going to make any change, we should just change OptionalBoolean so that it always allows the default version to be explicitly encoded, like other implementations do.
Flags: needinfo?(brian)
Duplicate of this bug: 1065331
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Firefox Aurora can't verify a certificate chain, even though it conforms the new pkix validation "behavior changes" → Firefox 33 can't verify a certificate chain, even though it conforms the new pkix validation "behavior changes" (bad DER encoding of default value)
[Tracking Requested - why for this release]: users can't connect to websites that use these certs because of the restrictions introduced as compared with earlier verification
I can't use Firefox beta anymore in my workplace due to this issue.
Is there any plan to address this issue?
David, Brian, could you help here? I would like to have fix for this in 33. We start the build of beta 6 today.
Thanks
Flags: needinfo?(dkeeler)
Flags: needinfo?(brian)
Attached patch patchSplinter Review
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8493178 - Flags: review?(brian)
Flags: needinfo?(dkeeler)
(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> David, Brian, could you help here? I would like to have fix for this in 33.
> We start the build of beta 6 today.
> Thanks

The fix is fairly simple. We'll probably be able to get it into 33.
Attachment #8493178 - Flags: review?(brian) → review+
Flags: needinfo?(brian)
https://hg.mozilla.org/mozilla-central/rev/0c50a30b5be7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
David, could you fill the uplift request?
Flags: needinfo?(dkeeler)
Attached patch patch for betaSplinter Review
Approval Request Comment
[Feature/regressing bug #]: mozilla::pkix (enabled by default in 31, can't not use it in 33)
[User impact if declined]: Check Point TLS devices/products might not work
[Describe test coverage new/current, TBPL]: has tests
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8494604 - Flags: review+
Attachment #8494604 - Flags: approval-mozilla-beta?
Flags: needinfo?(dkeeler)
Comment on attachment 8493178 [details] [diff] [review]
patch

Approval Request Comment
see comment 21
Attachment #8493178 - Flags: approval-mozilla-aurora?
Attachment #8494604 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8493178 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.