Closed Bug 1264761 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: geekmasher, Assigned: keeler)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 files)

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]
Excellent. Glad I can help.

GeekMasher.
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)
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)
(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 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 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)
https://hg.mozilla.org/mozilla-central/rev/f7caa27d445d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.