414812 - document.domain = "0.0.1" on a page on 127.0.0.1 should not work

Status: RESOLVED FIXED

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

Description

[Jeff Walden [:Waldo]]

There's a comment in the code for nsHTMLDocument::SetDomain which claims this should fail due to a downstream component's edge-case checking, but this wasn't tested in the patch which introduced this code, and it is broken.

I might or might not take this; not sure whether I have the time just now.  It shouldn't be a difficult patch, so don't take this as preventing you from fixing this, dwitte.  :-)

Comment 1

[[e-mail]]

actually, it does exactly what the comments says it does :p

Comment 2

[Jeff Walden [:Waldo]]

Attachment: I think dwitte owes me a Guinness for fixing his mistakes

Comment 3

[[e-mail]]

Comment on attachment 300611 [details] [diff] [review]
I think dwitte owes me a Guinness for fixing his mistakes

>+    PRStatus result = PR_StringToNetAddr(current.get(), &dummy);
>+    if (result == PR_SUCCESS)
>+      return NS_ERROR_DOM_BAD_DOCUMENT_DOMAIN;

hmm. so, this call isn't always cheap (and in most cases pokes the OS). perhaps a better approach would be to replace the current GetBaseDomain() call with one that grabs the base domain of |uri|, then makes sure |domain| is equal to or a subdomain of that. the IP address test will then be folded in, and we only do it once. (extra string fu - but that's cheap, compared.)

nice tests. ;)

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Good idea

Comment 5

[[e-mail]]

Comment on attachment 301067 [details] [diff] [review]
Good idea

>+    // We're golden if the new domain is a subdomain of the current domain.

hmm, you mean superdomain, right?

>-    nsCAutoString baseDomain;
>-    ok = NS_SUCCEEDED(tldService->GetBaseDomain(newURI, 0, baseDomain));
>+    nsCAutoString currentBaseDomain;
>+    if (NS_FAILED(tldService->GetBaseDomain(uri, 0, currentBaseDomain)))
>+      return NS_ERROR_NOT_AVAILABLE;
>+    ok = StringEndsWith(domain, currentBaseDomain);
>   }
>   if (!ok) {
>     // Error: illegal domain
>     return NS_ERROR_DOM_BAD_DOCUMENT_DOMAIN;
>   }

we should probably still return NS_ERROR_DOM_BAD_DOCUMENT_DOMAIN on GetBaseDomain() failure, since in most cases it really does mean the domain was bad. (in which case, just say |ok = NS_SUCCEEDED(...) && StringEndsWith(...);|?)

also, a bit above this, where we get |current| and |domain|, you'll need to use GetAsciiHost() instead of GetHost() to have consistent normalization with |currentBaseDomain|. (worth rolling some utf8 chars into the test, to catch this? see netwerk/test/unit/test_idnservice.js if you want some utf8/punycode hosts to steal.)

r=me with fixes. two guinnesses now :)

Comment 6

[Jeff Walden [:Waldo]]

Attachment: With IDN testcase

(In reply to comment #5)
> >+    // We're golden if the new domain is a subdomain of the current domain.
> 
> hmm, you mean superdomain, right?

What I meant was subdomain of the base domain of the page -- adjusted comment to reflect this.


> we should probably still return NS_ERROR_DOM_BAD_DOCUMENT_DOMAIN on
> GetBaseDomain() failure, since in most cases it really does mean the domain
> was bad.

I hope we can't hit this situation, because it means that the stored domain was bad!  Reducing the number of possible thrown assertions is good, tho, so I made the change.


> also, a bit above this, where we get |current| and |domain|, you'll need to
> use GetAsciiHost() instead of GetHost() to have consistent normalization
> with |currentBaseDomain|. (worth rolling some utf8 chars into the test, to
> catch this? see netwerk/test/unit/test_idnservice.js if you want some
> utf8/punycode hosts to steal.)

Aha, *very* observant.  Given that I aim for only "safe" domains (ones that aren't in use on the Internet, or used in any meaningful manner) to be proxied, I couldn't use the ones there, so I had to add more.  :-)  Our whitelisting also monkey-wrenches this matter quite a bit -- *really* want to fix bug 414090 right now (five hours ago, too).


> r=me with fixes. two guinnesses now :)

I think I shall have to return the favor at least once (in kind?) for the GetAsciiHost observation.  :-)

Comment 7

[Jeff Walden [:Waldo]]

Gerv: you added the IDN TLDs to the whitelist temporarily a few months ago.  Given the type of tests I needed to do here, I took advantage of the Greek one's existence.  :-)  Before you remove it eventually I'll need to make some changes to the test or it'll break tinderboxen (unless bug 414090 is fixed before then), just so you know.  I'm also adding a comment to this effect in all.js as another reminder when you make that change.  :-)

Comment 8

[[e-mail]]

Comment on attachment 301105 [details] [diff] [review]
With IDN testcase

>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================
>+    nsCAutoString currentBaseDomain;
>+    ok = NS_SUCCEEDED(tldService->GetBaseDomain(uri, 0, currentBaseDomain)) &&
>+         StringEndsWith(domain, currentBaseDomain);

strictly speaking the StringEndsWith could be replaced by a simple length comparison, since the string compare was already done above - but it's insignificant and i think i like the robustness of doing a string compare better (since, as you know, IDN issues are legion).

r=me.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: With StringEndsWith replaced with a length-check and correctness assertion

I wasn't going to do this originally, but I had enough time to rethink the decision that I changed my mind.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 301218 [details] [diff] [review]
With StringEndsWith replaced with a length-check and correctness assertion

I believe this completes the necessary modifications to make it impossible to set |document.domain| to a bogus host (the other being the eTLD stuff that's already landed).

Comment 11

[Mike Schroepfer]

Comment on attachment 301218 [details] [diff] [review]
With StringEndsWith replaced with a length-check and correctness assertion

MochiTests make me happy!

Comment 12

[Jeff Walden [:Waldo]]

Fixt.