Last Comment Bug 368700 - Using TLDs in document.domain should not be allowed
: Using TLDs in document.domain should not be allowed
Status: RESOLVED FIXED
[sg:want P2]
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9alpha4
Assigned To: Wladimir Palant
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on: 331510 368702
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-30 08:12 PST by Wladimir Palant
Modified: 2008-04-18 14:30 PDT (History)
9 users (show)
dveditz: blocking1.8.1.12-
dveditz: blocking1.8.1.13-
dveditz: blocking1.8.1.15-
dveditz: wanted1.8.1.x-
asqueella: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (1.05 KB, text/html)
2007-01-30 09:36 PST, Wladimir Palant
no flags Details
Proposed patch (1.90 KB, patch)
2007-01-30 10:10 PST, Wladimir Palant
bzbarsky: review+
Details | Diff | Splinter Review
Proposed patch with nits fixed (1.95 KB, patch)
2007-04-03 07:24 PDT, Wladimir Palant
trev.moz: review+
dveditz: superreview+
Details | Diff | Splinter Review
Patch to be checked in (1.96 KB, patch)
2007-04-03 14:26 PDT, Wladimir Palant
no flags Details | Diff | Splinter Review

Description Wladimir Palant 2007-01-30 08:12:07 PST
I recently discovered XSS vulnerabilities in myspace.com and live.com/msn.com that could be abused because these sites don't check properly what they write into document.domain. The attack requires a malicious page on a .com domain, it can set its document.domain to "com" and make a vulnerable site loaded in a frame do the same. A variation of this attack is setting document.domain to "com." (you have to use addresses like http://www.myspace.com./ then).

Internet Explorer prevents the first attack, from what I can tell it doesn't allow changing document.domain to something without a dot in it. As to the second attack, exploitation is more difficult in Internet Explorer and Opera since those treat "myspace.com" and "myspace.com." as different domains when it comes to cookies. This makes users of Gecko-based browsers the most vulnerable ones.

While the web sites are certainly at fault here, this issue isn't a rare one according to Google's CodeSearch. And we could fix it relatively easily - don't allow using TLDs in document.domain. Setting document.domain to "localhost" for local testing will still be possible because the value doesn't change. I should have a patch soon.
Comment 1 Wladimir Palant 2007-01-30 09:36:50 PST
Created attachment 253350 [details]
Testcase

This testcase shouldn't be able to set document.domain to "org" or "org." (the latter when opened as https://bugzilla.mozilla.org./attachment.cgi?...).
Comment 2 Wladimir Palant 2007-01-30 10:10:54 PST
Created attachment 253353 [details] [diff] [review]
Proposed patch

This together with the patch in bug 368702 should take care of the issue.
Comment 3 Robert Chapin 2007-01-30 15:58:10 PST
Related to Bug #252342 ?
Comment 4 Wladimir Palant 2007-01-30 16:03:23 PST
How so? Cookie domain checks are a different thing, even though they also require the Effective TLD Service for a correct implementation.
Comment 5 Robert Chapin 2007-01-30 16:29:33 PST
Same issue, in my mind ;)  The guys working on the cookies should have audited all related domain trust issues.  It would be sad to find "Uber certificate signed for .com. valid on entire TLD" or similar bugs that are foreseeable because they need to rely on the same service.  (bad analogy, but you get my point?)  btw, I hope this vulnerability is caused by a CNAME/javabug rather than script injection.
Comment 6 Robert Chapin 2007-01-30 16:41:02 PST
Oops also, "From what I can tell it doesn't allow changing document.domain to something without a dot in it."  Does that mean the attack works just fine on vulnerable co.uk websites?
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2007-02-01 08:08:55 PST
Comment on attachment 253353 [details] [diff] [review]
Proposed patch

>Index: content/html/document/src/nsHTMLDocument.cpp
>+      nsCAutoString str;
>+      PRUint32 tldLength;
>+
>+      CopyUTF16toUTF8(aDomain, str);

Why not:

  NS_ConvertUTF16toUTF8 str(aDomain);

?

(NS_FAILED(tldService->GetEffectiveTLDLength(str, &tldLength)))
>+        return NS_ERROR_FAILURE;

Why not return whatever GetEffeciveTLDLength returned instead?  That would make more sense to me.

With those nits, looks good.
Comment 8 Wladimir Palant 2007-04-03 07:24:01 PDT
Created attachment 260452 [details] [diff] [review]
Proposed patch with nits fixed

Fixed bz's nits, also added a missing null-check in case getting EffectiveTLDService fails.
Comment 9 Daniel Veditz [:dveditz] 2007-04-03 12:31:57 PDT
Comment on attachment 260452 [details] [diff] [review]
Proposed patch with nits fixed

>+      if (!tldService)
>+        return NS_ERROR_FAILURE;

ObNit: NS_ERROR_NOT_AVAILABLE would be more appropriate

>+      nsresult rv = tldService->GetEffectiveTLDLength(str, &tldLength);
>+      if (tldLength < str.Length())

I hate this interface :-(

sr=dveditz
Comment 10 Wladimir Palant 2007-04-03 14:26:49 PDT
Created attachment 260511 [details] [diff] [review]
Patch to be checked in
Comment 11 Nickolay_Ponomarev 2007-04-25 13:49:10 PDT
mozilla/content/html/document/src/nsHTMLDocument.cpp  3.718

Does this need a regression test?
Comment 12 Wladimir Palant 2007-04-25 14:22:53 PDT
A regression test would be good, problem is to get proper testing if the only domain you can use in the test is localhost. nsIDocShell.loadStream would allow to associate our data with an arbitrary URL but it is [noscript].
Comment 13 Brendan Eich [:brendan] 2007-11-13 17:19:25 PST
At least one large portal contacted me expressing the desire for this patch to go into 1.8.0.x / Firefox 2.0.0.x soon.

/be
Comment 14 Daniel Veditz [:dveditz] 2007-12-22 11:29:08 PST
I'm not sure we can get the whole eTLD mechanism checked into the 1.8 branch (although then we could fix the cookie issue, too).
Comment 15 Daniel Veditz [:dveditz] 2008-01-08 16:05:38 PST
brendan: much as it would be great to land eTLD for the branch I'm not sure it's realistic to land at this time. Do you have a volunteer for the work who's not already swamped with FF3 stuff?
Comment 16 Daniel Veditz [:dveditz] 2008-04-18 13:19:22 PDT
Firefox 3 will have this feature and is coming soon. Since it's not a critical security hole, new feature-ish, and potentially site-breaking (should any site be dumb enough to actually use this) it's hard to see how this is in scope for a branch release.

How is a large portal vulnerable? Wouldn't that mean that the site itself is setting document.domain to .com as well?
Comment 17 Wladimir Palant 2008-04-18 14:30:24 PDT
(In reply to comment #16)
> How is a large portal vulnerable? Wouldn't that mean that the site itself is
> setting document.domain to .com as well?

Yes, they were writing a value into document.domain that they didn't validate properly. In case of MySpace, the algorithm was "take current host name and everything following the second last dot is our domain name" - only that you could use "www.myspace.com." as host name (no longer, will redirect to the version without a dot). In case of several Microsoft sites the case was even more primitive - they took a parameter from the query string and put it into document.domain without any validation whatsoever, obviously assuming that the browser will do enough validation already. Apparently, there isn't much awareness that setting document.domain to a random value is dangerous (probably because it shouldn't be - which was exactly the point of this bug).

Anyway, I guess this isn't worth porting to the branch at this time point - especially since we didn't even port effective TLDs yet.

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