Closed Bug 1162411 Opened 9 years ago Closed 9 years ago

Cross origin restriction bypass with Fetch API

Categories

(Core :: DOM: Core & HTML, defect)

40 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- disabled
firefox39 + verified
firefox38.0.5 --- disabled
firefox40 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- disabled
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- fixed

People

(Reporter: sdna.muneaki.nishimura, Assigned: nsm)

Details

(Keywords: sec-high)

Attachments

(3 files)

Attached image Capture.PNG
User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2383.0 Safari/537.36

Steps to reproduce:

1) Open Fiddler web debugger and enable "Decrypt HTTPS Traffic" option
2) Launch https://mallory.csrf.jp/fetch/request.html
3) See the request packet sent from the 2) by Fiddler


Actual results:

Fetch API doesn't restrict request header names to be set.
In this scenario, the request shown in 3) has invalid Host and Cookie headers to the other origin, csrf.jp, and the response data has come from https://csrf.jp/index.html.

fetch("/index.html", {
  method: "GET",
  headers: {
    "Host": 'csrf.jp',
    "Cookie": "Arbitorary Values"
  }
})


Expected results:

Fetch API should restrict header names that can be set (see the attached image).
For example, Host, Cookie and sec-* headers should be ignored.
The implementation of XmlHttpRequest.setRequestHeader would be a good reference.
Component: Untriaged → DOM
Product: Firefox → Core
Flags: sec-bounty?
Per spec https://fetch.spec.whatwg.org/#fetch-method calls https://fetch.spec.whatwg.org/#dom-request which in this case will end up in step 26 calling https://fetch.spec.whatwg.org/#concept-headers-fill which calls <https://fetch.spec.whatwg.org/#concept-headers-append>.  The guard is "request", since that's what https://fetch.spec.whatwg.org/#dom-request set up, so things that are on the "forbidden header name" list should be ignored.  That list is at https://fetch.spec.whatwg.org/#forbidden-header-name and includes "Host".  And "Cookie".

That said, we seem to implement at least parts of that... Need to figure out what's going on here.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Ah, looks like the bug is that Request::Constructor never sets the guard on the Headers object to "request" anywhere.
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(nsm.nikhil)
Attached patch fixSplinter Review
Assignee: nobody → nsm.nikhil
Attachment #8603047 - Flags: review?(bzbarsky)
Comment on attachment 8603047 [details] [diff] [review]
fix

r=me
Attachment #8603047 - Flags: review?(bzbarsky) → review+
[Tracking Requested - why for this release]:
Security bug that allows cross origin restriction bypass in the new fetch api.
Is it possible to get this into 38 or a 38 point release?  Given it's the next ESR should it just wait until 38's first point release?
Flags: needinfo?(lmandel)
The Fetch API is not enabled by default in 38.  We only enabled it in 39 (see bug 1133861).
Given that this is a sec bug, it should have a sec rating and may have required sec approval to land. From comment 8 I take it that this fix isn't required for ESR but should be uplifted to 39, which becomes Beta on Monday.
Flags: needinfo?(lmandel)
Yes, this needs a rating and sec-approval+ before any checkin.

https://wiki.mozilla.org/Security/Bug_Approval_Process
Comment on attachment 8603047 [details] [diff] [review]
fix

This is my first time, so I hope I got things right.
Should I publicly be pushing to inbound?

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The test case is a potential exploit

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes

Which older supported branches are affected by this flaw?
The feature is not in release builds yet. It was shipped in Fx 39 and is in Dev edition.

If not all supported branches, which bug introduced the flaw?
Bug 1039846

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply directly. Otherwise, it is easy to create a backport.

How likely is this patch to cause regressions; how much testing does it need?
Low. Tests already exist and this patch adds a few more.
Flags: needinfo?(nsm.nikhil)
Attachment #8603047 - Flags: sec-approval?
We need a security rating on this. Could you take a look at https://wiki.mozilla.org/Security_Severity_Ratings

FYI, you won't be able to check in for a few weeks. We're shipping on Tuesday next week and starting a new six week dev cycle and, normally, we don't allow security bugs to be checked in during the first two weeks unless there is a very good reason.
It's already been checked in (and then backed out), so I'm not sure how much of a point there is to delaying it landing at this point, honestly.
Flags: needinfo?(abillings)
Well, it still needs a rating. Otherwise, we can't write advisories (among other things).
Flags: needinfo?(abillings)
I'm not disagreeing about that part.  This is probably a sec-high: it's a cross-origin policy violation allowing reading data cross-site (though not from arbitrary sites, only ones on the same IP), but doesn't allow privilege escalation to arbitrary execution or anything like that.  I don't think the same-IP restriction is enough to downgrade to a sec-moderate, personally.
Comment on attachment 8603047 [details] [diff] [review]
fix

Thanks for the rating. I'll mark this as sec-approval+. Might as well check it in given the previous exposure.
Attachment #8603047 - Flags: sec-approval? → sec-approval+
Muneaki, Chrome seems to be similarly affected.  Have you reported this to the Chromium project?  Thanks!
Ehsan, I've checked it on the latest Chrome (44.0.2383.0 m) but as far as I looked illegal headers  were implicitly removed from HTTP requests. If you found some ways to bypass the restriction on Chrome please report it from you.

And note that I also reported another cross origin violation issue of Fetch API (with ServiceWorker) as Bug 1162018. Please check the bug if you've never seen.
https://hg.mozilla.org/mozilla-central/rev/4d40d05a58df
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: sec-bounty? → sec-bounty+
Nikhil can you request uplift to beta since it looks like 39 is affected? Thanks.
Flags: needinfo?(nsm.nikhil)
Comment on attachment 8603047 [details] [diff] [review]
fix

Approval Request Comment
[Feature/regressing bug #]: Bug 1039846
[User impact if declined]: Websites can spoof headers like User-Agent and Origin, as well as pass cookies.
[Describe test coverage new/current, TreeHerder]: Patch has tests.
[Risks and why]: None. This API only shipped in Firefox 39.
[String/UUID change made/needed]: None.
Flags: needinfo?(nsm.nikhil)
Attachment #8603047 - Flags: approval-mozilla-beta?
Comment on attachment 8603047 [details] [diff] [review]
fix

Approved for uplift to beta (39) since this is a sec-high bug.
Attachment #8603047 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
The test changes need rebasing around bug 1149987.
Flags: needinfo?(nsm.nikhil)
Target Milestone: --- → mozilla40
Attached patch fix for betaSplinter Review
This is ready to go.
Flags: needinfo?(nsm.nikhil) → needinfo?(ryanvm)
Reproduced the initial issue on Aurora 39.0a2 (2015-05-05), using the steps and sample page from Comment 0.

Per my conversation with :nsm, this is now verified fixed on 39.0b1-build1 (20150520170903) - the request is no longer setting a cookie.
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.