Location: that won't parse to valid URI causes 3xx body to be rendered

RESOLVED FIXED

Status

()

Core
Networking: HTTP
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: Garrett Held, Assigned: jduell)

Tracking

(Blocks: 1 bug, {sec-vector})

11 Branch
x86
Mac OS X
sec-vector
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr10-)

Details

(Whiteboard: [sg:vector-moderate] invalid, embargoed until reporter fixes website error or June-2012)

Attachments

(4 attachments, 6 obsolete attachments)

1.94 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
2.18 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
4.37 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
5.53 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120312181643

Steps to reproduce:

This requires a special character sequence, more information below.

Used a web app that did a 302 redirect that used HttpServletResponse.sendRedirect(). For this example we'll use http://x/redirect?url=

Under normal circumstances a value of:
http://x/redirect?url=http://www.mozilla.org
would redirect the user properly.

A value of:
http://x/redirect?url=http://www.mozilla.org"%20onmouseover%3d"alert(document.cookie)">a:%b
would redirect the user and produce an error (properly)

However, a value of:
http://x/redirect?url=http://www.mozilla.org"%20onmouseover%3d"alert(document.cookie)">a:%20b
causes the HTML to be rendered, the link shown, and the document.cookie for the server is shown. The colon followed by a space, followed by a character, seemed to be necessary.

A value of:
http://x/redirect?url=data:http://www.mozilla.org"%20onmouseover%3d"alert(document.cookie)">a
produced the same bad result


Actual results:

The HTML and JavaScript was rendered and executed. An event-driven reflected cross-site scripting attack was successful.


Expected results:

The body should not have been rendered and the browser should have attempted a redirect.

Safari and Chrome both attempt redirects properly.

Comment 1

6 years ago
Thoughts, Patrick?
Assignee: nobody → mcmanus

Comment 2

6 years ago
Do you have an actual HTTP testcase for this?

From your basic description, it appears that you would be sending an HTTP response of this form:

HTTP/1.1 302 Moved
Location: http://www.mozilla.org" onmouseover="alert(document.cookie)">a: b

It's not clear from your description what the response *body* actually is. Are you saying that we are incorrectly treating everything after the first space as the response body?

This is obviously an invalid URL for a location header. I suspect that we should simply be throwing away the invalid Location header and just display the response body, but I'd have to test our behavior more thoroughly.
What kind and version of server?

Could you attach a copy of the 302 that's returned (including headers)? This behavior is not generally true of redirects. There may be a problem with us being able to recognize this particular 302 as a redirect, but it also sounds like the particular server in question has some problems sanitizing the output of the redirect body.
(Reporter)

Comment 4

6 years ago
Sorry that was not clear. I considered this a gray area (because the web app should be encoding output) but since there was a difference in browser behavior and it used a commonly utilized method, I wanted to report it. 

I didn't mean to imply response splitting, the header appears fine. However, the Location header is being ignored when supplied with values from the examples above.

This causes the body to be rendered:
"""
The page has moved <a href="http://www.mozilla.org/" onmouseover="alert(document.cookie)">a: b">here</a>
"""

I realize the web application level or framework is allowing this attack by not encoding the double-quote in the body but I thought it was odd this only worked in Firefox.
(In reply to Garrett Held from comment #4)
> Sorry that was not clear. I considered this a gray area (because the web app
> should be encoding output) but since there was a difference in browser
> behavior and it used a commonly utilized method, I wanted to report it. 
> 

I'm glad you did - it sounds like a smuggling approach of some sort. It would just be simplest if we could see what it looks like on the wire. 

I don't have a java environment set up to try it with - I can certainly do so next week (and then try and remember how to write in it) but it would speed things along if either:

a] you can capture the wire response with "wireshark" (beware that doesn't work on localhost on windows)

or 

b] you have a url to point to that illustrates it.


Thanks!
(Reporter)

Comment 6

6 years ago
I'll see if I can work up a sanitized repro.
(Reporter)

Comment 7

6 years ago
I didn't go to the trouble of the vulnerable framework (and thus no xss in the body), but here is a poc for the 302: http://pure-window-2231.herokuapp.com/
This is the HTTP transaction generated by comment 7

GET / HTTP/1.1
Host: pure-window-2231.herokuapp.com
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120330 Firefox/14.0a1
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip, deflate
DNT: 1 
Connection: keep-alive

===

HTTP/1.1 302 FOUND
Content-Type: text/html; charset=utf-8
Date: Fri, 30 Mar 2012 21:23:28 GMT
Location: http://www.mozilla.org">a: b
Server: Werkzeug/0.8.3 Python/2.7.2
Content-Length: 274
Connection: keep-alive


<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN">
<title>Redirecting...</title>
<h1>Redirecting...</h1>
<p>You should be redirected automatically to target URL: <a href="http://www.mozilla.org&quot;&gt;a: b">http://www.mozilla.org"&gt;a: b</a>.  If not click the link.

Comment 9

6 years ago
We are parsing the location header correctly, and it's ending up here:

http://hg.mozilla.org/mozilla-central/annotate/6fe5b0271cd1/netwerk/protocol/http/nsHttpChannel.cpp#l3523

At that point we fail to create a URI from the location header and proceed to ignore the malformed location header. We then display the HTTP body. I believe that this is correct behavior and merely shows a bug in your web service/framework. I'm going to close this bug as INVALID. I'm going to keep it hidden for now, but I'd like to make it public as soon as possible, so please let me know if that is not acceptable or you have not yet fixed your service to be secure.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
Whiteboard: invalid, embargoed until reporter fixes website error or June-2012
I think we might want to not display the body here.. jason has been dealing with this issue in the form of a blank location header so I would want to hear from him.
(Assignee)

Comment 11

6 years ago
I'd lean toward not displaying the body.  There do seem to be sites that allow URI params, etc to wind up in the Location header, so this seems like a plausible CRLF vector (ex: bug 716801 comment 0).  

And I'm not too worried about breaking much of the web here (Yes, this is said by the guy who didn't think anyone would be sending empty Location: headers in non-redirect replies :)  But if important sites are actually using invalid URIs in Location in order to guarantee 3xx bodies get rendered, I'll be surprised (and we can always backout).
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
Summary: 302 Redirect Renders Response Body and Allows Cross-Site Scripting With Character Sequence → Location: that won't parse to valid URI causes 3xx body to be rendered
(Assignee)

Comment 12

6 years ago
Created attachment 613750 [details] [diff] [review]
v1. pop up corrupt content page if Location won't parse to URI.
Assignee: mcmanus → jduell.mcbugs
Attachment #613750 - Flags: review?(mcmanus)
Status: REOPENED → NEW
Whiteboard: invalid, embargoed until reporter fixes website error or June-2012 → [sg:vector-moderate] invalid, embargoed until reporter fixes website error or June-2012
I agree with Jason and Patrick. Also, it isn't a matter of just *displaying* the response body; it also affects XHR and other non-display applications of HTTP channels. Finally, as noted in another bug, we don't cache the response body of 3xx responses, so you would get inconsistent results the first time you load a redirect (from the network) and when you subsequently load it (from the cache).
Comment on attachment 613750 [details] [diff] [review]
v1. pop up corrupt content page if Location won't parse to URI.

yep, that will prevent any consumer (page, xhr, etc..) from getting the message-body data.
Attachment #613750 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 15

6 years ago
Comment on attachment 613750 [details] [diff] [review]
v1. pop up corrupt content page if Location won't parse to URI.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7814c8fdc48b

Worth taking upstream?  Fix is simple enough.

[Approval Request Comment]
Regression caused by (bug #): n/a: security bug
User impact if declined:  possible CRLF attack for HTTP redirects. Note the Chrome is vulnerable and no know exploits at moment, so we're not out on a limb here.
Testing completed (on m-c, etc.):  just landing on m-c.
Risk to taking this patch (and alternatives if risky):  very low IMO
String changes made by this patch: none
Attachment #613750 - Flags: approval-mozilla-beta?
Attachment #613750 - Flags: approval-mozilla-aurora?

Comment 16

6 years ago
https://hg.mozilla.org/mozilla-central/rev/7814c8fdc48b
Status: NEW → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
1. Is there a test for this?

2. I am working on nsHttpChannel now and noticed that there are some more things that should probably be updated as part of this patch. For example, this comment:

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp?rev=bfa27d58769d#2998

"If this is a cached redirect, we must process the redirect asynchronously since AsyncOpen may not have returned yet.  Make sure there is a Location header, otherwise we'll have to treat this like a normal 200 response."
To expand on comment 17: what happens when we've already cached a document that contains a corrupted-content error? It seems like we need to do the corrupted content checks as part of CheckCache() too; if the cached entry is corrupt, we must doom it and continue on with the network fetch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
QA Contact: untriaged → networking.http
Target Milestone: Firefox 14 → ---
Comment on attachment 613750 [details] [diff] [review]
v1. pop up corrupt content page if Location won't parse to URI.

[Triage Comment]
This bug has now been reopened - please renominate once this is resolved fixed again.
Attachment #613750 - Flags: approval-mozilla-beta?
Attachment #613750 - Flags: approval-mozilla-beta-
Attachment #613750 - Flags: approval-mozilla-aurora?
Attachment #613750 - Flags: approval-mozilla-aurora-
tracking-firefox-esr10: --- → -
(Assignee)

Updated

6 years ago
Blocks: 747215
(Assignee)

Comment 20

6 years ago
Created attachment 616839 [details] [diff] [review]
test

First the good news:  here's a test for my code.  I've also checked what happens if a CORRUPT redirect is cached (they are no longer cached with my patch, but they could have been cached by previous version): the entry is expired (date: 1969), so it winds up having no effect, and we still show the CORRUPT error page.

Now the bad news: while writing the test, I discovered that the control path that I added "return NS_ERROR_CORRUPTED_CONTENT" in my fix has a bug in it--it winds up not calling OnStartRequest (only OnStopRequest).   There is other code in that pathway (ContinueProcessResponse called from ProcessResponse) which will also not call OnStartRequest (the existing return CORRUPT pathway that checks for NS_ERROR_DOM_BAD_URI).  So a simple backout of this patch doesn't get rid of the issue.  I'm going to fix the missing OnStart in bug 747215.  Meanwhile we need to figure out what to do with this bug (which has landed on central).

Here's our options:

1) Keep the patch in. While it's a logical error to not call OnStart here, it doesn't seem to be causing any errors (we're getting the error page just fine).  But if some addon stuck a listener into the chain, it could get confused by the lack of OnStartRequest.

2) Back out this patch and wait for bug 747215 to re-land it.  This means we won't get the secfix in for FF 14.  Not a big deal IMO--Chrome has same vulnerability.  We will still have a codepath that doesn't call OnStartRequest, but we'll have fewer of them.

3) Try to get 747215 and this into FF 14.  I don't see +a as likely to happen, as the patch I'm working on isn't small enough.

I could go either way.  I guess I'd lean toward #1 since this code seems to be working and we can backout from aurora/beta easily later.
Attachment #616839 - Flags: review?(mcmanus)
Good catch jason - I favor option 2 from comment 20. The fix isn't ready for the FF14 train. Let's get it (and 747215) into FF15 in the next 10 days or so - that way it will get an aisle seat on the FF15-express.
Comment on attachment 616839 [details] [diff] [review]
test

The test is cool.
Attachment #616839 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 23

6 years ago
Created attachment 617628 [details] [diff] [review]
Backout incomplete bug fix

I've been told on IRC I should request approval for this backout, since it's been in the tree for 10 days.  See comment 20 onward for explanation of backout.

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any): we don't get this secfix in FF/fennec 14.
Attachment #617628 - Flags: review+
Attachment #617628 - Flags: approval-mozilla-central?
Comment on attachment 617628 [details] [diff] [review]
Backout incomplete bug fix

[Triage Comment]
Merge of 14 to Aurora has completed.  Please renominate for mozilla-aurora.
Attachment #617628 - Flags: approval-mozilla-central?
(Assignee)

Comment 25

6 years ago
Created attachment 618124 [details] [diff] [review]
part 2: fix missing OnStart

Patrick, let me know if you think we should land this on aurora too (to get full fix), or backout the 1st patch (and get full un-fix :)   I'd lean toward landing on aurora, but could go either way.  

Re: comment 17: I'm not dealing with cached redirects here because unless we fix bug 748510 we have no body to render or not, so the code always doesn't render.

Note: I assume it's ok to remove the Cancel calls from AsyncProcessRedirection because when we return failure, both the ProcessResponse and OnRedirectVerifyCallback codepaths do  "mStatus = failure code; DoNotifyListeners()", which seems to be equivalent to "Cancel(rv); DoNotifyListeners" (we use both idioms a lot, seemingly interchangeably: obviously Cancel also cancels the transaction/pumps, but both seem to affect the OnData/Stop events the same way, i.e. OnData is skipped).
Attachment #618124 - Flags: review?(mcmanus)
(Assignee)

Comment 26

6 years ago
Created attachment 618128 [details] [diff] [review]
part 2, v2
Attachment #618128 - Flags: review?(mcmanus)
(Assignee)

Comment 27

6 years ago
Comment on attachment 618124 [details] [diff] [review]
part 2: fix missing OnStart

created v2 because I was missing some changes in my editor that I hadn't saved when I uploaded v1.
Attachment #618124 - Attachment is obsolete: true
Attachment #618124 - Flags: review?(mcmanus)
(Assignee)

Comment 28

6 years ago
Created attachment 618396 [details] [diff] [review]
part 2, v3

Changed to just switch on error codes, not a separate bool arg.  Simpler.
Attachment #618128 - Attachment is obsolete: true
Attachment #618128 - Flags: review?(mcmanus)
Attachment #618396 - Flags: review?(mcmanus)
Attachment #618396 - Flags: review?(mcmanus) → review-
(Assignee)

Comment 29

6 years ago
Comment on attachment 618396 [details] [diff] [review]
part 2, v3

Patrick:  any comments to go along with the r-?
(In reply to Jason Duell (:jduell) from comment #29)
> Comment on attachment 618396 [details] [diff] [review]
> part 2, v3
> 
> Patrick:  any comments to go along with the r-?

umm.. that was supposed to be r+

that was my damned trackpad - not the first time I've made a "scroll gesture" when looking for the save button.

sorry.
Attachment #618396 - Flags: review- → review+
(In reply to Jason Duell (:jduell) from comment #31)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/11b16556248e

Sorry, had to backout the push for xpcshell failures on all platforms:
(Note the CPG landing before this is responsible for the OS X opt specific failure, so ignore that one)

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=12d1d626759c
eg https://tbpl.mozilla.org/php/getParsedLog.php?id=11419058&tree=Mozilla-Inbound

{
TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_nojsredir.js | running test ...
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_nojsredir.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>
### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/tmpUl4FRR/runxpcshelltests_leaks.log

TEST-INFO | (xpcshell/head.js) | test 1 pending

TEST-INFO | (xpcshell/head.js) | test 2 pending

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | running event loop

TEST-PASS | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/head_channels.js | [null : 147] 16 == 16

TEST-PASS | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_nojsredir.js | [completeIter : 30] true == true

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/head_channels.js | onStopRequest without onStartRequest event! - See following stack:
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 462
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/head_channels.js :: <TOP_LEVEL> :: line 132

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/head_channels.js | Error in onStopRequest: 2147500036 - See following stack:
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 462
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/head_channels.js :: <TOP_LEVEL> :: line 149
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/074c8fb332a8
(Assignee)

Comment 33

6 years ago
Created attachment 621802 [details] [diff] [review]
Part 3: also don't render UNKNOWN_PROTOCOL

OK, I think this is finally it for this patch.  The last patch caused test_redirect-caching_failure.js to fail.  Turns out that test redirects to a bad protocol ("httpx://..."), which only fails later in the codepatch (not when we create a URI, but later, when we try to create a channel), with NS_ERROR_UNKNOWN_PROTOCOL.  So this patch checks for that (and MALFORMED_URI, too, for good measure) too.

This all hits the same code as the patch for 747215 you already +r'd, Patrick: to reduce the amount of patch cross-bug dependencies, I'm going to blow away that patch--this patch includes the same logical fixes.
Attachment #621802 - Flags: review?(mcmanus)
(Assignee)

Comment 34

6 years ago
Created attachment 621807 [details] [diff] [review]
Part 3, v2: also don't render UNKNOWN_PROTOCOL

fixed one little thing:  the new version of the patch wasn't returning NS_ERROR_CORRUPTED_CONTENT any more for the "NS_ERROR_DOM_BAD_URI and redirect to non-http" case.  Not sure if we have a good boilerplate error page for that, or if we'd want to use it, so I'm converting rv to NS_ERROR_CORRUPTED_CONTENT in that case.
Attachment #621807 - Flags: review?(mcmanus)
(Assignee)

Updated

6 years ago
Attachment #621802 - Attachment is obsolete: true
Attachment #621802 - Flags: review?(mcmanus)
(Assignee)

Comment 35

6 years ago
Created attachment 621820 [details] [diff] [review]
Part 3, v2: also don't render UNKNOWN_PROTOCOL

Ah, one of those days.  Forgot to qref.
Attachment #621807 - Attachment is obsolete: true
Attachment #621807 - Flags: review?(mcmanus)
Attachment #621820 - Flags: review?(mcmanus)
(Assignee)

Comment 36

6 years ago
Created attachment 621880 [details] [diff] [review]
Part 3, v4: also don't render UNKNOWN_PROTOCOL

try revealed that I also needed to change test_redirect_failure.js to expect a failure now that we don't render "httpx://" redirect bodies.
Attachment #621820 - Attachment is obsolete: true
Attachment #621820 - Flags: review?(mcmanus)
Attachment #621880 - Flags: review?(mcmanus)
Comment on attachment 621880 [details] [diff] [review]
Part 3, v4: also don't render UNKNOWN_PROTOCOL

apologies for the delay
Attachment #621880 - Flags: review?(mcmanus) → review+
(Assignee)

Updated

6 years ago
Attachment #617628 - Attachment is obsolete: true
(Assignee)

Comment 38

6 years ago
Comment on attachment 618396 [details] [diff] [review]
part 2, v3

Testing completed (on m-c, etc.):  has test
String or UUID changes made by this patch: none

So part 1 of this patch landed long enough ago that it's on aurora now.  I'd like to land parts 2/3 and the test on aurora as well, so we've got the whole fix together.  The options for dealing with this mess (sorry for creating it) are still laid out pretty well in comment 20.   

The argument for taking this is that the risk is low (only changes code for an uncommon codepath--malformed redirects) and fixes an existing codepath that will remain broken if we backout.   But this code gets hit infrequently enough that I'm also fine with backing out part 1 from aurora and landing all this on m-c.  Actually I'd be fine with leaving in part 1 only on aurora too... (the 3 options from comment 20 are all ok with me, in other words)

I haven't landed this on m-c for now, as doing so would leave the bugfix splayed across
2 releases, which seems like a Big No-No.  If we don't take this for aurora I'll open a new bug for the 2nd part (or if we backout the 1st patch from aurora, land them all together on m-c).
Attachment #618396 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
Attachment #616839 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
Attachment #621880 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 39

6 years ago
Do I need to CC drivers, or can y'all see security bugs w/o it?
(In reply to Jason Duell (:jduell) from comment #38)
> The argument for taking this is that the risk is low (only changes code for
> an uncommon codepath--malformed redirects) and fixes an existing codepath
> that will remain broken if we backout.   But this code gets hit infrequently
> enough that I'm also fine with backing out part 1 from aurora and landing
> all this on m-c.  Actually I'd be fine with leaving in part 1 only on aurora
> too... (the 3 options from comment 20 are all ok with me, in other words)

CC'ing Dan to get his opinion on whether we need this sg:vector-moderate fix on FF14. My preference from a risk mitigation standpoint would of course be to let it ride the trains on FF15.
Keywords: sec-vector
Keywords: sec-other
Comment on attachment 616839 [details] [diff] [review]
test

[Triage Comment]
Aurora is now FF15 - no need to approve for that branch.
Attachment #616839 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
Attachment #618396 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
Attachment #621880 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(Assignee)

Comment 42

6 years ago
Parts 2 and 3 landed as bug 761932
Blocks: 761932
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.