195201 - An image that sends a "Location: javascript:document.location.href='....';" http header will redirect the whole browser window

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[Sam Cannell]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; PN)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030226

The page linked to from http://plaz.debian.co.nz/mozilla/ has <img 
src="image.jpg"> .. image.jpg is a php script that sends the http 
header "Location: javascript:document.location.href='http://www.mozilla.org/'".

This causes the browser window to be redirected to the mozilla site.

Reproducible: Always

Steps to Reproduce:
Described above
Actual Results:  
Browser window was redirected to www.mozilla.org

Expected Results:  
Not execute javascript delivered in http headers.

Theme was default.
Computer is a duron 850 with 640mb ram, running Windows 2000.
I'll download the nightly Linux build when I get to work tomorrow, and try it 
with that.

I'm guessing this could cause problems with people posting images to forums 
that redirect browsers to porn sites or similar.

Comment 1

[timeless]

while this could be annoying, i don't think there are any security ramifications.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Actually, I can see not supporting JS in Location: headers.  But yes, this is
not a security bug.

Comment 3

[Christopher Hoess (gone)]

->Networking: HTTP

Comment 4

[Sam Cannell]

I'm hesitant to agree on that.  

I haven't tested it fully, but given that the document.location code is
executing, is there the possibility that other javascript code could be
executed, modifying the document in the web browser without the user's
knowledge?  For example, changing the action property on a form in the document
and redirecting the submission?

Comment 5

[Russell Odom]

OK, I see this (1.3b/Win2K). Confirming. I agree with comment 4; I'm sure that
there are plenty of nasty things someone could do with this. Furthermore, I
can't think of any times where you would legitimately put some Javascript in a
Location header that couldn't be done in a better way.

I've just tried in IE5.5, and it isn't susceptible to this; the test URL just
shows a broken image.

Comment 6

[Sam Cannell]

Likewise with IE6 and Konqueror (In fact Konq spews warnings apout javascript 
redirects to stdout when it tries to open the image)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, but JS code executing this way is no different from JS code executing any
other way, imo (think linking to an image that's SVG and just has script
embedded in it).

Comment 8

[Darin Fisher]

given the timing of this bug report, i wonder if this bug could be a duplicate
of or possibly related to bug 193887.

Comment 9

[Sam Cannell]

In response to comment 7:

There's no real issue if you're hosting images yourself, but if you have a site
that links to images on a host that you're not in control of, then somebody with
access to the images you're linking could manipulate the content of your site at
the browser.

Comment 10

[Jesse Ruderman]

Many forums allow posts to include <img>s with off-site srces.  I disagree with
bz's conclusion that this isn't a security problem.

Comment 11

[Darin Fisher]

we have discussed possibly limiting HTTP redirects to a whitelist. i don't know
if now is the right time to do that or not...

Comment 12

[Bradley Baetz (:bbaetz)]

I thought that we'd blocked js redirects a long time back.

Even ignoring this, why is the document getting redirected, not the image? Is
this because ofhte nsImageDocument hacks? Even so, 'document' shouldn't be the
parent document.

Comment 13

[Boris Zbarsky [:bzbarsky]]

The script is executed in the context of the parent document, in this case.

Jesse, your concern about off-site images does not address the fact that SVG has
precisely that problem, and that as people move from the deprecated (in XHTML)
<img> to <object> this will only get worse, since HTML could be used instead of
SVG as the script vehicle.

Comment 14

[Bradley Baetz (:bbaetz)]

svg doesn't yet work in <img>, and theres a bug somewhere detailing the security
concerns this would have.

Comment 15

[Mitchell Stoltz (not reading bugmail)]

If an HTML page includes a <SCRIPT SRC="script file from some other host"> tag,
then the script runs with the permissions of the page and can modify or read the
DOM of the page. It's always been that way: including a script from another host
gives anyone with write access on that other host control over your page. The
problem here is that there may be an assumption among page authors that the same
is not true for images. Lots of people include images from other hosts without
realizing this security implication. For that reason, I think this is a real
security issue that should be fixed ASAP.

Marking this bug Security-Sensitive.

Comment 16

[chris hofmann]

should try to fix for 1.4 final if possible.

Comment 17

[Darin Fisher]

mitch, doug: can one of you pick this up for 1.4 final?  thanks!

Comment 18

[Doug Turner (:dougt)]

in 1.4b, i get an uncaught exception: permission deniced to get property
Function.href.

I am updating my tree now.  

Comment 19

[Doug Turner (:dougt)]

yup.  i can repro with a build from this morning.  I hope to look at this over
the 3 day weekend.  If someone has time, it would be a good exercise to reduce
the range of dates where this bug started to occur.

Comment 20

[Doug Turner (:dougt)]

I checked version 1.3, 1.4a, and 1.4b.  I could only reproduce this problem in
1.3.  Mitch we attempt to fix any similar issues between 1.3 and now?  I am not
sure if this is a regression or if this is a hole that we never encountered.

This test case is slightly misleading.  The contents of the image doesn't really
matter in the general case.  You could set up a server that on every image
request, the server returns a redirect as part of the response.  

There is no way to prevent an image request from returning a Location field in
the http response.  So, the root of the problem, i believe, is that we are
honoring URI's that have no business being in a Location field.  This follows
darin comment #11.

Also Mitch, we currently call CheckLoadURI() when processing the redirect.  It
would be nice to continue using this check instead of special casing the code in
http.  How do you suggest that we instruct this method to exclude javascript and
possibly other URL schemes during a redirect check?  

Any suggestions on alternative fixes?  

Comment 21

[Mitchell Stoltz (not reading bugmail)]

Your approach sounds good. We can add another "flag" to CheckLoadURI that
excludes javascript: and data: URLs.

Comment 22

[Mitchell Stoltz (not reading bugmail)]

Attachment: CAPS patch

This patch adds a new option to CheckLoadURI that prevents loading javascript:
and data: URLs. To use it, pass nsIScriptSecurityManager::DISALLOW_JAVASCRIPT
as the third argument to CheckLoadURI.

Comment 23

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch - disregard the last one

Comment 24

[Doug Turner (:dougt)]

Attachment: patch v.2

Although, I am not convinced that this is the best solution to the problem, it
does fix the potential security hole and it is quite simple.

Comment 25

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 124384 [details] [diff] [review]
patch v.2

I think this will do for now. r=mstoltz.

Comment 26

[Doug Turner (:dougt)]

Comment on attachment 124384 [details] [diff] [review]
patch v.2

brendan, can you review this change?

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 124384 [details] [diff] [review]
patch v.2

Call it DISALLOW_SCRIPT to generalize to other languages than javascript, and
to take in the notion of data: URLs.  Maybe DISALLOW_SCRIPT_OR_DATA?  Your
call.

/be

Comment 28

[Doug Turner (:dougt)]

mitch, you pick the name.

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 124384 [details] [diff] [review]
patch v.2

Feel free to change the define name before checkin, but let's get this in. 
a=rjesup

Comment 30

[Doug Turner (:dougt)]

Checking in caps/idl/nsIScriptSecurityManager.idl;
/cvsroot/mozilla/caps/idl/nsIScriptSecurityManager.idl,v  <-- 
nsIScriptSecurityManager.idl
new revision: 1.54.36.1; previous revision: 1.54
done
Checking in caps/src/nsScriptSecurityManager.cpp;
/cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v  <-- 
nsScriptSecurityManager.cpp
new revision: 1.201.2.1; previous revision: 1.201
done
Checking in netwerk/protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <-- 
nsHttpChannel.cpp
new revision: 1.165.2.1; previous revision: 1.165
done

Waiting on the trunk to open to check in there.

Comment 31

[Paul Wyskoczka]

Adding branch fixed1.4 keyword 

Comment 32

[Doug Turner (:dougt)]

fixed on trunk:
Checking in caps/idl/nsIScriptSecurityManager.idl;
/cvsroot/mozilla/caps/idl/nsIScriptSecurityManager.idl,v  <-- 
nsIScriptSecurityManager.idl
new revision: 1.55; previous revision: 1.54
done
Checking in caps/src/nsScriptSecurityManager.cpp;
/cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v  <-- 
nsScriptSecurityManager.cpp
new revision: 1.203; previous revision: 1.202
done
Checking in netwerk/protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <-- 
nsHttpChannel.cpp
new revision: 1.166; previous revision: 1.165
done
bash-2.05b$ 

Comment 33

[benc]

Is there an updated test case for this bug? The mentioned site is now gone.

Comment 34

[Sam Cannell]

I've put it back up at http://plaz.net.nz/mozilla/

Sorry about that - the box I had plaz.net.nz hosted on fell over somewhat messily.

Cheers,

Sam

Comment 35

[benc]

So if the fix worked the page simply displays w/ a broken image right?

Comment 36

[Sam Cannell]

Yeah - that's right.

On Mozilla 1.3 and below, it redirected the entire browser window to another 
site.

Comment 37

[benc]

V, verified 1.4, using Netscape 7.1

Comment 38

[Daniel Veditz [:dveditz]]

Bugs published on the Known-vulnerabilities page long ago, removing confidential
flag.