Closed Bug 633001 Opened 13 years ago Closed 11 years ago

SSL cannot set exceptions on IPv6 addresses

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox23 - wontfix
firefox24 + verified
firefox25 --- verified
firefox26 --- verified
firefox-esr17 - wontfix

People

(Reporter: gavinspearhead, Assigned: KaiE)

References

Details

(Whiteboard: [risk/reward see comment 89])

Attachments

(3 files, 14 obsolete files)

5.87 KB, patch
keeler
: review+
Details | Diff | Splinter Review
4.81 KB, patch
Details | Diff | Splinter Review
5.95 KB, patch
Details | Diff | Splinter Review
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.
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.
Attached patch patch should fix bug 633001 (obsolete) — Splinter Review
Attachment #523787 - Flags: review?(sstamm)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → gavinspearhead
Component: General → Security: PSM
Product: Firefox → Core
QA Contact: general → psm
Assignee: gavinspearhead → bugs
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)?
fyi the patch fixed the issue for me
Depends on: 679757
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.
Attachment #523787 - Flags: review?(sstamm) → review-
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
Hi,

Any updates would be appreciated!

thanks,
Prerna Gupta
(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?
(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.
(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).
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
Erik - are you able to continue working on this or should we find another owner?
Attached patch updated patch (obsolete) — Splinter Review
Attachment #523787 - Attachment is obsolete: true
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.
Attachment #571158 - Flags: feedback?(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;
Attachment #571158 - Flags: feedback?(sstamm) → feedback+
Attached patch updated patch (2) (obsolete) — Splinter Review
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.
Attachment #571158 - Attachment is obsolete: true
Attachment #571382 - Flags: review?(bsmith)
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 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
Attachment #571382 - Flags: review?(bsmith) → review-
Attached patch updated patch (3) (obsolete) — Splinter Review
Thank you, I've updated the patch according to your suggestions.
Attachment #571382 - Attachment is obsolete: true
Attachment #587210 - Flags: review?(cbiesinger)
Attachment #587210 - Flags: review?(cbiesinger) → review+
Erik - do you need someone to check this in for you? Have you run it through the try server?
(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?
(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!
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/mcmanus@ducksong.com-8e09a09ae2e3
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.
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.
Assignee: erik → bsmith
Keywords: checkin-needed
Assignee: bsmith → erik
Attachment #587210 - Flags: review?(bsmith)
Attachment #587210 - Flags: review?(honzab.moz)
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?
(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 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().
Attachment #587210 - Flags: review?(honzab.moz) → review+
Attached patch updated patch (4) (obsolete) — Splinter Review
Thanks for the feedback, I updated the patch accordingly.
Attachment #587210 - Attachment is obsolete: true
Attachment #587210 - Flags: review?(bsmith)
Comment on attachment 595876 [details] [diff] [review]
updated patch (4)

Who's next to review this patch in order to make progress? Brian?
Attachment #595876 - Flags: review?(bsmith)
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.
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.
Does ->  Does the problem only occur if the cert contains IPv6 address.  Sorry about that.
Does ->  Does the problem only occur if the cert contains IPv6 address.  Sorry about that.
This problem still exists in recent FF 12.0
(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?
(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.
Is there an ETA for this fix?
(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.
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?
(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?
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.
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!
This problem still exists in recent FF 16.0.2
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.
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.
Attachment #595876 - Attachment is obsolete: true
Attachment #595876 - Flags: review?(bsmith)
Attachment #710900 - Flags: review?(bsmith)
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!
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.
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. ;-)
(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?
Flags: needinfo?(bsmith)
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.
Flags: needinfo?(bsmith)
Comment on attachment 710900 [details] [diff] [review]
updated patch (5) - (beta-24 and aurora-25)

Clearing review request until test is written.
Attachment #710900 - Flags: review?(bsmith)
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.
Blocks: 871560
See Also: 871560
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.
Assignee: erik → sstamm
um, okay.  Is this still a problem after bug 871560 was marked fixed?  Erik, can you or the original reporter check?
Flags: needinfo?(gavinspearhead)
Bug 871560 was "fixed" by backout of bug 861117. We need to fix this bug to reland bug 861117 without regressing bug 871560 again.
Blocks: 861117
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.
Flags: needinfo?(sstamm)
Keywords: qawanted
QA Contact: mwobensmith
I guess the main question is: does this become a more prominent issue because of Mixed Content Blocking?
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?
Flags: needinfo?(sstamm) → needinfo?(brian)
(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.
Flags: needinfo?(brian)
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?
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)?
Attached patch patch v5 for esr-17 branch (obsolete) — Splinter Review
This is the same as "updated patch (5)", but with the right context to apply on the ESR-17 branch.
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.
Blocks: 828873
Attached patch patch testing on esr-17 branch (obsolete) — Splinter Review
This test fails.
With patch v5 applied, this test succeeds.
Tested on esr-17 branch.
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.
Attachment #794147 - Flags: review?(brian)
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
Attachment #794154 - Flags: review?(brian)
Attachment #710900 - Attachment description: updated patch (5) → updated patch (5) - (beta-24 and aurora-25)
Attachment #794154 - Attachment description: patch testing on beta-24 branch → patch testing on beta-24 and aurora 25 branch
Attachment #794224 - Flags: review?(brian)
(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.
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.
(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.
Flags: needinfo?(gavinspearhead)
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?
Attachment #794224 - Flags: review?(brian) → review-
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.
Attachment #794154 - Flags: review?(brian) → review-
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).
Attachment #794147 - Flags: review?(brian) → review-
Attached patch patch v6 (obsolete) — Splinter Review
updated patch as requested
Attachment #710900 - Attachment is obsolete: true
Attachment #794125 - Attachment is obsolete: true
Attachment #794147 - Attachment is obsolete: true
Attachment #794154 - Attachment is obsolete: true
Attachment #794224 - Attachment is obsolete: true
Attachment #796093 - Flags: review?(dkeeler)
Comment on attachment 796093 [details] [diff] [review]
patch v6

oh, you requested changes to testing, too...
Attachment #796093 - Attachment is obsolete: true
Attachment #796093 - Flags: review?(dkeeler)
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.
Attached patch patch v7 (obsolete) — Splinter Review
also removed the test for "localhost", test literal IP addresses, only.
Attachment #796112 - Flags: review?(dkeeler)
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
Attachment #796112 - Flags: review?(dkeeler) → review+
Attached patch Patch v8 (obsolete) — Splinter Review
Thanks for the testing idea.

Do you want to take a final look, since I added code to the implementation?
Assignee: sstamm → kaie
Attachment #796112 - Attachment is obsolete: true
Attachment #796275 - Flags: review?(dkeeler)
Attached patch patch v9Splinter Review
... removed the obsolete comment
Attachment #796275 - Attachment is obsolete: true
Attachment #796275 - Flags: review?(dkeeler)
Attachment #796277 - Flags: review?(dkeeler)
Attachment #796277 - Flags: review?(dkeeler) → review+
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.
(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
Keywords: qawanted
Whiteboard: [risk/reward see comment 89]
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
Attachment #796321 - Flags: approval-mozilla-esr17?
Fixed a lowercase/uppercase issue in the test.
Attachment #796320 - Attachment is obsolete: true
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
Attachment #796329 - Flags: approval-mozilla-beta?
Attachment #796329 - Flags: approval-mozilla-aurora?
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.
https://hg.mozilla.org/mozilla-central/rev/03c5dfa11453
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
Attachment #796329 - Flags: approval-mozilla-beta?
Attachment #796329 - Flags: approval-mozilla-beta+
Attachment #796329 - Flags: approval-mozilla-aurora?
Attachment #796329 - Flags: approval-mozilla-aurora+
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.
Attachment #796321 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17-
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.
Depends on: 903853
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: