If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Certificates ASN.1 version number set to v4 stops certificate viewer from showing PKI chain

RESOLVED FIXED in Firefox 48

Status

()

Core
Security: PSM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: geekmasher, Assigned: keeler)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8741466 [details]
Zip containing all the certs and screenshot

Hey Guys,

I created a test case where I set the ASN.1 version number variable of a X509v3 leaf certificate to version 4.
The certificate was accepted (signed by my CA), which doesn't cause major issues, but when I tried to view the certificate chain in the viewer, it was broken and doesn't display anything at all.

Files Attachments:

Certificates:
    - certs/cert.pub.pem
    - certs/cert.key.pem
    - certs/ca.pem

Screenshots:
    - screenshots/viewer.png
    - screenshots/general.png


I'm not 100% sure if both this bug should be in "Security" and if this could lead to any major vulnerabilities but I will still submit it as hidden.


I've been developing a tool called SquarePeg (https://github.com/cigital/SquarePeg), which is still not published at the time I submit this bug report
I built is to test for TLS/SSL vulnerabilities in applications.
Group: core-security → crypto-core-security
Thanks for filing this bug. It's more or less a display issue (the backend code is handling the unexpected value by returning an error and the frontend essentially bails as a result).
Assignee: nobody → dkeeler
Group: crypto-core-security
Status: UNCONFIRMED → ASSIGNED
Component: Security → Security: PSM
Ever confirmed: true
Whiteboard: [psm-assigned]
Created attachment 8742481 [details]
MozReview Request: bug 1264761 - improve handling of x509 versions in certificate manager r?Cykesiopka

Review commit: https://reviewboard.mozilla.org/r/47223/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47223/
Attachment #8742481 - Flags: review?(cykesiopka.bmo)
(Reporter)

Comment 3

a year ago
Excellent. Glad I can help.

GeekMasher.

Comment 4

a year ago
I skimmed over the changes for now, and in general they look good. I just wanted to confirm something before looking closer...

It looks like since Bug 1047177, mozilla::pkix treats v4 certs as v3 internally.
Do we want to surface this fact in the UI?
Flags: needinfo?(dkeeler)
(Reporter)

Comment 5

a year ago
Either, Firefox should treat the v4 certificate like any other unknown versions (negatives and 5+) with a DER encoding error or it should display in the UI the fact that it is a v4 certificate.

I don't know if or when there will be a X509v4 certificate but support for it should be provided. Whether now or when the standard is published.
(In reply to :Cykesiopka from comment #4)
> It looks like since Bug 1047177, mozilla::pkix treats v4 certs as v3
> internally.
> Do we want to surface this fact in the UI?

In the future I'm imagining some kind of built-in linter that would flag this with a warning like "you probably meant x509v3, in which case you should set the value to 2 here", but I think that's out of scope for this bug. My goal here was to just show whatever was in the certificate, even if it claimed to be an x509 version that doesn't exist yet.
Flags: needinfo?(dkeeler)

Comment 7

a year ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #6)
> In the future I'm imagining some kind of built-in linter that would flag
> this with a warning like "you probably meant x509v3, in which case you
> should set the value to 2 here", but I think that's out of scope for this
> bug. My goal here was to just show whatever was in the certificate, even if
> it claimed to be an x509 version that doesn't exist yet.

OK, thanks for clarifying.

Comment 8

a year ago
Comment on attachment 8742481 [details]
MozReview Request: bug 1264761 - improve handling of x509 versions in certificate manager r?Cykesiopka

https://reviewboard.mozilla.org/r/47223/#review44235

Looks good.

::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties:56
(Diff revision 1)
>  # LOCALIZATION NOTE (nick_template): $1s is the common name from a cert (e.g. "Mozilla"), $2s is the CA name (e.g. VeriSign)
>  nick_template=%1$s's %2$s ID
>  #These are the strings set for the ASN1 objects in a certificate.
>  CertDumpCertificate=Certificate
>  CertDumpVersion=Version
> -CertDumpVersion1=Version 1
> +CertDumpVersionValue=Version %S

Nit: An l10n note to describe the %S part would probably help here.

::: security/manager/ssl/nsNSSCertHelper.cpp:105
(Diff revision 1)
>      // If there is no version present in the cert, then rfc2459
>      // says we default to v1 (0)

Optional: Update this to say RFC5280.

::: security/manager/ssl/tests/unit/test_cert_version.js:58
(Diff revision 1)
> +  Assert.equal(version.displayValue, expectedVersionString,
> +            "Actual and expected version strings should match");

Nit: Aline the message, and maybe drop the |Assert.| part.
Attachment #8742481 - Flags: review?(cykesiopka.bmo) → review+
https://reviewboard.mozilla.org/r/47223/#review44235

Thanks!

> Nit: An l10n note to describe the %S part would probably help here.

Good idea.

> Optional: Update this to say RFC5280.

Sounds good.

> Nit: Aline the message, and maybe drop the |Assert.| part.

Fixed.
Comment on attachment 8742481 [details]
MozReview Request: bug 1264761 - improve handling of x509 versions in certificate manager r?Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47223/diff/1-2/

Comment 11

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb60c7a0b0c5
I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5c27d9625278 for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=26158852&repo=mozilla-inbound
Flags: needinfo?(dkeeler)
Comment on attachment 8742481 [details]
MozReview Request: bug 1264761 - improve handling of x509 versions in certificate manager r?Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47223/diff/2-3/
Turned out to be a 32-bit/64-bit issue, basically.
Flags: needinfo?(dkeeler)

Comment 15

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7caa27d445d

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7caa27d445d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.