Last Comment Bug 141061 - XMLHttpRequest allows reading of local files
: XMLHttpRequest allows reading of local files
Status: VERIFIED FIXED
[ADT1][m5+][fixed-trunk]
: topembed+
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.0
Assigned To: Darin Fisher
: Hixie (not reading bugmail)
:
Mentors:
http://sec.greymagic.com/adv/gm001-ns/
: 141208 141348 141453 141551 141727 141755 (view as bug list)
Depends on:
Blocks: 138000
  Show dependency treegraph
 
Reported: 2002-04-29 17:46 PDT by Giscard Girard
Modified: 2013-04-04 13:53 PDT (History)
48 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove xmlextras from configure.in (784 bytes, patch)
2002-04-30 09:19 PDT, timeless
cls: review+
Details | Diff | Splinter Review
calls document.load('redir.asp') (1.23 KB, text/html)
2002-04-30 11:06 PDT, Giscard Girard
no flags Details
redirects to file:///c:/test.xml (75 bytes, application/octet-stream)
2002-04-30 11:06 PDT, Giscard Girard
no flags Details
patch - solution #2 (2.08 KB, patch)
2002-04-30 11:41 PDT, Darin Fisher
no flags Details | Diff | Splinter Review
patch - same thing, with equivalent code commented out of nsDocLoader.cpp (3.28 KB, patch)
2002-04-30 12:10 PDT, Darin Fisher
security-bugs: review+
hjtoi-bugzilla: superreview+
asa: approval+
Details | Diff | Splinter Review

Description Giscard Girard 2002-04-29 17:46:36 PDT
When an http server redirects the user to a local file, XMLHttpRequest gets
tricked into thinking the page came from the http server.
Comment 1 Matthias Versen [:Matti] 2002-04-29 18:04:52 PDT
i can confirm this with win2k build 20020428..
Comment 2 Mitchell Stoltz (not reading bugmail) 2002-04-29 18:14:51 PDT
I am marking this bug Security-Sensitive. That means only people in the Bugzilla
security group and everyone whose email appears in the fields of the bug
(reporter, etc.) will be able to view it. If anyone objects, simply uncheck the
box above.
Comment 3 Giscard Girard 2002-04-29 18:32:32 PDT
The folks at greymagic found this bug.  Just so there's no confusion, I take no
responsability for finding this bug.
Comment 4 Bradley Baetz (:bbaetz) 2002-04-30 03:05:12 PDT
BTW: http://www.theregister.co.uk/content/4/25079.html

It looks like the problem is that http doesn't do any security handling itsself.
Instead, it relies on docshell to call CheckLoadURI in its OnRedirect handler -
see http://lxr.mozilla.org/seamonkey/source/uriloader/base/nsDocLoader.cpp#1270

darin, Mitch - should http be doing its own security checks? Is there ever a
case where we don't want these run?
Comment 5 Bradley Baetz (:bbaetz) 2002-04-30 03:37:40 PDT
In 1.0RC1, this test case crashes for me on linux. A local test case sometimes
crashes, and sometimes hangs, depending on the details of my redirect. I don't
have a branch debug build to work our where this is happening.

On the trunk, using that test page, I can load chrome urls, and have the text
appear. I can't load js ones, though, because the channel doesn't provide an
nsIInterfaceRequestor for us to get teh script global from, so I hit the
assertion in nsJSThunk::EvaluateScript.
Comment 6 Mike Shaver (:shaver -- probably not reading bugmail closely) 2002-04-30 08:08:40 PDT
Any point in keeping this closed now that the Register has pasted the (trivial)
sample code, as well as the GreyMagic advisory page having a working demonstration?
Comment 7 Mike Shaver (:shaver -- probably not reading bugmail closely) 2002-04-30 09:00:52 PDT
cls and timeless are going to turn off xmlextras in the default build until we
have a fix, so I'm adding them to the cc:.  Whatever fix-patch we end up with
can tweak configure.in again.
Comment 8 timeless 2002-04-30 09:19:30 PDT
Created attachment 81685 [details] [diff] [review]
remove xmlextras from configure.in
Comment 9 cls 2002-04-30 10:55:18 PDT
Comment on attachment 81685 [details] [diff] [review]
remove xmlextras from configure.in

Noted on IRC but for the record, r=cls.

FYI, this change is going to remove xmlextras from the build completely.  To
enable it in a build, you'll have to explicitly specify it: e.g.
--enable-extensions=all,xmlextras
Comment 10 timeless 2002-04-30 11:01:58 PDT
cc'ing heikki the module owner for xmlextras
Comment 11 Giscard Girard 2002-04-30 11:06:09 PDT
Created attachment 81704 [details]
calls document.load('redir.asp')
Comment 12 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-04-30 11:06:24 PDT
*** Bug 141208 has been marked as a duplicate of this bug. ***
Comment 13 Giscard Girard 2002-04-30 11:06:57 PDT
Created attachment 81705 [details]
redirects to file:///c:/test.xml
Comment 14 Giscard Girard 2002-04-30 11:10:38 PDT
This bug is also affected by document.load().  To test this 
put load.html and redir.asp on a webserver.  
point your browser to http://<yourwebserver>/load.html.  
load.html will display c:/test.xml in your browser.
Comment 15 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-04-30 11:12:14 PDT
I don't think there is point in keeping this secret, it is public knowledge now
(articles at The Register etc.).

This also affects document.load(), which you cannot disable by disabling
xmlextras from the build. This needs a real fix.

Marking nsbeta1+ and topembed+ (pretty sure Jud would not mind;).
Comment 16 Darin Fisher 2002-04-30 11:20:59 PDT
two solutions:

1) make sure we have an nsIHttpEventSink::OnRedirect implementation whenever
loading http URLs.

2) make http call CheckLoadURI itself.

i'd prefer to go the route of solution 1 if at all possible.  i know it's
riskier, but it would keep things more modular (necko would have to depend on caps).
Comment 17 Darin Fisher 2002-04-30 11:41:50 PDT
Created attachment 81716 [details] [diff] [review]
patch - solution #2

this patch ensures that CheckLoadURI is called for all HTTP redirects.
Comment 18 Darin Fisher 2002-04-30 11:44:36 PDT
can people try out this patch and verify that it does indeed fix this bug?  thx!
Comment 19 Darin Fisher 2002-04-30 11:59:22 PDT
ok, i verified this patch against an XMLHttpRequest based exploit.

if inside the netscape firewall, see http://unagi.mcom.com/~darinf/test.html

if you have a local file on your system with the URL file:///tmp/test.txt,
you'll see it's contents appear in an alert box when you load test.html.  if you
apply my patch, you'll no longer see any text in the alert box.
Comment 20 Darin Fisher 2002-04-30 12:08:04 PDT
-> me
Comment 21 Darin Fisher 2002-04-30 12:10:17 PDT
Created attachment 81725 [details] [diff] [review]
patch - same thing, with equivalent code commented out of nsDocLoader.cpp

this patch comments out the redundant code in nsDocLoader.cpp
Comment 22 scottputterman 2002-04-30 12:27:44 PDT
adding the to beta stopper list.  Who should r=/sr= this?
Comment 23 Dawn Endico 2002-04-30 12:41:11 PDT
undoing the keyword, whiteboard and priority changes i wiped away with
my last change. sorry. readding nsbeta1+ and topembed+ because it looks
like those were removed accidently as well.
Comment 24 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-04-30 13:02:16 PDT
I tried to make document.load() tests for Apache, but they don't seem to work.
These are on an internal Netscape server (Netscape people should be able to log
in normally if you want to tweak things).

http://green.nscp.aoltw.net/heikki/141061/load-win.html (c:\temp\test.txt)
http://green.nscp.aoltw.net/heikki/141061/load-unix.html (/tmp/test.txt)

Anyone who could test the load tests from Giscard with Darin's patch?
Comment 25 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-04-30 13:05:46 PDT
Oops, sorry, I can confirm this fixes the document.load() bug on Windows as well.
Comment 26 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-04-30 13:07:17 PDT
To test the load() tests, place test.xml file on your local disc.
c:\temp\test.xml for Windows and /tmp/test.xml for Unix.

That test file could be something as simple as: <doc>Foobar</doc>
Comment 27 Mitchell Stoltz (not reading bugmail) 2002-04-30 13:24:09 PDT
Comment on attachment 81725 [details] [diff] [review]
patch - same thing, with equivalent code commented out of nsDocLoader.cpp

Looks good. r=mstoltz
Comment 28 scottputterman 2002-04-30 13:26:38 PDT
putting back on the beta stopper list.
Comment 29 Mitchell Stoltz (not reading bugmail) 2002-04-30 15:02:19 PDT
I'd like to keep this bug security-confidential for another day or two. If
anyone objects, please send mail to security-group@mozilla.org and we'll discuss
it there - NOT here in the bug.
Comment 30 timeless 2002-04-30 15:05:46 PDT
patch manager is really good at bad collisions...
Comment 31 Mitchell Stoltz (not reading bugmail) 2002-04-30 15:39:31 PDT
On second thought, let's open up the bug. Transparency is good.
Comment 32 Bradley Baetz (:bbaetz) 2002-04-30 15:42:58 PDT
Has anyone got a debug branch build to see if this fixes the crash, too?
Comment 33 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-04-30 15:48:59 PDT
Comment on attachment 81725 [details] [diff] [review]
patch - same thing, with equivalent code commented out of nsDocLoader.cpp

sr=heikki
Comment 34 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-04-30 15:52:59 PDT
The crash was another issue, it has been fixed in XMLHttpRequest on the trunk &
branch. Harishd just made a patch for document.load() case. Basically the crash
occurred when those objects were used to load non-XML data, especially if they
contained stylesheets.

I am making a testcase to see if this fixes the case of redirecting stylesheets.
Comment 35 Asa Dotzler [:asa] 2002-04-30 15:58:15 PDT
Comment on attachment 81725 [details] [diff] [review]
patch - same thing, with equivalent code commented out of nsDocLoader.cpp

a=asa (on behalf of drivers) for checkin to the 1.0 branch
Comment 36 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-04-30 16:07:14 PDT
Create test.css file with contents

p {color:red;}

and place it in c:\temp\test.css, then point your browser into this URL

http://green.nscp.aoltw.net/heikki/141061/style-win.html

Confirmed that this patch fixes that as well. This bug was reported on bugtraq.
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2002-04-30 16:27:07 PDT
*** Bug 141348 has been marked as a duplicate of this bug. ***
Comment 38 Darin Fisher 2002-04-30 16:29:01 PDT
fixed-on-trunk
Comment 39 scottputterman 2002-04-30 17:15:09 PDT
adding adt1.0.0+.  Please check this into the branch as soon as possible and add
the fixed1.0.0 keyword.
Comment 40 Ben Bucksch (:BenB) 2002-05-01 02:37:48 PDT
Also fixed on 1.0 branch.
Comment 41 Christian :Biesinger (don't email me, ping me on IRC) 2002-05-01 05:42:05 PDT
shouldn't this be marked FIXED now?
Comment 42 Oliver Klee 2002-05-01 05:56:20 PDT
*** Bug 141453 has been marked as a duplicate of this bug. ***
Comment 43 Darin Fisher 2002-05-01 09:11:20 PDT
marking FIXED
Comment 44 R.K.Aa. 2002-05-01 12:46:46 PDT
*** Bug 141551 has been marked as a duplicate of this bug. ***
Comment 45 Gilles Durys 2002-05-02 06:32:42 PDT
*** Bug 141727 has been marked as a duplicate of this bug. ***
Comment 46 Alfonso Martinez 2002-05-02 08:40:16 PDT
*** Bug 141755 has been marked as a duplicate of this bug. ***
Comment 47 bsharma 2002-05-02 15:19:49 PDT
Verified on 2002-05-01-trunk and 2002-05-01-branch on WinNT

3 testcases:
1. URL
2. barrowma.com/redirtest.html
3. test provided by Heikki

work fine.
Comment 48 Mike Schiraldi 2002-05-03 12:27:00 PDT
I was playing around with the exploit example page at
http://sec.greymagic.com/adv/gm001-ns/ and i was wondering about what appears to
be a similar problem.

After updating to the very latest (fixed) Moz, the file://whatever hole seems to
be fixed. However, if i type, say, http://slashdot.org/users.pl?op=edituser into
the textbox and click "sniff", the content that their site grabs from me
includes what is supposed to be private data -- my email address and so on. 

Could an approach similar to this bug be used to, say, grab someone's stock
portfolio information by sniffing a URL at Yahoo Finance?
Comment 49 Boris Zbarsky [:bz] (still a bit busy) 2002-05-03 12:55:29 PDT
Mike, chances are the answer is "yes", from what I remember of this code...
could you please file a separate bug on it?
Comment 50 Darin Fisher 2002-05-03 13:07:11 PDT
Mike: yup, you raise a good point... however, short of breaking compatibility
with a large number of websites, there is very little we can do to avoid that
security risk.  IMO, concerned users ought to disable javascript anyways when
potentially visiting a malicious website.
Comment 51 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-05-03 13:32:04 PDT
I can fix the problem Mike mentioned in XMLHttpRequest itself (maybe also for
document.load but I am not sure). I'll do that in bug 133170.
Comment 52 Christopher Blizzard (:blizzard) 2002-05-04 09:00:13 PDT
Darin, what sites?  Who uses this?  And this seems like a serious problem, one
that will show up.  I don't want to ship like that.  Heikki, do we need to make
sure that the bug you mentioned stays on the 1.0 radar?
Comment 53 Darin Fisher 2002-05-06 11:23:49 PDT
blizzard: i was referring primarily to document.load ... seems like it'd be
common place for websites to document.load an URL that results in a redirect to
a site under a different domain.  so, how can we block cross-site redirects on
document.load?  no other browser does, so we'd just end up making the product
incompatible with who-knows-what top100 website.
Comment 54 Marc Rubin 2002-05-06 13:07:09 PDT
To get a version which includes the fix, are the nightly builds stable enough 
for download by end-users? Or should they wait for RC2? Any estimate yet on a 
release date for RC2? 
Comment 55 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-05-06 13:36:54 PDT
document.load(), and XMLHttpRequest.open() already check if they are allowed to
access the URL that is passed in, and fail if the security check fails. The fact
that you can go around this check by using a redirect seems like a bug to me,
clear and simple.

I created two testcases that show this allows a hacker access to Bugzilla using
my account, and access to W3C member-only sections using my account. The
exploits are possible if I have logged in to Bugzilla (cookies remember my login
and are sent automatically) or W3C (basic auth login remembered and sent
automatically) during this session.

http://green.nscp.aoltw.net/heikki/133170/xmlhttpbugzilla.html
http://green.nscp.aoltw.net/heikki/133170/xmlhttpw3cauth.html

XMLHttpRequest is the more serious issue here because it can give you access to
documents regardless of format. document.load(), I believe, will only be able to
give the hacker access to XML documents. I have a fix for XMLHttpRequest
altready, and I am debugging similar fix for document.load() in bug 133170.

I would advice moving this discussion there.
Comment 56 Alex Bishop 2002-05-06 13:54:46 PDT
> Any estimate yet on a release date for RC2?

Any day now.
Comment 57 Christian :Biesinger (don't email me, ping me on IRC) 2002-05-07 03:31:25 PDT
> Any estimate yet on a release date for RC2?

When all dependencies for bug 138000 are fixed, except the nsIFIle one
Comment 58 Mike Pinkerton (not reading bugmail) 2002-05-07 07:17:44 PDT
for what it's worth, PPEmbed from 5/6/02 1.0 branch still has this problem.

should i reopen? removing verified1.0.0 keyword. we need traction on this asap,
it's a beta blocker for embedding clients.
Comment 59 Mike Pinkerton (not reading bugmail) 2002-05-07 08:59:02 PDT
never mind, it appears we jumped the gun. this bug appears fixed in 5/6/02
ppembed. someone is toking up without sharing again. ;)
Comment 60 scottputterman 2002-05-07 09:22:48 PDT
adding back verified1.0.0 based on pinkerton's most recent comments.  It sounds
like new issues are being handled in 133170.
Comment 61 Mike Schiraldi 2002-05-07 14:08:58 PDT
I was away for a few days; after reading the new comments, i'm not sure whether
you still want me to file a new bug as requested in #49, or if someone else is
taking care of this potential problem.
Comment 62 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-05-07 14:19:15 PDT
Mike, that is being covered by bug 133170.
Comment 63 benc 2002-05-08 01:50:53 PDT
Darin: Will this solve Bug 102262?

I've been wanting that to go away for a long time, but haven't had time to focus
on it.
Comment 64 bsharma 2002-05-09 10:45:21 PDT
Verified on 2002-05-08-6.2.3 on WinNT.
3 testcases:

1. URL ("This is blocked by the security manager)
2. barrowma.com/redirtest.html (an empty alert is shown)
3. test provided by Heikki (text is not red)

work fine.


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