Closed Bug 370445 (CVE-2007-0981) Opened 17 years ago Closed 17 years ago

embedded nulls in location.hostname confuse same-origin checks (Zalewski XSS vulnerability)

Categories

(Core :: Networking, defect)

1.8 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: dveditz, Assigned: dveditz)

References

Details

(Keywords: verified1.8.0.10, verified1.8.1.2, Whiteboard: [sg:high] xss)

Attachments

(1 file, 3 obsolete files)

Michal Zalewski sent the following mail to security@mozilla.org, full-disclosure and bugtraq:
- - - - - - - - - - - - - - - - - - - - -
There is a serious vulnerability in Mozilla Firefox, tested with 2.0.0.1,
but quite certainly affecting all recent versions.

The problem lies in how Firefox handles writes to the 'location.hostname'
DOM property. It is possible for a script to set it to values that would
not otherwise be accepted as a hostname when parsing a regular URL -
including a string containing \x00.

Doing this prompts a peculiar behavior: internally, DOM string variables
are not NUL-terminated, and as such, most of checks will consider
'evil.com\x00foo.example.com' to be a part of *.example.com domain. The
DNS resolver, however, and much of the remaining browser code, operates on
ASCIZ strings native to C/C++ instead, treating the aforementioned example
as 'evil.com'.

This makes it possible for evil.com to modify location.hostname as
described above, and have the resulting HTTP request still sent to
evil.com. Once the new page is loaded, the attacker will be able to set
cookies for *.example.com; he'll be also able to alter document.domain
accordingly, in order to bypass the same-origin policy for XMLHttpRequest
and cross-frame / cross-window data access.

A quick demonstration is available here:

  http://lcamtuf.dione.cc/ffhostname.html

If you want to confirm a successful exploitation, check Tools -> Options
-> Privacy -> Show Cookies... for coredump.cx after the test; for the demo
to succeed, the browser needs to have Javascript enabled, and must accept
session cookies.

The impact is quite severe: malicious sites can manipulate authentication
cookies for third-party webpages, and, by the virtue of bypassing
same-origin policy, can possibly tamper with the way these sites are
displayed or how they work.

Regards,
/mz
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
Whiteboard: [sg:high]
The bug is filed under Networking: Cookies. It depends on the code, but the way I understand the bug is that Mozilla thinks the data comes from coredump.cx although it actually comes from dione.cc. Cookies are just one victim of this misbelief, but there are probably more implications.
How many sites use setting 'location.hostname'?
Workaround: deny setting location.hostname.

user_pref("capability.policy.default.Location.hostname.set", "noAccess");
> How many sites use setting 'location.hostname'?

You tell me.  There are stats on this sort of thing out there, last I checked.

We should probably just sanitize location.hostname.  Or rather nsStandardURL::SetHost should do whatever it is that SetSpec does wrt nulls, no?
Component: Networking: Cookies → Networking
QA Contact: networking.cookies → networking
This is cute.  I made a comment an hour or more ago, and it does not appear here.  I can only infer that there is moderation going on (in the strictest sense of the word). Because my training is security, in addition to application development, is my technical assessment less valued?

If it was a burp in the bugzilla software, then "never mind" :-)

I'll stand by my comments - this is a very serious issue, please don't dismiss it or treat it with flip comments.  There are millions of Firefox users, some of them are going to be victimized - not just "at risk".

Thanks for listening.
Comments on this bug and all other bugs on bugzilla.mozilla.org are not moderated in any way. If you posted a comment and it doesn't appear here, there must have been a problem with your submission.
Ah, so SetSpec() doesn't so much deal with this either.  It produces some inconsistent state.  In particular, all the segment offsets/lengths are relative to the original URI string, while the mSpec is truncated at the null due to http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/src/nsStandardURL.cpp&rev=1.95&mark=640#640 at the end of BuildNormalizedSpec.

I think we should consider just throwing from all this stuff if an unexpected null is found after escaping, etc...
Attached patch Stop hostname attack (obsolete) — — Splinter Review
This stops the attack in the bug.

There's some pre-existing weirdness involving setting .hash and .search, but they handle nulls OK and don't affect the domain.
Attachment #255211 - Flags: superreview?(darin.moz)
Attachment #255211 - Flags: review?(bzbarsky)
Attachment #255211 - Flags: approval1.8.1.2?
Attachment #255211 - Flags: approval1.8.0.10?
(In reply to comment #0)
> Once the new page is loaded, the attacker will be able to set
> cookies for *.example.com; he'll be also able to alter document.domain
> accordingly, in order to bypass the same-origin policy for XMLHttpRequest
> and cross-frame / cross-window data access.

This allows document.domain to be set maliciously which can be used to attack pages that explicitly set the same document.domain. That will limit a same-orgin violation attack considerably, although it only takes one document.domain-setting page on a site to make the whole site vulnerable.

XMLHttpRequest is always based on the original location, ignoring any document.domain so this vulnerability does not enable arbitrary XMLHttpRequest attacks. If you find a page that sets document.domain you could inject the XHR into that page and abuse that site, but not sites that do not use document.domain.
Summary: Zalewski cookie stealing / same-domain bypass vulnerability → Zalewski cookie setting / same-domain bypass vulnerability
Blocks: 370501
Oh, yup, you are correct - contrary to some sources, XMLHttpRequest() indeed pays no attention to document.domain in FF. Strike that then. The cross-domain frame attack is somewhat less relevant, anyway: the ability for a malicious site to inject previously acquired, known session cookies into the browser, so that the victim then authenticates within attacker's session, is nasty.

Side note: there are several other unusual problems in how document location updates work. I stumbled upon these, but did not research them in any detail. The one where a script suddenly seems to have document.location in about: namespace may have some implications. A quick demo page can be found there: http://lcamtuf.coredump.cx/fftests.html

Comment on attachment 255211 [details] [diff] [review]
Stop hostname attack

Do we want to address the SetSpec issue too?
> http://lcamtuf.coredump.cx/fftests.html

The "about:" button on that page just gives me an infinite number of alerts.  All of them say "data:....." in current trunk.  Is that the expected "correct" behavior?  Seems a little odd.  ;)  In any case, that testcase seems to be about bug 337260.  Running script in about:blank (which is what happens on branch here) is safe, for what it's worth.
in reply to Comment #2:
> user_pref("capability.policy.default.Location.hostname.set", "noAccess");

(How) can I use this workaround as firefox user? 
Is it accessible via about:config?

Thanks for the help
Attached patch sledgehammer approach to fix ::SetSpec (obsolete) — — Splinter Review
This works for ::SetSpec, but a global check like this kills all nulls and most parts of the URI are perfectly happy to encode them.

Another way to do it would be to rely on the encoding routines for the rest of the spec and check the hostname specifically in BuildNormalizeSpec() right before the NormalizeIDN call.
Assignee: nobody → dveditz
Status: NEW → ASSIGNED
(In reply to comment #12)
> in reply to Comment #2:
> > user_pref("capability.policy.default.Location.hostname.set", "noAccess");
> 
> (How) can I use this workaround as firefox user? 
> Is it accessible via about:config?
> 
> Thanks for the help
> 

Add the line to your user.js file. If you don't have one, create one in notepad for example and name it user.js and put it in your profile directory where prefs.js is located. 


(In reply to comment #14)
> (In reply to comment #12)
> > in reply to Comment #2:
> > > user_pref("capability.policy.default.Location.hostname.set", "noAccess");
> > 
> > (How) can I use this workaround as firefox user? 
> > Is it accessible via about:config?
> > 
> > Thanks for the help
> > 
> 
> Add the line to your user.js file. If you don't have one, create one in notepad
> for example and name it user.js and put it in your profile directory where
> prefs.js is located. 
> 

Thanks a lot,
before adding a user.js, I had a look in file prefs.js and there was the line already!
  My first intention (before comment #12) was to add it in about:config via
  context popup > new > string
  name: capability.policy.default.Location.hostname.set
  value: noAccess
this seems to work, at least I think so.(?)
Attached patch more limited null squashing in setSpec (obsolete) — — Splinter Review
I think I like this better than attachment 255245 [details] [diff] [review] -- nulls in other parts of the URI get escaped to %00 as before, just in case anyone's relying on that behavior.
Attachment #255245 - Attachment is obsolete: true
Comment on attachment 255249 [details] [diff] [review]
more limited null squashing in setSpec

We don't need both patches, either the narrow first one or this slightly broader one. sticking different reviewers on just to get more attention.
Attachment #255249 - Flags: superreview?(jst)
Attachment #255249 - Flags: review?(dbaron)
Attachment #255249 - Flags: approval1.8.1.2?
Attachment #255249 - Flags: approval1.8.0.10?
Comment on attachment 255245 [details] [diff] [review]
sledgehammer approach to fix ::SetSpec

>Index: netwerk/base/src/nsSimpleURI.cpp
> NS_IMETHODIMP
> nsSimpleURI::SetScheme(const nsACString &scheme)
> {
>+    const nsPromiseFlatCString &flat = PromiseFlatCString(scheme);
>+    if (!net_IsValidScheme(flat)) {
>+        NS_ERROR("the given url scheme contains invalid characters");
>+        return NS_ERROR_UNEXPECTED;
>+    }
>+

I wonder whether this should be NS_ERROR_MALFORMED_URI.

It seems like nsSimpleURI::SetSpec should probably also have a net_IsValidScheme check -- and maybe even additional null-byte checks -- although I'm not sure what the behavior of NS_EscapeURL is on embedded nulls (does it escape them?).  From a quick glance it seems like nsSimpleURI stores the escaped URL and nsStandardURL stores the unescaped URL, although I'm not looking too closely (yet).

>Index: netwerk/base/src/nsStandardURL.cpp
>+    if (strlen(spec) < specLength)
>+        return NS_ERROR_UNEXPECTED; // embedded null
>+
>     // filter out unexpected chars "\r\n\t" if necessary
>     nsCAutoString buf1;
>     if (net_FilterURIString(spec, buf1)) {

I wonder whether this should be NS_ERROR_MALFORMED_URI, and also whether we should instead just make net_FilterURIString filter out the nulls just like the other chars in the code quoted.

>+    if (host && strlen(host) < flat.Length())
>+        return NS_ERROR_UNEXPECTED; // found embedded null

Likewise about the error code.

>Index: netwerk/base/src/nsURLHelper.cpp
>     for (; schemeLen && *scheme; ++scheme, --schemeLen) {
>         if (!(nsCRT::IsAsciiAlpha(*scheme) ||
>               nsCRT::IsAsciiDigit(*scheme) ||
>               *scheme == '+' ||
>               *scheme == '.' ||
>               *scheme == '-'))
>             return PR_FALSE;
>     }
> 
>+    if (schemeLen != 0)  // found embedded null
>+        return PR_FALSE;

It would be simpler to just replace:

>-    for (; schemeLen && *scheme; ++scheme, --schemeLen) {
>+    for (; schemeLen; ++scheme, --schemeLen) {

and maybe comment that you want to forbid embedded nulls.
Attachment #255245 - Attachment is obsolete: false
But, yeah, I think I like the third patch better than the second, and I think that makes my net_FilterURIString comment obsolete.
Attached patch Third patch + dbaron's comments — — Splinter Review
nsSimpleURI::SetSpec was null-safe because of the NS_EscapeURL() call (esc_OnlyNonAscii escapes 00-1f also), but I added the net_IsValidScheme check anyway because the current code accepts other invalid schemes like "me$$y:". Not strictly part of this bug and probably wasn't harmful.

Other than that I changed the error codes and the for() condition in net_IsValidScheme.
Attachment #255211 - Attachment is obsolete: true
Attachment #255245 - Attachment is obsolete: true
Attachment #255249 - Attachment is obsolete: true
Attachment #255211 - Flags: superreview?(darin.moz)
Attachment #255211 - Flags: review?(bzbarsky)
Attachment #255211 - Flags: approval1.8.1.2?
Attachment #255211 - Flags: approval1.8.0.10?
Attachment #255249 - Flags: superreview?(jst)
Attachment #255249 - Flags: review?(dbaron)
Attachment #255249 - Flags: approval1.8.1.2?
Attachment #255249 - Flags: approval1.8.0.10?
Attachment #255252 - Flags: superreview?(dbaron)
Attachment #255252 - Flags: review?(bzbarsky)
Attachment #255252 - Flags: approval1.8.1.2?
Attachment #255252 - Flags: approval1.8.0.10?
Comment on attachment 255252 [details] [diff] [review]
Third patch + dbaron's comments

r=bzbarsky given that we need a fix on branches, but I do think that darin or biesi should look at this too... they know this code a lot better than I do.
Attachment #255252 - Flags: review?(bzbarsky) → review+
Comment on attachment 255252 [details] [diff] [review]
Third patch + dbaron's comments

>+    // nsCStrings may have embedded nulls -- reject those too

might want to say "counted strings" rather than "nsCStrings"


Anyway, sr=dbaron, although I'd also say it would be good to get darin and/or biesi to look at this when possible.  (e.g., I don't really know whether you should be returning NS_ERROR_UNEXPECTED or NS_ERROR_MALFORMED_URI or whether it matters, etc.)
Attachment #255252 - Flags: superreview?(dbaron) → superreview+
NS_ERROR_MALFORMED_URI seems the logical choice to me, NS_ERROR_INVALID_HOSTNAME never made it into the tree.
Comment on attachment 255252 [details] [diff] [review]
Third patch + dbaron's comments

Approved for both branches, a=jay
Attachment #255252 - Flags: approval1.8.1.2?
Attachment #255252 - Flags: approval1.8.1.2+
Attachment #255252 - Flags: approval1.8.0.10?
Attachment #255252 - Flags: approval1.8.0.10+
Comment on attachment 255252 [details] [diff] [review]
Third patch + dbaron's comments

Would like an eventual look-see from Darin on this.
Attachment #255252 - Flags: review?(darin.moz)
Fix checked into trunk, 1.8 and 1.8.0 branches
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment on attachment 255252 [details] [diff] [review]
Third patch + dbaron's comments

r=darin

could similar tricks be played with the userpass field?
Attachment #255252 - Flags: review?(darin.moz) → review+
> could similar tricks be played with the userpass field?

darin, do you mean by embedding nulls into the value passed into location.href?

> darin, do you mean by embedding nulls into the value passed into location.href?

yes, or via XMLHttpRequest.open
0 in window.open() terminates the whole string.
0 in iframe.src sets the location to about:blank on latest trunk but src still contains 0.
If you try to mess with the userpass by setting location.href it gets escaped in BuildNormalizedSpec. If you try to set a null-containing userpass using location.hostname it gets caught and rejected.
(In reply to comment #30)
> 0 in iframe.src sets the location to about:blank on latest trunk but src still
> contains 0.
> 

Does this have any effect on the effectiveness of the patch? Or is this not a security issue?
(In reply to comment #9)
> http://lcamtuf.coredump.cx/fftests.html
> 

Are all of these issues with locations.hostname? Are number 3 and 4 anything to worry about, or has no one investigated those two?
#1 (both versions) is just an "oddity" as the page says, perfectly safe.
perhaps we should reconsider our policy of showing nothing when the location is "about:blank", or at least if the document is not completely empty. In normal use modified "about:blank" pages are in frames with nothing that would show their location anyway.

#2 is a bug, but harmless (neutered privileges)

#3 is fixed by this patch.

#4 is a JavaScript performance problem dealing with strings 131072 characters long.
(In reply to comment #34)
> #1 (both versions) is just an "oddity" as the page says, perfectly safe.
> perhaps we should reconsider our policy of showing nothing when the location is
> "about:blank", or at least if the document is not completely empty. In normal
> use modified "about:blank" pages are in frames with nothing that would show
> their location anyway.
> 
> #2 is a bug, but harmless (neutered privileges)
> 
> #3 is fixed by this patch.
> 
> #4 is a JavaScript performance problem dealing with strings 131072 characters
> long.
> 
Thanks, just wanted to make sure everything was being honored here.
> #4 is a JavaScript performance problem dealing with strings 131072 characters
> long.

I can't even reproduce the issue on trunk; is that problem branch-only (or only if error pages are disabled) or something?
(In reply to comment #34)
> #1 (both versions) is just an "oddity" as the page says, perfectly safe.

Actually, probably not, see my demos at:
http://lcamtuf.coredump.cx/ffblank/

But I filed this as a separate bug 370555.

/mz
(In reply to comment #34)
> #4 is a JavaScript performance problem dealing with strings 131072 characters
> long.

No, it's not a JS performance problem.

/be
v.fixed on both branches with:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.2) Gecko/20070215 Firefox/2.0.0.2
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.10) Gecko/20070216 Firefox/1.5.0.10

Original exploit testcase http://lcamtuf.dione.cc/ffhostname.html and test #3 from http://lcamtuf.coredump.cx/fftests.html both pass with the latest rc builds.  

Tests #1 and #2 from http://lcamtuf.coredump.cx/fftests.html are being discussed in bug 370555 and test #4 needs more investigation to determine whether it is an JS issue or something else.
(In reply to comment #38)
> No, it's not a JS performance problem.

It's a performance problem with code written in JavaScript, did not mean to impugn the JS engine itself.

It appears to be in the safebrowsing code, the stack always seems to have nsXPCnsUrlClassifierCallbackWrapper::HandleEvent in it. Setting browser.safebrowsing.enabled to false seems to clear the problem.
(In reply to comment #34)
> #2 is a bug, but harmless (neutered privileges)

[...fetting off-topic for that bug entry, but hey...]

This one might be not that harmless after all, either. The general problem seems to be that when a "blocking" Javascript event is taking place (alert(), prompt(), synchronous XMLHttpRequest.open, etc), other scripts running at that time will have limited permissions, including browser code - that nevertheless retains some of its privileges because of the namespace it originates from.

Consider this: body onload handler on a malicious page updates document.location to foo.cgi, which pauses for 1 s, then returns application/octet-stream .exe file; the same handler also sets a 100ms timeout to a function that issues a synchronous XMLHttpRequest.open() to bar.cgi, which pauses for, say, 2 seconds, and returns some text; as a result, a toothless but still partly operational download prompt will be rendered (the "privileged" code will be not able to declare event handlers, access keys, etc - disabling some of the anti-phishing security measures - but will be able to perform actions such as running an external application). Although this example is not particularly "exploitable", races like this are likely to have security implications.

Browser code aside, two unrelated pages can apparently interfere with the ability for the other page to carry out certain actions without getting NS_ERROR_DOM_XPCONNECT_ACCESS_DENIED, no ability to create event handlers, access keys, etc. 
> The general problem seems to be that when a "blocking" Javascript event is
> taking place (alert(), prompt(), synchronous XMLHttpRequest.open, etc)

I'm curious that you're lumping those together.  While synchronous XMLHttpRequest does have the problem you describe (see bug 326777) it should not be an issue for alert() and prompt().
I filed the performance bug as bug 370860.
I tried the workaround putting 
user_pref("capability.policy.default.Location.hostname.set", "noAccess");
in user.js in ~/library/Mozilla/Profiles/default/vg9m3khy.slt/
but M.Zalewski's still shows FF as vulnerable even after restarting.
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-GB; rv:1.8.0.9) Gecko/20061206 IE
Hope the update is available soon.
Thanks for the hard work on this.
BTW I assume 'document.location=' is covered by the same fix.
(In reply to comment #43)
> I'm curious that you're lumping those together.  While synchronous
> XMLHttpRequest does have the problem you describe (see bug 326777) it should
> not be an issue for alert() and prompt().

I can't see that and many some security-related bugs mentioned here and in related reports, sorry. The example given in my fftests.html testcase seems to affect about:neterr in a very similar manner through alert('') popup.

Summary: Zalewski cookie setting / same-domain bypass vulnerability → embedded nulls in location.hostname confuse same-origin checks (Zalewski XSS vulnerability)
Whiteboard: [sg:high] → [sg:high] xss
I tried the quick demo of this bug in comment #1, and got a return of "YOUR BROWSER IS VULNERABLE" in my SeaMonkey 1.1 browser.  So Firefox doesn't stand alone on this, but I am surprised that no mention was made of SeaMonkey in the 46 comments already entered.
All the code in question is shared, so there's no need to talk about every single Gecko-based browser separately.
This would be obvious to the persons working on this bug, the "in" crowd if you will.  But for less technical, less involved users I don't think this would be a clear assumption; I certainly didn't take it for granted.  

You say all the code 'in question' is shared, but that implies that some other code may not be common among all browsers, and if that is true, if the different Mozilla family browsers differ in some respects, in some code segments, then I don't think it's reasonable to expect all readers of these bug reports to know the difference.  It seems a small addition to clarify if a bug affects all in the family, or spell out which ones are included.
Fred Yontze, please read https://bugzilla.mozilla.org/page.cgi?id=etiquette.html and study http://www.mozilla.org/bugs/ -- you should also read https://bugzilla.mozilla.org/describecomponents.cgi if you haven't yet -- and stop spamming this bug.

This bug is in the Core product, Networking component. That's enough for anyone who is at all familiar with bugzilla and Mozilla's use of it to know this bug is about the shared "core" code at the center of all XUL applications. Your job as a newcomer is to learn this other than by complaining in comments in bugs.

/be
(Fred: sorry for misspelling your name -- typo)

/be
Alias: CVE-2007-0981
Flags: in-testsuite?
> seems to affect about:neterr in a very similar manner through alert('') popup.

Indeed.  I filed bug 371858 on that issue.
You need to log in before you can comment on or make changes to this bug.