1661163 - New firefox cert details viewer is unable to view a cert

Status: NEW

(Firefox :: Security, defect, P5)

Description

[clark.boylan]

Attachment: limestone_cacert.pem

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:80.0) Gecko/20100101 Firefox/80.0

Steps to reproduce:

Visit https://osa.continuous.pw:5000/ which results in "Warning: Potential Security Risk Ahead" as this is a self signed cert. Next click on "Advanced" then "View Certificate".

I'm attaching the cert in case the cert at that destination changes and no longer reproduces this result.

Actual results:

Firefox opens a new page that says:

Something went wrong.

We were unable to find the certificate information, or the certificate is corrupted. Please try again.

Expected results:

I expected to get the new cert viewer with the cert details.

Chrome is ok with this cert as is openssl s_client. One oddity I notice is there is no OU and maybe the new viewer expects that to be present?

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a7180808eaaeb815467d7794477f69a71b62a93b&tochange=b01aab238aa0293e21f204be90eea391e35c71ec

Suspect:
c407e5da6487773b8a4a3585676ee9f047a0698f Carolina — Bug 1580455 - Uses normalizeToKebabCase in adjustCertInformation for the result labels.r=johannh,fluent-reviewers,flod

Comment 3

[:Gijs (he/him)]

I'm not sure if Carolina is still around...

Mark, you expressed interest in looking at some Firefox bugs again, maybe you could have a look? It would seem that we're failing to parse this because the x509 instance we create at https://searchfox.org/mozilla-central/rev/ce21a13035623c1d349980057d09000e70669802/toolkit/components/certviewer/content/certDecoder.js#475 in its subject.typesAndValues array has an entry for the SAN. This appears to be the result of the certificate's distinguished names using the subject alt names extension, and our localization structure for that extension is such ( https://searchfox.org/mozilla-central/rev/ce21a13035623c1d349980057d09000e70669802/toolkit/components/certviewer/content/strings.js#310-315 ) that the code trying to use it ( https://searchfox.org/mozilla-central/rev/ce21a13035623c1d349980057d09000e70669802/toolkit/components/certviewer/content/certDecoder.js#106-125 ) fails, because there's an extra level of nesting (for whatever reason) - ie the object it's getting is {name: {short: "...", long: "..."}} instead of {short: "...", long: "..."}. I imagine the simplest fix would be to remove the additional level of nesting? It's not clear to me why it's there...

I expect we haven't seen this before because it may be unusual for the SAN to be included in the DN? But I don't actually know all that much about the gory details of x509 (and would sort of like to keep it that way...).

Comment 4

[:Gijs (he/him)]

(prior to the regressing bug, I expect we would have displayed the SAN in the DN section without a heading/label for that row -- nothing tried to do much with the undefined value we'd get for the "name" of the extension, and that'd be that.)

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I don't think that's a certificate that we need to handle. RFC 5280 mandates that implementations handle a limited set of X.520 attribute types in the issuer/subject distinguished names of certificates, but an attribute with OID 2.5.29.17 (id-ce-subjectAltName) is not one of them.

Comment 6

[:Gijs (he/him)]

Dana and I discussed briefly - though we don't need to handle the cert, it does appear like the // extensions strings in strings.js are unused and should be removed, so that they don't interfere with the DN parsing code, which will then just dump OID things as labels for identifiers it doesn't recognize.

(In reply to clark.boylan from comment #0)

Chrome is ok with this cert

FWIW, neither Dana and I see this - we get an error page in Chrome with no way to bypass. Can't speak for Dana but mine includes text (under the "advanced" button) that says "You cannot visit osa.continuous.pw at the moment because the website sent scrambled credentials that Google Chrome cannot process."

Comment 7

[clark.boylan]

Attachment: chrome_works_for_me.png

I'm running Chrome on an reasonably up to date openSuse Tumbleweed install and it works for me here. Perhaps a difference in tls libraries?

Comment 8

[clark.boylan]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #5)

I don't think that's a certificate that we need to handle. RFC 5280 mandates that implementations handle a limited set of X.520 attribute types in the issuer/subject distinguished names of certificates, but an attribute with OID 2.5.29.17 (id-ce-subjectAltName) is not one of them.

I won't claim to be an expert on any of this but RFC 5280 also says "As noted above, distinguished names are composed of attributes. This specification does not restrict the set of attribute types that may appear in names." That implies to me that while odd the SAN in the issuer DN isn't strictly prohibited? Not responding with an error would be nice for users if that is the case.

Comment 9

[Mark Goodwin [:mgoodwin]]

(In reply to :Gijs (he/him) from comment #6)

Dana and I discussed briefly - though we don't need to handle the cert, it does appear like the // extensions strings in strings.js are unused and should be removed, so that they don't interfere with the DN parsing code, which will then just dump OID things as labels for identifiers it doesn't recognize.

I'm happy to take a look at this (feel free to assign).

Comment 10

[Mark Goodwin [:mgoodwin]]

(In reply to :Gijs (he/him) from comment #6)

Dana and I discussed briefly - though we don't need to handle the cert, it does appear like the // extensions strings in strings.js are unused and should be removed, so that they don't interfere with the DN parsing code, which will then just dump OID things as labels for identifiers it doesn't recognize.

I can verify that removing the extensions from strings.js allows the certificate detail to be viewed.

I'll submit the patch when I've got my phabricator access back... (see bug 1662545).

FWIW, I think the right fixes here are:

1. to correctly handle any known OID, regardless of nesting - but maybe that can wait until we find an instance where a failure of this type occurs with a valid certificate.
2. to honey-badger cases where the OID appears known but the value does not parse. I say this because I think a certificate viewer should not be opinionated ("What a cert look like?" is a very different question to "Is this cert valid?"). In other implementations, a common approach is to display the OID and the raw (Base64 / Hex encoded) data in any case where the OID is unknown.

Comment 11

[:Gijs (he/him)]

(In reply to Mark Goodwin [:mgoodwin] from comment #10)

FWIW, I think the right fixes here are:

1. to correctly handle any known OID, regardless of nesting - but maybe that can wait until we find an instance where a failure of this type occurs with a valid certificate.
2. to honey-badger cases where the OID appears known but the value does not parse. I say this because I think a certificate viewer should not be opinionated ("What a cert look like?" is a very different question to "Is this cert valid?"). In other implementations, a common approach is to display the OID and the raw (Base64 / Hex encoded) data in any case where the OID is unknown.

SGTM - thanks for jumping on this, Mark!

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.