Last Comment Bug 414812 - document.domain = "0.0.1" on a page on 127.0.0.1 should not work
: document.domain = "0.0.1" on a page on 127.0.0.1 should not work
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9beta4
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks: 400552
  Show dependency treegraph
 
Reported: 2008-01-30 02:34 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2008-02-07 13:59 PST (History)
5 users (show)
jwalden+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
I think dwitte owes me a Guinness for fixing his mistakes (5.61 KB, patch)
2008-01-31 00:35 PST, Jeff Walden [:Waldo] (remove +bmo to email)
dwitte: review-
Details | Diff | Splinter Review
Good idea (5.27 KB, patch)
2008-02-02 20:03 PST, Jeff Walden [:Waldo] (remove +bmo to email)
dwitte: review+
Details | Diff | Splinter Review
With IDN testcase (19.00 KB, patch)
2008-02-03 02:53 PST, Jeff Walden [:Waldo] (remove +bmo to email)
dwitte: review+
Details | Diff | Splinter Review
With StringEndsWith replaced with a length-check and correctness assertion (19.22 KB, patch)
2008-02-03 20:44 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jwalden+bmo: review+
jst: superreview+
mtschrep: approval1.9+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2008-01-30 02:34:11 PST
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 dwitte@gmail.com 2008-01-30 17:17:06 PST
actually, it does exactly what the comments says it does :p
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2008-01-31 00:35:13 PST
Created attachment 300611 [details] [diff] [review]
I think dwitte owes me a Guinness for fixing his mistakes
Comment 3 dwitte@gmail.com 2008-02-02 18:45:33 PST
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] (remove +bmo to email) 2008-02-02 20:03:00 PST
Created attachment 301067 [details] [diff] [review]
Good idea
Comment 5 dwitte@gmail.com 2008-02-02 20:32:58 PST
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] (remove +bmo to email) 2008-02-03 02:53:22 PST
Created attachment 301105 [details] [diff] [review]
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] (remove +bmo to email) 2008-02-03 02:57:49 PST
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 dwitte@gmail.com 2008-02-03 03:20:29 PST
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] (remove +bmo to email) 2008-02-03 20:44:19 PST
Created attachment 301218 [details] [diff] [review]
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] (remove +bmo to email) 2008-02-06 16:21:14 PST
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 2008-02-06 23:39:39 PST
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] (remove +bmo to email) 2008-02-07 13:59:21 PST
Fixt.

Note You need to log in before you can comment on or make changes to this bug.