IPv6 address certificates in certificate store are ignored

RESOLVED FIXED in Firefox 54

Status

()

defect
P1
major
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: darkspirit, Assigned: keeler)

Tracking

54 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54+ fixed, firefox55 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 attachment)

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/
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.
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".
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
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
Priority: -- → P1
Whiteboard: [psm-assigned]
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+
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.
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
https://hg.mozilla.org/mozilla-central/rev/e8c4477db9bd
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
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+
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.