409349 - Can get globalStorage objects for partial IP addresses

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Daniel Veditz [:dveditz]]

globalStorage[] does a domain-like match on partial IP-addresses. They should be treated as an indivisible unit (unless and until we scrap this syntax in favor of the new spec).

Given a page at http://63.245.209.10/  (current mozilla.com address)

javascript:alert(globalStorage['63.245.209.10'])   // should alert
javascript:alert(globalStorage['245.209.10'])      // should throw
javascript:alert(globalStorage['x.63.245.209.10']) // should throw

Currently all three alert (meaning the storage domains are accessible), but the latter two should be errors.

I didn't get a chance to test IPv6 addresses but I bet they're fubar'd too.

Comment 1

[Daniel Veditz [:dveditz]]

Although I rated this "low" in comparison to malware-installing exploits, it has the potential to generate a blogosphere "0MG tracking!" meltdown.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This would just go away if we fixed bug 407839, right? If not we need to fix this.

Comment 3

[Daniel Veditz [:dveditz]]

Moving out due to lack of progress, but if we get a 1.9 fix in time we'd like a backported patch.

(Actually, is this a problem on trunk? the eTLD service vetting might have prevented this bug there)

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to comment #0)
> globalStorage[] does a domain-like match on partial IP-addresses. They should
> be treated as an indivisible unit (unless and until we scrap this syntax in
> favor of the new spec).
> 
> Given a page at http://63.245.209.10/  (current mozilla.com address)
> 
> javascript:alert(globalStorage['63.245.209.10'])   // should alert
> javascript:alert(globalStorage['245.209.10'])      // should throw
> javascript:alert(globalStorage['x.63.245.209.10']) // should throw
> 
> Currently all three alert (meaning the storage domains are accessible), but the
> latter two should be errors.
> 
> I didn't get a chance to test IPv6 addresses but I bet they're fubar'd too.
> 

As far as I understand the problem, everything that is not a regular domain name OR complete IPv4/IPv6 should be ignored (an exception should be thrown), shouldn't be? Isn't than the right solution to check the parameter using regular expression? For example this is working RE for checking domain in email address:

[a-zA-Z0-9](-?[a-zA-Z0-9])*(\\.[a-zA-Z0-9](-?[a-zA-Z0-9])*)+$

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: Checking IP, v1

This is solution that in case we are in context of an IP address the requested domain must be exactly the same IP address.

I am converting the address string to internal NSPR representation because IPv6 addresses might have ambiguous string representation of the same IP address. Tested for both ipv4 and ipv6 addresses.

Comment 6

[Jeff Walden [:Waldo]]

You should add a Mochitest for this, using an iframe containing a test page running on 127.0.0.1:8888 to run from an IP address.  You can send the test-finished notification using window.postMessage; if you have any questions on doing this, feel free to ask.  The patch in bug 414812 includes a test which uses postMessage in this manner for origin-sensitive testing which may be instructive.

http://developer.mozilla.org/en/docs/DOM:window.postMessage

Not sure when the test would actually be checked in as this is an sg bug, but I don't see how this is substantively different than being able to set a cookie on a TLD or a prefix of an IP address, and while neither behavior is desirable we lived with them for years.

Comment 7

[Daniel Veditz [:dveditz]]

On the trunk globalStorage is supposed to be replaced by "localStorage" (per HTML5/WHAT-WG changes). For compatibility only the current host will be supported for globalStorage[], and this kind of case would throw a security error. Hopefully that change makes it into FF3 which "solves" this bug.

Need the fix for 1.8-branch though, and until the above localStorage change is made might as well have this fix on the trunk just in case.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 301688 [details] [diff] [review]
Checking IP, v1

I'm not a mozilla/dom peer, you should get r= from dcamp

Looks OK to me, though if we weren't supposed to be killing this on the trunk I would think using the effectiveTLD service would be better since you could block things like globalStorage["co.uk"] at the same time. Also you could be even more strict and simply say that if CurrentDomain is an address then we require an exact string match between aCurrentDomain and aRequestedDomain.

sr=dveditz

Comment 9

[Dave Camp (:dcamp)]

Comment on attachment 301688 [details] [diff] [review]
Checking IP, v1

I'm not a peer either, enndeakin has been doing the globalStorage reviews.

Comment 10

[Neil Deakin]

Comment on attachment 301688 [details] [diff] [review]
Checking IP, v1


>+    PRBool domainsEqual = (memcmp(&requestedAddress, &address, sizeof(PRNetAddr)) == 0);
>+    return domainsEqual;
>+  }
>+

As already suggested, can a string direct comparison would be used here, avoiding a second call to PR_StringToNetAddr? Otherwise, just return the result rather than assigning it and then returning it.

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: Checking IP by string-to-string compare + mochitest, TRUNK version

This is temporary fix before localStorage is implemented on TRUNK.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: Checking IP by string-to-string compare, 1.8 BRANCH version

This is fix for 1.8.1 branch.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304469 [details] [diff] [review]
Checking IP by string-to-string compare + mochitest, TRUNK version

Please ask for approval only after you get a review.

Comment 14

[Neil Deakin]

Comment on attachment 304469 [details] [diff] [review]
Checking IP by string-to-string compare + mochitest, TRUNK version


> nsDOMStorageList::CanAccessDomain(const nsAString& aRequestedDomain,
>                                   const nsAString& aCurrentDomain)
> {
>+  PRNetAddr address;
>+  PRStatus status = PR_StringToNetAddr(NS_ConvertUTF16toUTF8(aCurrentDomain).get(), &address);
>+
>+  if (status == PR_SUCCESS) {
>+    // We are currently browsing IP address, requested domain must be the same IP address
>+    // Exact much is the correct approach. We ignore ambigulty of IPv6 addresses

Some spelling errors here (much -> match , ambigulty -> ambiguity)

Better, just say: "An IP address must match exactly" then something about IPv6 addresses, although I don't know what issue you're trying to describe about IPv6 addresses here.

I don't understand the test you included. It seems to append to a message string when an error occurs, but the test never compares the message string to see if it is correct. Better is to post the string after each step indicating whether it passed or failed and have the parent check for 'pass' or 'fail'. Then the parent would end after 6 responses have been received.

Comment 15

[Neil Deakin]

Comment on attachment 304471 [details] [diff] [review]
Checking IP by string-to-string compare, 1.8 BRANCH version

Fix up the comment and the branch patch looks ok.

Comment 16

[Honza Bambas (:mayhemer)]

Attachment: Checking IP by string-to-string compare + mochitest, TRUNK version, v2

I fixed the comment.

And left the test. Because it is exact copy of a proposed test given as exampe to me, please see comment 6. If you wish I can fix both these tests (there is possible solution in some ajax offline test, very complicated IMHO for our such small test), but personally I don't think this is that wrong. The test failure is very well visible in the test results this way. Isn't it what the test should mainly provide?

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: Checking IP by string-to-string compare, 1.8 BRANCH version, v2

Comment 18

[Jeff Walden [:Waldo]]

Exact string comparisons raise my hackles.  Can you add some tests for this behavior in the presence of IDN domains, please, both for accessing via IDN name and via punycode, and for other various cases where this method is called?

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to comment #18)
> Exact string comparisons raise my hackles.  Can you add some tests for this
> behavior in the presence of IDN domains, please, both for accessing via IDN
> name and via punycode, and for other various cases where this method is called?
> 

Hmmm... Exact string comparisons are used only and only if the current domain (i.e. asciiHost of subject principal URI) could be understood as IP address representation. This is not applicable to IDN domains in unicode nor punicode representation.

I was running the only IDN mochitest I found (it passed): dom-level0/test_setting_document.domain_idn.html

Unfortunatelly bug 320568 is still not fixed then there is not a way for me to test this patch on IDNs.

Comment 20

[Jeff Walden [:Waldo]]

(In reply to comment #19)
> Hmmm... Exact string comparisons are used only and only if the current domain
> (i.e. asciiHost of subject principal URI) could be understood as IP address
> representation. This is not applicable to IDN domains in unicode nor punicode
> representation.

I'd prefer if you demonstrated that with tests, not by referring to the actual implementation, which is subject to change over time.

> I was running the only IDN mochitest I found (it passed):
> dom-level0/test_setting_document.domain_idn.html

No, I'm specifically referring to the interactions for the globalStorage-specific functionality.  For example, it should either be possible to either access both of these or neither of these:

  globalStorage["sub1.παράδειγμα.δοκιμή"]
  globalStorage["sub1.xn--hxajbheg2az3al.xn--jxalpdlp"]

> Unfortunatelly bug 320568 is still not fixed then there is not a way for me to
> test this patch on IDNs.

Mochitests can run on pages at IDN hosts, and you shouldn't need bug 320568.  If the available hosts listed at the following URL are insufficient, we can easily add more:

http://developer.mozilla.org/en/docs/Mochitest#How_do_I_test_issues_which_only_show_up_when_tests_are_run_across_domains.3F

Comment 21

[Neil Deakin]

Comment on attachment 304736 [details] [diff] [review]
Checking IP by string-to-string compare + mochitest, TRUNK version, v2

OK, I think I understand the test now. It seems to combine all failures into one message, which is later available in the event's data property.

Comment 22

[Damon Sicore (:damons)]

Comment on attachment 304736 [details] [diff] [review]
Checking IP by string-to-string compare + mochitest, TRUNK version, v2

a1.9+=damons

Comment 23

[Reed Loden [:reed]]

Checking in dom/src/storage/nsDOMStorage.cpp;
/cvsroot/mozilla/dom/src/storage/nsDOMStorage.cpp,v  <--  nsDOMStorage.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in dom/tests/mochitest/bugs/Makefile.in;
/cvsroot/mozilla/dom/tests/mochitest/bugs/Makefile.in,v  <--  Makefile.in
new revision: 1.21; previous revision: 1.20
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/bugs/iframe_bug409349.html,v
done
Checking in dom/tests/mochitest/bugs/iframe_bug409349.html;
/cvsroot/mozilla/dom/tests/mochitest/bugs/iframe_bug409349.html,v  <--  iframe_bug409349.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/bugs/test_bug409349.html,v
done
Checking in dom/tests/mochitest/bugs/test_bug409349.html;
/cvsroot/mozilla/dom/tests/mochitest/bugs/test_bug409349.html,v  <--  test_bug409349.html
initial revision: 1.1
done

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 304737 [details] [diff] [review]
Checking IP by string-to-string compare, 1.8 BRANCH version, v2

approved for 1.8.1.13, a=dveditz for release-drivers

Comment 25

[Honza Bambas (:mayhemer)]

Please commit attachment 304737 [details] [diff] [review] ASAP. Thanks.

Comment 26

[Reed Loden [:reed]]

Checking in dom/src/storage/nsDOMStorage.cpp;
/cvsroot/mozilla/dom/src/storage/nsDOMStorage.cpp,v  <--  nsDOMStorage.cpp
new revision: 1.1.2.11; previous revision: 1.1.2.10
done

Comment 27

[Al Billings [:abillings - ex-MoCo]]

I can't reproduce this problem as described in Comment 0 in Firefox 2.0.0.12.

If I go to http://63.245.209.10/  or even http://63.245.209.10/en-US/, it goes to http://www.mozilla.com/en-US/ on a redirect.

After that, using any of these three:

javascript:alert(globalStorage['63.245.209.10'])   
javascript:alert(globalStorage['245.209.10'])     
javascript:alert(globalStorage['x.63.245.209.10']) 

causes no alert but does throw to the error console. 

Is there a better site to test this with in order to reproduce the problem in 2.0.0.12 (before testing that it is fixed in 2.0.0.13)?

Comment 28

[Alexander Sack]

not a 1.8.0 issue afaics. in case i am wrong, please request blocking1.8.0.15.

Comment 29

[Daniel Veditz [:dveditz]]

(In reply to comment #27)
> After that, using any of these three: [...]
> causes no alert but does throw to the error console. 

Of course, since you ended up on a host that doesn't match the domains you were trying to use in globalStorage.
 
> Is there a better site to test this with in order to reproduce the problem in
> 2.0.0.12 (before testing that it is fixed in 2.0.0.13)?

For some reason any mozilla domain IP address now redirects (even the IP for bugzilla), maybe something to do with the geolocation stuff they put in. Any machine you can reach by IP address will work. 127.0.0.1 if you have a server on your local machine; currently http://209.131.36.158/ works for yahoo; go through your phishing spam and look for raw addresses :-)