633001 - SSL cannot set exceptions on IPv6 addresses

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[gavinspearhead]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b12pre) Gecko/20110203 Firefox/4.0b12pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b12pre) Gecko/20110203 Firefox/4.0b12pre

When connecting to a raw IPv6 address (https://[2001:1888:1000::3]:443 over ssl with a selfsigned certificate, this error is shown is a popup 

"An error occurred during a connection to 2001:1888:1000::3:443.

Peer's certificate issuer has been marked as not trusted by the user.

(Error code: sec_error_untrusted_issuer)"

The "confirm security exception" button remains disabled.

Reproducible: Always

Steps to Reproduce:
1.enter url https://[2001:1888:1000::3]:443
2. add excetpion
3. get certificate

Actual Results:  

"An error occurred during a connection to 2001:1888:1000::3:443.

Peer's certificate issuer has been marked as not trusted by the user.

(Error code: sec_error_untrusted_issuer)"

The "confirm security exception" button remains disabled.

Expected Results:  
"confirm security exception" button is enabled. And I can add it.

Comment 1

[Erik Lax]

I can confirm this bug, made some investigation and traced down the problem to the nsNSSBadCertHandler where proxied_stss->IsStsHost (StrictTransportSecurityService) is called with a string representation of a hostname, IPv4 or IPv6 address.

IsStsHost constructs a URI from this string (aHost) without taking the IPv6 URI scheme into consideration (enclosing IPv6 literals in square brackets).

See attached patch.

Comment 2

[Erik Lax]

Attachment: patch should fix bug 633001

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

I'm not sure I fully understand how the symptoms map to a fix in the patch (attachment 523787 [details] [diff] [review]).

It seems to me that the problem is that "confirm security exception" is disabled, when in fact it should be enabled.  This symptom indicates that nsStrictTransportSecurityService thinks the IPv6 address is an HSTS host -- perhaps mistakenly.

If the patch in attachment 523787 [details] [diff] [review] does indeed cause the "confirm security exception" button to be re-enabled, then there might be a deeper problem with how IsStsHost (and subroutines) decide whether or not the host is HSTS.

What currently happens when the IPv6 address string is concatenated passed into NS_NewURI()?  If NS_NewURI() fails, then the whole IsStsHost() check should fail.  If NS_NewURI() doesn't fail, perhaps it should (given an invalid URI)?

Comment 5

[Gareth Watts]

fyi the patch fixed the issue for me

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 523787 [details] [diff] [review]
patch should fix bug 633001

Review of attachment 523787 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is the right approach, since the underlying problem seems to be that the STS service is concatenating strings to make new URIs.  Ideally, that shouldn't be happening. 

Even though it may fix the symptoms for IPv6-based URIs, I think execution should only enter the HSTS code paths if there is a domain name in the URI.  Section 7.1.1 of the HSTS spec disqualifies IPv4 addresses as valid HSTS hosts, and I think IPv6 is probably also included since "subdomains" doesn't make a whole lot of sense for IPv6 hosts.

I would prefer a fix that tweaks the caller of IsStsHost() (nsNSSIOLayer.cpp:3533) to only query the STS Service if the host is a domain name and not an IPV4 or IPV6 literal -- but this may not be necessary if we tweak IsStsHost() to not abuse IPv4 and IPv6 URIs.

We should probably also fix IsStsHost() to clone the URI instead of build a new one with string concatenation, so I filed bug 679757.

Comment 8

[prerna]

The above patch fixed the issue for me when i rebuilt mozilla with the patch provided. When can i expect the fix in mozilla firefoz release as this issue is critical for our product.

waiting for a prompt reply!

thanks,
Prerna Gupta

Comment 9

[prerna]

Hi,

Any updates would be appreciated!

thanks,
Prerna Gupta

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Sid Stamm [:geekboy] from comment #6)
> I would prefer a fix that tweaks the caller of IsStsHost()
> (nsNSSIOLayer.cpp:3533) to only query the STS Service if the host is a
> domain name and not an IPV4 or IPV6 literal

IsStsHost() should do this check itself because enforcement of the HSTS constraints should be done by the STS service..

> We should probably also fix IsStsHost() to clone the URI instead of build a
> new one with string concatenation, so I filed bug 679757.

Which URI would it clone?

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Brian Smith (:bsmith) from comment #10)
> IsStsHost() should do this check itself because enforcement of the HSTS
> constraints should be done by the STS service..

I guess that makes sense, though I was trying to avoid the extra xpcom call.  Rather than in IsStsHost(), we should do the check in IsStsURI(), which is called by IsStsHost(), and then we only have to check in one place.  We should at the same time look for IPv4 literals and disallow them since the spec is only defined for DNS hostnames.

Would it make more sense to construct an nsIURI with a dummy hostname and then insert the hostname via SetHost() into the newly constructed URI object?  I don't fully understand how the host is parsed in this fashion, or what is the best way to construct the URI, but I imagine it would take be a better approach than string concatenation.  Looking at nsStandardURL.cpp, I *think* this technique will identify and properly escape the IPv6 literal.

> > We should probably also fix IsStsHost() to clone the URI instead of build a
> > new one with string concatenation, so I filed bug 679757.
> 
> Which URI would it clone?

Yeah, I don't know what was wrong with me when I wrote that comment.  Please ignore that.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Sid Stamm [:geekboy] from comment #11)
> I guess that makes sense, though I was trying to avoid the extra xpcom call.

Doesn't matter, because anything related to cert override handling is not performance-critical.

> Would it make more sense to construct an nsIURI with a dummy hostname and
> then insert the hostname via SetHost() into the newly constructed URI
> object?  Looking at nsStandardURL.cpp, I *think* this technique will
> identify and properly escape the IPv6 literal.

Yes, that appears to be the case. But, I noticed EscapeIPv6 doesn't do much validation, so we should carefully review where we are getting this hostname from (see bug 559469, bug 554596, bug 67730).

Comment 13

[prerna]

Reminder!

when can I expect the fix to be ported to a mozilla firefox release? Some of my tasks are blocked due to this and it is crucial for our product.

thanks,
Prerna

Comment 14

[Josh Aas]

Erik - are you able to continue working on this or should we find another owner?

Comment 15

[Erik Lax]

Attachment: updated patch

Comment 16

[Erik Lax]

I made some changes, hopefully according to your suggestions. The URI object is created in IsStsHost (using SetHost) and the check for IPv4 (as suggested) and IPv6 addresses is done in IsStsURI.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 571158 [details] [diff] [review]
updated patch

Review of attachment 571158 [details] [diff] [review]:
-----------------------------------------------------------------

Please provide more context with your patches (hg diff -p -U 8, or see https://developer.mozilla.org/en/Creating_a_patch)... it's much easier to review with more context.

The approach looks good, so long as the items in this comment are addressed.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +328,4 @@
>    nsCOMPtr<nsIURI> uri;
>    nsDependentCString hostString(aHost);
>    nsresult rv = NS_NewURI(getter_AddRefs(uri),
> +                          NS_LITERAL_CSTRING("https://"));

Please test that this works without a dummy host that will get overwritten by SetHost() later... I think it does work without the host, but haven't verified it myself.

@@ +344,4 @@
>    NS_ENSURE_TRUE(NS_IsMainThread(), NS_ERROR_UNEXPECTED);
>  
>    nsresult rv;
> +

This is a good place to assign false to *aResult since there are two return points you're inserting below (each where *aResult should be  false).

@@ +351,5 @@
> +  rv = GetHost(aURI, host);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  PRNetAddr hostAddr;
> +  if (PR_StringToNetAddr(host.get(), &hostAddr) == PR_SUCCESS) {

Nit: Please use the rv = foo and NS_SUCCEEDED() pattern here instead of ==.  For example, assuming an earlier assignment of false to *aResult:

 rv = PR_StringToNetAddr(host.get(), &hostAddr);
 if (NS_SUCCEEDED(rv)) return NS_OK;

Comment 18

[Erik Lax]

Attachment: updated patch (2)

This updated patch should address the feedback given in the review. I have verified that it works without a dummy host. And it was one major reason for leaving it out.

Comment 19

[prerna]

Hi everyone,

I just tried to validate my IPV6 scenario on mozilla firefox latest release 9 and I am still facing the issue. Does anyone have any idea when can this fix be expected in a release?

thanks,
Prerna Gupta

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 571382 [details] [diff] [review]
updated patch (2)

   nsresult rv = NS_NewURI(getter_AddRefs(uri),
-                          NS_LITERAL_CSTRING("https://") + hostString);
+                          NS_LITERAL_CSTRING("https://"));

this feels a little weird to me, can you initialize it with a dummy host instead of nothing?

+  rv = PR_StringToNetAddr(host.get(), &hostAddr);
+  if (NS_SUCCEEDED(rv)) return NS_OK;

Unfortunately, sid was not correct here. PR_StringToNetAddr does not return an nsresult, so NS_SUCCEEDED can't be used here. The code in the previous version was correct, please change back to that. I would also put the return on the next line so that a) it's possible to set breakpoints on it/step through it, and b) that it's clearer that this is not a return rv; which most of the single-line versions are

Comment 21

[Erik Lax]

Attachment: updated patch (3)

Thank you, I've updated the patch according to your suggestions.

Comment 22

[Josh Aas]

Erik - do you need someone to check this in for you? Have you run it through the try server?

Comment 23

[Erik Lax]

(In reply to Josh Aas (Mozilla Corporation) from comment #22)
> Erik - do you need someone to check this in for you?

Yes, please

> Have you run it through the try server?

No, I probably don't have access to any of these, could someone else do it for me?

Comment 24

[Patrick McManus [:mcmanus]]

(In reply to Erik Lax from comment #23)

> No, I probably don't have access to any of these, could someone else do it
> for me?

I sent it to try:

https://tbpl.mozilla.org/?tree=Try&rev=8e09a09ae2e3

Erik, consider filing a bug asking for level-1 commmit access. That will let you use try (but not actually checkin to any shipping repo). Try is really, really helpful! (tbpl.mozilla.org/?tree=Try) .. cc me (or any other committer) and we'll provide the required vouch.

Thanks!

Comment 25

[Mozilla RelEng Bot]

Try run for 8e09a09ae2e3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8e09a09ae2e3
Results (out of 262 total builds):
    success: 232
    warnings: 27
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8e09a09ae2e3

Comment 26

[Josh Aas]

I don't see anything worrisome in the try results. I starred a bunch of them and the mac debug red is probably an infrastructure problem.

Comment 27

[Patrick McManus [:mcmanus]]

removing checkin-needed that josh asked me to set because I just remembered that PSM has special authoring and review rules.

I don't think the audit trail here qualifies.. 

assign to brian to figure out and either set the review flags or re-add checkin-needed as apropos.

Comment 28

[Honza Bambas (:mayhemer)]

Comment on attachment 587210 [details] [diff] [review]
updated patch (3)

>   nsresult rv = NS_NewURI(getter_AddRefs(uri),
>-                          NS_LITERAL_CSTRING("https://") + hostString);
>+                          NS_LITERAL_CSTRING("https://dummyhost.mozilla.org"));
>   NS_ENSURE_SUCCESS(rv, rv);
>+  rv = uri->SetHost(hostString);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

Hmm.. isn't a need for doing this a bug in nsStandardURL implementation?

Comment 29

[Erik Lax]

(In reply to Honza Bambas (:mayhemer) from comment #28)
> Hmm.. isn't a need for doing this a bug in nsStandardURL implementation?

I don't think so, creating a URI object from a host that can be any of IPv4, IPv6 or a host name most likely requires the "SetHost" call. SetHost detects the host type and safely updates the URI object. In a previous attempt, the IPv6 address was also detected but enclosed in square brackets before calling NS_NewURI, not to mistake any of the groups (::) with a port. But this way is a much cleaner implementation.

Comment 30

[Honza Bambas (:mayhemer)]

Comment on attachment 587210 [details] [diff] [review]
updated patch (3)

Review of attachment 587210 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +332,2 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +  rv = uri->SetHost(hostString);

Blank line before this one please.

@@ +346,5 @@
>    nsresult rv;
> +  *aResult = false;
> +
> +  // If the host is an address literal, it doesn't qualify to be an
> +  // STS host (HSTS Section 7.1.1)

I've found this in Appendix A, para 4.  I don't think it is good to reference the spec by article number.  The spec may change and this gets obsolete (as have already happened).

@@ +352,5 @@
> +  rv = GetHost(aURI, host);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  PRNetAddr hostAddr;
> +  if (PR_StringToNetAddr(host.get(), &hostAddr) == PR_SUCCESS)

Instead of .get() you may use .BeginReading().

Comment 31

[Erik Lax]

Attachment: updated patch (4)

Thanks for the feedback, I updated the patch accordingly.

Comment 32

[Erik Lax]

Comment on attachment 595876 [details] [diff] [review]
updated patch (4)

Who's next to review this patch in order to make progress? Brian?

Comment 33

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Erik,

I am working on improving the testing infrastructure so that we can test changes like this effectively. I will revisit this in two weeks.

Comment 34

[Lawrence Cohen]

We are hitting this issue now.  Is a release with this fix coming soon ?
Also, just to understand a bit more.   Does if the cert contains IPv6 addresses in the commonName or Subject Alternative Name fields ?   Or is it just a problem accessing the system with an IPv6 address.

Comment 35

[Lawrence Cohen]

Does ->  Does the problem only occur if the cert contains IPv6 address.  Sorry about that.

Comment 36

[Lawrence Cohen]

Does ->  Does the problem only occur if the cert contains IPv6 address.  Sorry about that.

Comment 37

[Mars G Miro]

This problem still exists in recent FF 12.0

Comment 38

[Erik Lax]

(In reply to Mars G Miro from comment #37)
> This problem still exists in recent FF 12.0

Yes it does, and the patch still applies and fixes this problem.

(In reply to Brian Smith (:bsmith) from comment #33)
> Erik,
> 
> I am working on improving the testing infrastructure so that we can test
> changes like this effectively. I will revisit this in two weeks.

Brian,

Are you still up for the code review, or is there someone else that could/should do it?

Comment 39

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Erik Lax from comment #38)
> Brian wrote:
> > I am working on improving the testing infrastructure so that we can test
> > changes like this effectively. I will revisit this in two weeks.
> 
> Are you still up for the code review, or is there someone else that
> could/should do it?

Sorry about the delay. I am still working on the testing infrastructure. I am getting very close to the point where we can write an automated test for your patch in a reasonable way. Once that is done, I will write a test and review the patch.

Comment 40

[Zach Shepherd]

Is there an ETA for this fix?

Comment 41

[Josh Aas]

(In reply to Zach Shepherd from comment #40)
> Is there an ETA for this fix?

No ETA yet. We're waiting on testing capabilities from Brian, but if that is going to take much longer we might want to take the fix without the testing.

Comment 42

[Josh Aas]

Thinking about it for another minute, I don't think we're going to have that testing capability any time soon. IMO we should take a patch now and do our best, pay attention during the bake cycles.

Brian - can you please review this?

Comment 43

[Zach Shepherd]

(In reply to Josh Aas (Mozilla Corporation) from comment #41)
> (In reply to Zach Shepherd from comment #40)
> > Is there an ETA for this fix?
> 
> No ETA yet. We're waiting on testing capabilities from Brian, but if that is
> going to take much longer we might want to take the fix without the testing.

(In reply to Josh Aas (Mozilla Corporation) from comment #42)
> Thinking about it for another minute, I don't think we're going to have that
> testing capability any time soon. IMO we should take a patch now and do our
> best, pay attention during the bake cycles.
> 
> Brian - can you please review this?

Thanks for the quick replies.

Are there any particular manual testing results that would be useful to have once the fix is in?

Comment 44

[Christian Krause]

I'm the reporter of one of the duplicates (#661118) of this bug report.

I have tested the current patch "updated patch (4)":

test setup:
- ssl server with a certificate which can't be validated (in my case: CA unknown)
- FF 14.0 with (and without) the mentioned patch

steps:
- try connecting to the following URLs:

a) host name: https://name/
b) IPv4 address: https://192.168.1.1/
c) IPv6 address: https://[fd07:....]/

results:
a) works fine with and without the patch
b) works in vanilla FF 14, but is broken with the patch (same behaviour as the original report with IPv6: "Confirm Security Exception" button is greyed out)
c) does not work in vanilla FF 14 (as expected), but is fixed by the provided patch

I.e. the suggested patch fixes the issue for IPv6 but causes a regression by breaking the feature of adding a security exception for IPv4.

I would be happy to help testing a new version of the patch.

Comment 46

[Mars G Miro]

Hi guys

 I finally found time to test the patch, but unfortunately the code has changed in FF 15.X at least. If there is an updated patch, I'd be happy to test it and report back ;-)
 
 Thanks!

Comment 47

[Gabor Batori]

This problem still exists in recent FF 16.0.2

Comment 48

[Matthieu Bouthors]

I think I have the same issue with FF 18.0.2 on Windows 7 64bits. Impossible to log into SSL IPv6 websites with self signed certificates.
No problems with IE9 or chrome 24.

Comment 49

[Josh Aas]

Attachment: updated patch (5) - (beta-24 and aurora-25)

Updated to current trunk. I don't know this code, so it may not be correct, I just translated the code over and made sure it compiled.

Comment 50

[Mars G Miro]

I adjusted the patch for FF-18.0.2 - http://pastebin.com/0EeQrcPL

It seems to work ;-)

Unpatched FF-18.0.1 - http://i.imgur.com/fJUhNeX.png?1

Patched FF-18.0.2 - http://i.imgur.com/U6N5Dbe.png?1

This is on FreeBSD 9.1-RELEASE/amd64.

Thanks!

Comment 51

[Christian Krause]

I have tried to verify the latest patch, but unfortunately even with an unpatched firefox 17/18 I can't access https IPv6 URLs anymore (firefox does not even show the certificate warning page). It looks like that other users have the same problem:
https://bugzilla.mozilla.org/show_bug.cgi?id=828873

Once the other issue is fixed, I'm going to test the latest patch.

Comment 52

[Christian Krause]

Encouraged by the success in comment #50 I have now also tested the latest patch against firefox 18.0:

- it solves issue #828873
- ssl certificate exceptions can now be granted for all kind of addresses:
a) host name: https://name/
b) IPv4 address: https://192.168.1.1/
c) IPv6 address: https://[fd07:....]/

I'm happy with the current patch and I hope it gets reviewed and committed soon. ;-)

Comment 53

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Brian Smith (:bsmith) from comment #39)
> Sorry about the delay. I am still working on the testing infrastructure. I
> am getting very close to the point where we can write an automated test for
> your patch in a reasonable way. Once that is done, I will write a test and
> review the patch.

Any update?

Comment 54

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 710900 [details] [diff] [review]
updated patch (5) - (beta-24 and aurora-25)

Review of attachment 710900 [details] [diff] [review]:
-----------------------------------------------------------------

This should have an automated test. It would be very simple to create an xpcshell test that just does a do_GetService and then calls isSTSHost. In particular, it is not clear what forms of ipv6 literals that aHost is allowed to be in. In particular, are we suppposed to accept literals with our without the brackets? This should be covered by the test.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +369,4 @@
>    nsCOMPtr<nsIURI> uri;
>    nsDependentCString hostString(aHost);
>    nsresult rv = NS_NewURI(getter_AddRefs(uri),
> +                          NS_LITERAL_CSTRING("https://dummyhost.mozilla.org"));

Use example.org instead of a real domain, especially a real mozilla.org domain, to minimize risk.

Comment 55

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 710900 [details] [diff] [review]
updated patch (5) - (beta-24 and aurora-25)

Clearing review request until test is written.

Comment 56

[Mars G Miro]

Okay I adjusted the patch for FF-20 - http://pastebin.com/7bssFhS5

Unpatched FF-20 - http://imgur.com/G22kRW1

Patched FF-20 - http://imgur.com/y6gvgEH

This is on FreeBSD-9.1-RELEASE-p2 / amd64.

Thanks.

Comment 57

[Lukas Blakk [:lsblakk] use ?needinfo]

Hey Sid - assigning this to you (for now) because we're tracking it and need a reliable point person, re-assign to another dev if you have someone working on this.

Comment 58

[Sid Stamm [:geekboy or :sstamm]]

um, okay.  Is this still a problem after bug 871560 was marked fixed?  Erik, can you or the original reporter check?

Comment 59

[Masatoshi Kimura [:emk]]

Bug 871560 was "fixed" by backout of bug 861117. We need to fix this bug to reland bug 861117 without regressing bug 871560 again.

Comment 60

[Lukas Blakk [:lsblakk] use ?needinfo]

I don't see why we should still be tracking this issue since bug 871560 has been fixed by backing out bug 861117, it's been present since FF16 at least.  Sid, flagging ni? on you in case you see this issue as being still a blocker for FF23, renom with reasoning.

Comment 61

[Lukas Blakk [:lsblakk] use ?needinfo]

I guess the main question is: does this become a more prominent issue because of Mixed Content Blocking?

Comment 62

[Sid Stamm [:geekboy or :sstamm]]

I don't think mixed content blocking should raise the priority of this bug.

But we should still fix this so we can reland bug 861117.  What's left to do?  I am a bit confused.  Brian, do we need tests for this?

Comment 63

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #62)
> I don't think mixed content blocking should raise the priority of this bug.
> 
> But we should still fix this so we can reland bug 861117.  What's left to
> do?  I am a bit confused.  Brian, do we need tests for this?

I think we just need an xpcshell test that verifies that nsIStrictTransportSecurityService does something reasonable for IPv6 literal hostnames.

The question is, what is the reasonable thing to do? IIRC, we intend to discourage certificates that include IP addresses so perhaps the test should verify that nsIStrictTransportSecurityService never adds them as HSTS to the permission manager and always correctly returns "no" when asked if they are HSTS sites.

Comment 64

[Doktor Notor]

Guys, with 23.0, you don't even bother to display the error messages shown by the bug reporter. People are just getting a blank page. A.k.a., completely broken behaviour. 

Could you move to some action actually, instead of debating what you wish to "discourage" once again?

Comment 65

[Jan Horak [:jhorak]]

Also getting blank page with nightly. The 'updated patch(5)' is fixing this issue. What can I do to move things forward (some of our customers are hitting this issue and getting impatient)?

Comment 66

[Kai Engert [:KaiE:]]

Attachment: patch v5 for esr-17 branch

This is the same as "updated patch (5)", but with the right context to apply on the ESR-17 branch.

Comment 67

[Kai Engert [:KaiE:]]

The problem is worse than reported in this bug.

At least since Firefox 17, it's no longer possible to access URLS liks

  https :// [ ipv6-address ] /

at all. That problem is described in bug 828873.

Luckily, the patch attached to this bug fixes bug 828873, too.

Because bug 828873 is a regression, I propose to land the fix, despite not having an automated test yet.

Comment 68

[Kai Engert [:KaiE:]]

Attachment: patch testing on esr-17 branch

This test fails.
With patch v5 applied, this test succeeds.
Tested on esr-17 branch.

Comment 69

[Kai Engert [:KaiE:]]

Attachment: patch testing on beta-24 and aurora 25 branch

Comment 70

[Kai Engert [:KaiE:]]

I'm interested in getting Firefox 17 (current ESR) and Firefox 24 (next ESR) fixed for enterprise customers, and the above patches seem to work. They should get applied to the ESR branches.

Unfortunately the usual rule
  "must fix on mozilla-central first, prior to landing on branches"

probably doesn't apply here, as it seems the related code has been moved and reworked. Above patches don't apply to mozilla-central-26.

Comment 71

[Kai Engert [:KaiE:]]

Comment on attachment 794154 [details] [diff] [review]
patch testing on beta-24 and aurora 25 branch

pls ignore the unrelated copy/paste comment, we should remove that of course

Comment 72

[Kai Engert [:KaiE:]]

Attachment: both patch v5 and testing merged to mozilla-central 26

Comment 73

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #70)
> Unfortunately the usual rule
>   "must fix on mozilla-central first, prior to landing on branches"
> probably doesn't apply here, as it seems the related code has been moved and
> reworked. Above patches don't apply to mozilla-central-26.

Luckily it wasn't as bad as I had thought.

The code got renamed, but it was trivial to merge the patch to mozilla-central.

Comment 74

[bhavana bajaj [:bajaj]]

If this needs to resolved in Fx24 keeping ESR in mind, this should get be resolved and landed Monday on central and uplifted to the branches asap as we are already in the forth week of beta cycle.

Also clear risk vs reward analysis and testing completed in the uplift nomination will be very helpful.

Comment 75

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #58)

Looks like this is still a problem, and it's getting fixed.  Clearing ni.

Comment 76

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

Comment on attachment 794224 [details] [diff] [review]
both patch v5 and testing merged to mozilla-central 26

Review of attachment 794224 [details] [diff] [review]:
-----------------------------------------------------------------

As far as I can tell, the error is that nsSiteSecurityService accepts IP address literals as valid input, whereas it should just refuse to do any processing on them.

::: security/manager/boot/src/nsSiteSecurityService.cpp
@@ +377,5 @@
>    // Only HSTS is supported at the moment.
>    NS_ENSURE_TRUE(aType == nsISiteSecurityService::HEADER_HSTS,
>                   NS_ERROR_NOT_IMPLEMENTED);
>  
> +  // Create a dummy URI, and then set the host in order to get proper handling

Why create a dummy URI? Just call PR_StringToNetAddr as in IsSecureURI and exit early if this is an IP address literal (and maybe factor out the common code to a little helper function). Also, we should check this in ProcessHeader. Ideally it would return an error if an IP address literal is supplied, but we might not be able to enforce that. At the very least, it shouldn't try to make an IP literal host HSTS.

::: security/manager/ssl/tests/unit/test_sts_ipv4_ipv6.js
@@ +1,4 @@
> +function run_test() {
> +  let SSService = Cc["@mozilla.org/ssservice;1"]
> +                    .getService(Ci.nsISiteSecurityService);
> +

Also include calls to SSService.processHeader that attempt to set HSTS for IP address literals and check that they don't succeed.

@@ +2,5 @@
> +  let SSService = Cc["@mozilla.org/ssservice;1"]
> +                    .getService(Ci.nsISiteSecurityService);
> +
> +  do_check_false(SSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS,
> +                                        "localhost", 0));

Why are you checking localhost?

Comment 77

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

Comment on attachment 794154 [details] [diff] [review]
patch testing on beta-24 and aurora 25 branch

Review of attachment 794154 [details] [diff] [review]:
-----------------------------------------------------------------

Essentially, apply all comments to patch 794224 to this as well.

Comment 78

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

Comment on attachment 794147 [details] [diff] [review]
patch testing on esr-17 branch

Review of attachment 794147 [details] [diff] [review]:
-----------------------------------------------------------------

Apply all comments to patch 794224 to this as well.

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +17,5 @@
>  skip-if = true
>  # Bug 846862: disable test until bug 836097 is resolved
>  [test_sts_preloadlist_selfdestruct.js]
> +skip-if = true
> +

These empty lines aren't necessary (although the file should end with a newline, which it looks like it hadn't, previously).

Comment 79

[Kai Engert [:KaiE:]]

Attachment: patch v6

updated patch as requested

Comment 80

[Kai Engert [:KaiE:]]

Comment on attachment 796093 [details] [diff] [review]
patch v6

oh, you requested changes to testing, too...

Comment 81

[Kai Engert [:KaiE:]]

David, you asked:

> Also include calls to SSService.processHeader that attempt to set HSTS for IP address
> literals and check that they don't succeed.

However, the processHeader method will never return a failure based on the source URI, so we cannot test that from JS.

Comment 82

[Kai Engert [:KaiE:]]

Attachment: patch v7

also removed the test for "localhost", test literal IP addresses, only.

Comment 83

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

Comment on attachment 796112 [details] [diff] [review]
patch v7

Review of attachment 796112 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Kai Engert (:kaie) from comment #81)
> David, you asked:
> 
> > Also include calls to SSService.processHeader that attempt to set HSTS for IP address
> > literals and check that they don't succeed.
> 
> However, the processHeader method will never return a failure based on the
> source URI, so we cannot test that from JS.

I think I see what you're saying. However, we can still test this by doing the following: add a check in ProcessHeader that returns early if given a URI where the host is an IP address. Then, just do something like this:

let uri = Services.io.newURI("https://[1080::8:800:200C:417A]", null, null);
let parsedMaxAge = {};
let parsedIncludeSubdomains = {};
SSService.processHeader(Ci.nsISiteSecurityService.HEADER_HSTS, uri, "max-age=1000;includeSubdomains", 0, parsedMaxAge, parsedIncludeSubdomains);
do_check_neq(parsedMaxAge.value, 1000);
do_check_neq(parsedIncludeSubdomains.value, true);

(processHeader passes back the values parsed out of the header, but if it returns early, then it won't parse the values)

If that sounds reasonable to you, then r=me with these changes. Thanks for taking care of this!

::: security/manager/boot/src/nsSiteSecurityService.cpp
@@ +449,5 @@
>    nsresult rv = GetHost(aURI, host);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  /* An IP address never qualifies as a secure URI. */
> +  if (HostIsIPAddress(host.BeginReading()))

nit: add braces around the body of the conditional

Comment 84

[Kai Engert [:KaiE:]]

Attachment: Patch v8

Thanks for the testing idea.

Do you want to take a final look, since I added code to the implementation?

Comment 85

[Kai Engert [:KaiE:]]

Attachment: patch v9

... removed the obsolete comment

Comment 86

[Kai Engert [:KaiE:]]

Thanks for the r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/03c5dfa11453

Comment 87

[Kai Engert [:KaiE:]]

Attachment: patch v9 merged to aurora-25 and beta-24 branches

Comment 88

[Kai Engert [:KaiE:]]

Attachment: patch v9 merged to esr-17 branch

Note the ProcessStsHeader code on the ESR-17 branch doesn't return parsed header results, so we cannot use the extended testing on that branch.

Comment 89

[Kai Engert [:KaiE:]]

(In reply to bhavana bajaj [:bajaj] from comment #74)
> Also clear risk vs reward analysis and testing completed in the uplift
> nomination will be very helpful.


Risk analysis for uplifting to ESR 17 + 24 + 25 branches:

Short version: This patch disables a code path that was wrong in the first place.

Long version:

The effect of the change is limited to this scenario:
- connecting to a site using https
- site isn't specified with a hostname (most common scenario),
  but using a numeric/literal IP address

Only in above scenario, we no longer attempt to enforce strict security.

However, enforcing strict security for IP addressed based connections was wrong in the first place, because certificate authorities are discouraged from issueing certificates based on IP addresses anyway. As a result, such connections would have required a security exception, which isn't allowed in combination with strict security sites anyway!


Reward analysis:

- the patch fixes a regression (bug 828873)
- the patch enables users to connect to devices that can be accessed using IPv6 only,
  such as network infrastructure
- users will be happy that Firefox actually works and that they don't have to use 
  another browser

Comment 90

[Kai Engert [:KaiE:]]

Comment on attachment 796321 [details] [diff] [review]
patch v9 merged to esr-17 branch

Requesting approval for ESR17

User impact if declined: Users/Admins cannot connect to sites by IPv6 address
Fix Landed on Version: 26
Risk to taking this patch (and alternatives if risky): zero, relying on the fact that 
                                                       our code to distinguish an IP
                                                       address from a hostname works well.
String or UUID changes made by this patch: none

Local testing using new xpcshell-test: succeeded

Comment 91

[Kai Engert [:KaiE:]]

Attachment: patch v9 merged to aurora-25 and beta-24 branches (rev 2)

Fixed a lowercase/uppercase issue in the test.

Comment 92

[Kai Engert [:KaiE:]]

Comment on attachment 796329 [details] [diff] [review]
patch v9 merged to aurora-25 and beta-24 branches (rev 2)

Requesting approval for aurora-25 and beta-24

Bug caused by (feature/regressing bug #): Bug 760307 (see also bug 828873)

User impact if declined: Users/Admins cannot connect to sites by IPv6 address
Fix Landed on Version: 26
Risk to taking this patch (and alternatives if risky): zero, relying on the fact that 
                                                       our code to distinguish an IP
                                                       address from a hostname works well.
String or UUID changes made by this patch: none

Local testing using new xpcshell-test: succeeded

Comment 93

[Kai Engert [:KaiE:]]

Testing in mozilla-inbound looking good so far, but still running.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=03c5dfa11453

I'm off for today (now 1:30 am),
but if this could make it into beta-24 in time, that would be great.

Comment 94

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/03c5dfa11453

Comment 95

[bhavana bajaj [:bajaj]]

Comment on attachment 796329 [details] [diff] [review]
patch v9 merged to aurora-25 and beta-24 branches (rev 2)

Approcving for branches given this is low risk and keeping ESR in mind.

In addition, Matt can you please help with a solid test-plan to test this new feature given the bake time we will get here.

Comment 96

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/266e80cc971b
https://hg.mozilla.org/releases/mozilla-beta/rev/d13fb050f879

Comment 97

[Lukas Blakk [:lsblakk] use ?needinfo]

This is not a high or critical security fix and therefore doesn't meet ESR uplift criteria.  Combined with the fact that it's landed in FF24 and thus will be available to our ESR users in the first ESR24 release they can update to that version to access this functionality.

Comment 98

[Matt Wobensmith [:mwobensmith][:matt:]]

Completed testing of this fix.

I was able to reproduce the problem in FF23 and pre-patch FF24+ builds. 

Using post-patch builds of 24, 25 and 26, I set up a web server to be accessed via an IPv6 address, with a self-signed cert. Using both new and old profiles, I navigated to this site and exercised the SSL exception UI to ensure that it would both appear and persist. 

I used an IPv6 address assigned by the network, as well as the localhost equivalent ([::1] which had not been an issue before) just for comparison. I ran hostnames with and without post-fixed ports, as well as various URL resources. I also ran all tests using IPv4 cases to verify that the behavior seemed the same.

I performed the same tests in private browsing mode, as well as multiple tabs. I ran tests for server-side redirects between IPv4/IPv6 content and IPv6 exceptions, as well as scenarios with multiple exceptions concurrently. 

What I did not test:
- IPv6 in general
- SSL in general
- Complex site navigation
- Other ports besides 443, 80
- HSTS permission manager - which appears to be covered by the test above in comments 83 and 84.


If anyone has concerns about something we may have missed in test coverage, please feel free to bring them up. I am going to mark this bug verified.