Can get globalStorage objects for partial IP addresses

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
DOM
P3
normal
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: dveditz, Assigned: mayhemer)

Tracking

({fixed1.8.1.13})

1.8 Branch
mozilla1.9beta4
fixed1.8.1.13
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.13 +
wanted1.8.1.x +
blocking1.8.0.next -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low] data promiscuity)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Updated

10 years ago
Flags: wanted1.8.1.x+
Flags: blocking1.9?
Flags: blocking1.8.1.12?
Whiteboard: [sg;low] data promiscuity
(Reporter)

Comment 1

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

Updated

10 years ago
Whiteboard: [sg;low] data promiscuity → [sg:low] data promiscuity
This would just go away if we fixed bug 407839, right? If not we need to fix this.
Assignee: nobody → dcamp
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
(Reporter)

Comment 3

10 years ago
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)
Flags: blocking1.8.1.12? → blocking1.8.1.13?
(Assignee)

Updated

10 years ago
Assignee: dcamp → honzab
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

10 years ago
(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])*)+$
(Assignee)

Comment 5

9 years ago
Created attachment 301688 [details] [diff] [review]
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.
Attachment #301688 - Flags: review?(dveditz)
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.
(Reporter)

Comment 7

9 years ago
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.
Version: unspecified → 1.8 Branch
(Reporter)

Comment 8

9 years ago
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
Attachment #301688 - Flags: superreview+
Attachment #301688 - Flags: review?(dveditz)
Attachment #301688 - Flags: review?(dcamp)

Comment 9

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

I'm not a peer either, enndeakin has been doing the globalStorage reviews.
Attachment #301688 - Flags: review?(dcamp) → review?(enndeakin)
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.
(Assignee)

Comment 11

9 years ago
Created attachment 304469 [details] [diff] [review]
Checking IP by string-to-string compare + mochitest, TRUNK version

This is temporary fix before localStorage is implemented on TRUNK.
Attachment #301688 - Attachment is obsolete: true
Attachment #304469 - Flags: review?(enndeakin)
Attachment #304469 - Flags: approval1.9?
Attachment #301688 - Flags: review?(enndeakin)
(Assignee)

Comment 12

9 years ago
Created attachment 304471 [details] [diff] [review]
Checking IP by string-to-string compare, 1.8 BRANCH version

This is fix for 1.8.1 branch.
Attachment #304471 - Flags: review?(enndeakin)
Attachment #304471 - Flags: approval1.8.1.13?
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.
Attachment #304469 - Flags: approval1.9?
(Assignee)

Updated

9 years ago
Attachment #304471 - Flags: approval1.8.1.13?
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.
Attachment #304469 - Flags: review?(enndeakin) → review-
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.
Attachment #304471 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 16

9 years ago
Created attachment 304736 [details] [diff] [review]
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?
Attachment #304469 - Attachment is obsolete: true
Attachment #304736 - Flags: review?(enndeakin)
(Assignee)

Comment 17

9 years ago
Created attachment 304737 [details] [diff] [review]
Checking IP by string-to-string compare, 1.8 BRANCH version, v2
Attachment #304471 - Attachment is obsolete: true
Attachment #304737 - Flags: review?(enndeakin)
(Assignee)

Updated

9 years ago
Attachment #304736 - Attachment description: Checking IP by string-to-string compare + mochitest, TRUNK version → Checking IP by string-to-string compare + mochitest, TRUNK version, v2
(Assignee)

Updated

9 years ago
Attachment #304737 - Flags: review?(enndeakin) → approval1.8.1.13?
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?
Flags: in-testsuite?
(Assignee)

Comment 19

9 years ago
(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.
(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
(Reporter)

Updated

9 years ago
Flags: blocking1.8.1.13? → blocking1.8.1.13+
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.
Attachment #304736 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

9 years ago
Attachment #304736 - Flags: approval1.9?
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Comment on attachment 304736 [details] [diff] [review]
Checking IP by string-to-string compare + mochitest, TRUNK version, v2

a1.9+=damons
Attachment #304736 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
(Reporter)

Comment 24

9 years ago
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
Attachment #304737 - Flags: approval1.8.1.13? → approval1.8.1.13+
(Assignee)

Comment 25

9 years ago
Please commit attachment 304737 [details] [diff] [review] ASAP. Thanks.
Keywords: checkin-needed
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
Keywords: checkin-needed → fixed1.8.1.13
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

9 years ago
not a 1.8.0 issue afaics. in case i am wrong, please request blocking1.8.0.15.
Flags: blocking1.8.0.15-
(Reporter)

Comment 29

9 years ago
(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 :-)
(Reporter)

Updated

9 years ago
Group: security
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.