1345612 - IPv6 address certificates in certificate store are ignored

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Darkspirit]

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

[Tom Kloos]

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.

Comment 2

[Tom Kloos]

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".

Comment 3

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

Attachment: bug 1345612 - avoid calling NS_NewURI on IP addresses when checking certificate overrides

When determining if a certificate error override is allowed for a host, we
consult nsISiteSecurityService::IsSecureURI to see if the host is HSTS/HPKP.
This API takes an nsIURI, but the calling code only has a hostname as an
nsCString. Calling NS_NewURI works in all situations we will encounter except
when the hostname is an IPv6 address. Since IP addresses are never HSTS/HPKP
anyway, we can skip the NS_NewURI / IsSecureURI calls in those cases as a
workaround.

Review commit: https://reviewboard.mozilla.org/r/122080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122080/

Comment 4

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

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).

Comment 5

[:Cykesiopka]

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.

Comment 6

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

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 7

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

Comment on attachment 8849348 [details]
bug 1345612 - avoid calling NS_NewURI on IP addresses when checking certificate overrides

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8c4477db9bd
avoid calling NS_NewURI on IP addresses when checking certificate overrides r=Cykesiopka

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e8c4477db9bd

Comment 10

[Darkspirit]

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.

Comment 11

[Julien Cristau [:jcristau]]

Please request aurora uplift when you get a chance.

Comment 12

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

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

Comment 13

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

(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 14

[Gerry Chang [:gchang]]

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+.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6be6249c464

Comment 16

[Tom Kloos]

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

Thanks everyone!