IPv6 address certificates in certificate store are ignored

RESOLVED FIXED in Firefox 54

Status

()

Core
Security: PSM
P1
major
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: darkspirit, Assigned: keeler)

Tracking

54 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox54+ fixed, firefox55 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170308110300

Steps to reproduce:

not possible to add certificate exeptions on https://[::1]:1337 or https://[fd00::9ec7:a6ff:fe70:cb36]/ (=https://fritz.box/ consumer router)
I am using Nightly. I have this problem for weeks (or maybe longer undetected), but reported it only via the feedback form. Can you reproduce this?


Actual results:

if I confirm the popup to add the exception, nothing happens. If I connect to https://localhost:1337 or https://fritz.box (consumer router) it is possible to add certificate exceptions.
I was unsure if I should comment on this 6 year old bug which was about the same, but was fixed 4 years ago: https://bugzilla.mozilla.org/show_bug.cgi?id=633001


Expected results:

It should be possible to add certificate exceptions for https://[ipv6-address] as I can do it with https://ipv4-address/ and https://ipv6-domain/

Comment 1

a year ago
I'm having the same problem starting with firefox-54.0a2 AND seamonkey-2.51a2 versions.
Using the same profile for firefox-53.0a2 and seamonkey-2.50a2 works OK.  i.e. Existing personal certificates are honored on these previous version browsers.

This is definitely a problem when using a IPv6 link local address to access a LAN device.

Also (another bug?), it's no longer possible to "view" the certificate if some combination of CN, O, or OU fields are blank as is often the case with a LAN device having a self signed certificate.

Updated

a year ago
status-firefox54: --- → ?
tracking-firefox54: --- → ?

Comment 2

a year ago
After some experimentation some more details are available and the title should be changed to something like "IPv6 address certificates in certificate store are ignored".

FF >=54 will add them to the certificate store without problem, but then doesn't appear to see them.  FF version 53beta and below can then successfully use them (no need to add).

Tracking should be change to "54 branch" and importance possibly to "Major".
(Reporter)

Updated

a year ago
Severity: normal → major
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Not possible to add certificate exceptions on IPv6 addresses (https://[ipv6]/) → IPv6 address certificates in certificate store are ignored
Version: 55 Branch → 54 Branch
Comment hidden (mozreview-request)
Looks like this was caused by bug 1336867.

I'm not completely satisfied with how this turned out, so let me know if you can think of a more appropriate way to handle this (I'm also completely unsatisfied with how reviewboard decided to display this diff, but that's another story).
Assignee: nobody → dkeeler
Blocks: 1336867
status-firefox54: ? → affected
status-firefox55: --- → affected
Priority: -- → P1
Whiteboard: [psm-assigned]

Comment 5

a year ago
mozreview-review
Comment on attachment 8849348 [details]
bug 1345612 - avoid calling NS_NewURI on IP addresses when checking certificate overrides

https://reviewboard.mozilla.org/r/122080/#review125958

Apologies for the delay. 

I can't think of a better solution at the moment, so the change in general LGTM.

::: security/manager/ssl/SSLServerCertVerification.cpp:494
(Diff revision 1)
> -    return new SSLServerCertVerificationResult(mInfoObject,
> -                                               mDefaultErrorCodeToReport);
> +  // address fails. We do this to avoid that. A more comprehensive fix would be
> +  // to have Necko provide an nsIURI to PSM and to use that here (and
> +  // everywhere). However, that would be a wide-spanning change.
> +  const nsCString& hostname = PromiseFlatCString(mInfoObject->GetHostName());
> +  PRNetAddr unusedAddr;
> +  if (PR_StringToNetAddr(hostname.get(), &unusedAddr) == PR_SUCCESS) {

I believe we can use `net_IsValidIPv6Addr()` here instead (meaning we can avoid adding another NSPR dependency and make the `PromiseFlatCString()` usage unnecessary).

::: security/manager/ssl/SSLServerCertVerification.cpp:538
(Diff revision 1)
> -                          mInfoObject->GetOriginAttributes(),
> +                        mInfoObject->GetOriginAttributes(),
> -                          nullptr,
> +                        nullptr,
> -                          &hasPinningInformation);
> +                        &hasPinningInformation);
> -  if (NS_FAILED(nsrv)) {
> +  if (NS_FAILED(rv)) {
>      MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
> -           ("[%p][%p] checking for HPKP failed\n", mFdForLogging, this));
> +           ("[%p][%p] checking for HPKP failed", mFdForLogging, this));

Nit: Indent by one more space.
Attachment #8849348 - Flags: review?(cykesiopka.bmo) → review+
(Assignee)

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8849348 [details]
bug 1345612 - avoid calling NS_NewURI on IP addresses when checking certificate overrides

https://reviewboard.mozilla.org/r/122080/#review125958

No worries - thanks for the review!

> I believe we can use `net_IsValidIPv6Addr()` here instead (meaning we can avoid adding another NSPR dependency and make the `PromiseFlatCString()` usage unnecessary).

Perfect - this is what I was hoping existed but couldn't find.

> Nit: Indent by one more space.

Fixed.
Comment hidden (mozreview-request)

Comment 8

a year ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8c4477db9bd
avoid calling NS_NewURI on IP addresses when checking certificate overrides r=Cykesiopka

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e8c4477db9bd
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Comment 10

a year ago
Works for me (Thank you!), but there is another problem and I don't know if it belongs to this bug here.

uMatrix shows there are 3 scripts on the IPv6 address from my router(fritz box) which are allowed (green)
and 3rdparty scripts would be forbidden (red),
but the (allowed 1st-party) scripts don't get loaded, so the site is blank.
Only if I allow 3rdparty scripts the 1st-party scripts get loaded.
I don't know whether this is a new bug or not or if it's an uMatrix or a Firefox bug.
Please request aurora uplift when you get a chance.
tracking-firefox54: ? → +
Flags: needinfo?(dkeeler)
Comment on attachment 8849348 [details]
bug 1345612 - avoid calling NS_NewURI on IP addresses when checking certificate overrides

Approval Request Comment
[Feature/Bug causing the regression]: bug 1336867
[User impact if declined]: no certificate error overrides on IPv6 addresses (affects a small number of users, but still...)
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not really
[Why is the change risky/not risky?]: it's in the certificate error handling code, so any changes should be made with caution, but we have good test coverage here and nothing seems broken on Nightly, so I think this is low risk
[String changes made/needed]: none
Flags: needinfo?(dkeeler)
Attachment #8849348 - Flags: approval-mozilla-aurora?
(In reply to bugzilla from comment #10)
> Works for me (Thank you!)

Good to hear!

>, but there is another problem and I don't know if
> it belongs to this bug here.
> 
> uMatrix shows there are 3 scripts on the IPv6 address from my router(fritz
> box) which are allowed (green)
> and 3rdparty scripts would be forbidden (red),
> but the (allowed 1st-party) scripts don't get loaded, so the site is blank.
> Only if I allow 3rdparty scripts the 1st-party scripts get loaded.
> I don't know whether this is a new bug or not or if it's an uMatrix or a
> Firefox bug.

This sounds unrelated. Maybe file an issue with uMatrix (if possible) or file a new bug in Firefox :: Untriaged?
Comment on attachment 8849348 [details]
bug 1345612 - avoid calling NS_NewURI on IP addresses when checking certificate overrides

Fix an issue that user is unable to add certificate exceptions for https://[ipv6-address]. Aurora54+.
Attachment #8849348 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 15

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/a6be6249c464
status-firefox54: affected → fixed

Comment 16

a year ago
Works for me in today's (20170329...) builds of both firefox-54.0a2(64-bit) and seamonkey-2.51a2.

Thanks everyone!
You need to log in before you can comment on or make changes to this bug.