The default bug view has changed. See this FAQ.
Bug 879787 (CVE-2013-1714)

Cross Domain Policy override using webworkers

VERIFIED FIXED in Firefox 23, Firefox OS v1.1hd

Status

()

Core
DOM: Workers
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: federico lanusse, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({sec-high})

18 Branch
mozilla25
x86_64
Linux
sec-high
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox21 wontfix, firefox22 wontfix, firefox23+ verified, firefox24+ verified, firefox25 verified, firefox-esr1723+ fixed, b2g18? fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [adv-main23+])

Attachments

(8 attachments, 3 obsolete attachments)

98.45 KB, application/java-archive
Details
18.55 KB, patch
smaug
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
12.83 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
12.83 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
12.83 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
13.97 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
14.32 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
1.21 KB, patch
sicking
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Created attachment 758588 [details]
sorry, i needed to zip all together

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:18.0) Gecko/20100101 Firefox/18.0 (Beta/Release)
Build ID: 20130206152201

Steps to reproduce:

I create a web worker in hostA, the webworker has instructions to send a XMLHttpRequest to hostB, also this request include custom headers.

I have try the same XMLHttpRequest using regular javascript and it wont work, the security check are in place that way, the problem is related to the webworkers


Actual results:

The request went thought, attached firefox images and burp proxy


Expected results:

This is not compliant with the same domain policy, also the ability to include custom headers could be a mayor problem as is a vector to attack CSRF protection based on headers.
Does this happen in the latest version of Firefox (21 I believe)?
Flags: needinfo?(mwobensmith)
Attachment #758588 - Attachment mime type: application/octet-stream → application/java-archive
Component: Untriaged → DOM: Workers
Product: Firefox → Core
(Reporter)

Comment 2

4 years ago
not sure, I have firefox 18 in my current fedora 16
Matt is going to try it out on a more recent version.
smaug and/or I fixed something in the way that workers connected to their document's loadgroup a while back but I can't remember where the change was. Maybe smaug does?

Of course that change may not have actually fixed this, but I hope it did...
I can reproduce the issue on latest nightly m-c.

Using the worker, we can send an x-domain XHR via GET or POST that contains custom headers.

Without the worker, we generate the expected OPTIONS request instead - sans custom header - for the normal CORS preflight.
Flags: needinfo?(mwobensmith)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Oh, and of course the obvious - yes, this does happen in the current FF21.
status-b2g18: --- → affected
status-firefox21: --- → wontfix
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox-esr17: --- → affected
Keywords: sec-high
Flags: sec-bounty?
Created attachment 760577 [details] [diff] [review]
Patch, v1

This happens basically because XHR is currently using "IsCallerChrome()" rather than "IsSystemXHR()" to make some security decisions.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #760577 - Flags: review?(jonas)
https://tbpl.mozilla.org/?tree=Try&rev=0f8dd485410d
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bturner@mozilla.com-0f8dd485410d
Comment on attachment 760577 [details] [diff] [review]
Patch, v1

This apparently breaks the following tests:

  tests/content/base/test/test_bug380418.html
  tests/content/base/test/test_xhr_forbidden_headers.html

Not sure why yet. Jonas is pretty convinced that we have to fix these tests so that we never use IsCallerChrome in XHR.
Attachment #760577 - Attachment is obsolete: true
Attachment #760577 - Flags: review?(jonas)
And tests/content/base/test/test_bug383430.html
Created attachment 761048 [details] [diff] [review]
Patch, v1 [NOT FOR CHECKIN]

All the tests should be fixed now. They were basically doing things that unprivileged tests can't do...
Attachment #761048 - Flags: review?(bugs)
Comment on attachment 761048 [details] [diff] [review]
Patch, v1 [NOT FOR CHECKIN]

I think we should do this, although this might break some addons, at least in theory.
Attachment #761048 - Flags: review?(bugs) → review+
Comment on attachment 761048 [details] [diff] [review]
Patch, v1 [NOT FOR CHECKIN]

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Attached in bug

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? All that we currently care about

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No, but the code should be nearly identical.

How likely is this patch to cause regressions; how much testing does it need? May break some addons (we're not sure) so probably needs lots of bake time.
Attachment #761048 - Flags: sec-approval?
Comment on attachment 761048 [details] [diff] [review]
Patch, v1 [NOT FOR CHECKIN]

sec-approval+ for checkin on trunk on July 9 or after (two weeks into the next cycle).
Attachment #761048 - Flags: sec-approval? → sec-approval+
status-firefox22: affected → wontfix
tracking-firefox23: --- → ?
tracking-firefox24: --- → +
tracking-firefox-esr17: --- → ?
Whiteboard: [checkin after 7/9]
Firefox 22 is nearly out the door and this change is too risky to stuff into the last beta, but we'll want to uplift to Fx23 Beta to get bake time on that. We should aim for the 2nd beta so that if it breaks anything it'll stand out from any breakage from the much larger transition when Beta goes from 22 to 23. But due to worries about add-on breakage we shouldn't wait until the 3rd beta. First thing July is probably good to aim for.

(In reply to ben turner [:bent] from comment #13)
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Attached in bug

The point of that question was to judge the risk of premature discovery due to checking in, assuming the bug stays hidden until we release.

The XHR code change point at workers, if we have an innocuous enough comment (Dont mention workers!) it almost looks like just a synonym fix up. Ditto the first test changes which are just fixing up old tests. I'm nervous about checking in the workers tests though:

+++ b/dom/workers/test/Makefile.in
+++ b/dom/workers/test/test_xhr_headers.html
+++ b/dom/workers/test/test_xhr_headers_server.sjs
+++ b/dom/workers/test/test_xhr_headers_worker.js

Could we split those out and check them in after Fx23 ships in early August? If you agree to this plan probably best to clone a "check in the test for bug 879787" security bug and then nominate it for tracking 24 so we don't forget to do it later.

> Do you have backports for the affected branches? [...]
> No, but the code should be nearly identical.

We'll want to check in branch patches at the same time so you may want to get those ready or line up a branch-checkin-buddy.
tracking-b2g18: --- → ?
tracking-firefox23: ? → +
tracking-firefox-esr17: ? → 23+
Whiteboard: [checkin after 7/9] → [checkin after 7/1]
> The XHR code change point at workers,

does NOT point at workers, I meant.
Flags: sec-bounty? → sec-bounty+
(In reply to Daniel Veditz [:dveditz] from comment #15)
> Could we split those out and check them in after Fx23 ships in early August?

Sounds great.
Attachment #761048 - Attachment description: Patch, v1 → Patch, v1 [NOT FOR CHECKIN]
Created attachment 769735 [details] [diff] [review]
Patch for mozilla-esr17
Attachment #769735 - Flags: review+
Created attachment 769736 [details] [diff] [review]
Patch for mozilla-b2g18
Attachment #769736 - Flags: review+
Created attachment 769737 [details] [diff] [review]
Patch for mozilla-beta
Attachment #769737 - Flags: review+
Created attachment 769738 [details] [diff] [review]
Patch for mozilla-aurora
Attachment #769738 - Flags: review+
Created attachment 769739 [details] [diff] [review]
Patch for mozilla-central
Attachment #769739 - Flags: review+
Blocks: 888974
Filed bug 888974 for the additional tests.
Waiting on try server results for all these branch patches before they get checked in.
Created attachment 769744 [details] [diff] [review]
Patch for mozilla-esr17
Attachment #769735 - Attachment is obsolete: true
Attachment #769744 - Flags: review+
Created attachment 769753 [details] [diff] [review]
Patch for mozilla-b2g18
Attachment #769736 - Attachment is obsolete: true
Attachment #769753 - Flags: review+
https://hg.mozilla.org/releases/mozilla-esr17/rev/e943529a01e7
https://hg.mozilla.org/releases/mozilla-b2g18/rev/39607fd11f6b
https://hg.mozilla.org/releases/mozilla-beta/rev/3ec7f5434e27
https://hg.mozilla.org/releases/mozilla-aurora/rev/38ed376e845b
https://hg.mozilla.org/integration/mozilla-inbound/rev/51daaed82ceb
status-b2g18: affected → fixed
status-b2g18-v1.0.0: --- → affected
status-b2g18-v1.0.1: --- → affected
status-b2g-v1.1hd: --- → affected
status-firefox23: affected → fixed
status-firefox24: affected → fixed
status-firefox-esr17: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/51daaed82ceb
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox25: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/39607fd11f6b
status-b2g18-v1.0.0: affected → wontfix
status-b2g18-v1.0.1: affected → wontfix
status-b2g-v1.1hd: affected → fixed
Whiteboard: [checkin after 7/1]
Whiteboard: [adv-main23+][adv-esr1708+]
Alias: CVE-2013-1714
QA Contact: mwobensmith
This is not fixed on 17.0.8esr. I can still see the x-domain request sending a custom header via POST, and no preflight.

This is verified fixed, though, in the following builds:
m-c 2013-08-01
FF24 2013-07-10
FF23 2013-07-19
status-firefox23: fixed → verified
status-firefox24: fixed → verified
status-firefox25: fixed → verified
status-firefox-esr17: fixed → affected
This may mean that b2g18 1.1 isn't really fixed either. The only difference I see between the b2g18 and esr17 versions of the patch is that nsXMLHttpRequest::SetRequestHeader still checks "if (!privileged)" in ESR17 instead of replacing the UniversalXPConnect check with "if (!IsSystemXHR())"

Does the testcase checked in with the patch on ESR17 catch the failure? That is, did we not catch this because we're not running (or checking?) the tests on that branch, or because the test itself doesn't test the bug we're fixing?
(In reply to ben turner [:bent] from comment #4)
> smaug and/or I fixed something in the way that workers connected to their
> document's loadgroup a while back but I can't remember where the change
> was. Maybe smaug does?

If that change was after ESR17, perhaps it was not a fix in itself but was required to make this fix work correctly.

Couldn't find much that seemed to match the description except bug 753981. If that was it then b2g18 should be OK, and it should be easy enough to land that patch in ESR17 if it does the trick.
(In reply to Daniel Veditz [:dveditz] from comment #32)
> Does the testcase checked in with the patch on ESR17 catch the failure?

Nevermind, I asked in comment 18 that we not check in the tests until after ship (bug 888974). Ugh: good news, we didn't reveal the problem; bad news, we didn't catch the incomplete fix.
(In reply to Matt Wobensmith from comment #31)
> This is not fixed on 17.0.8esr. I can still see the x-domain request sending
> a custom header via POST, and no preflight.

What tool are you using for this? The builtin webdev tools aren't showing me anything for workers, and the LiveHttpHeaders extension isn't either...

Updated

4 years ago
Flags: needinfo?(mwobensmith)
I use Charles for inspecting the HTTP traffic. I staged the tests on a personal web server.
Flags: needinfo?(mwobensmith)
Created attachment 785478 [details] [diff] [review]
Additional patch for esr17

Additional patch for esr17.

I installed Charles. I see a GET request without this patch and an OPTIONS request with it.
Attachment #785478 - Flags: review?(jonas)
Attachment #785478 - Attachment description: changes.patch → Additional patch for esr17
Attachment #785478 - Flags: review?(jonas) → review+
Comment on attachment 785478 [details] [diff] [review]
Additional patch for esr17

[Approval Request Comment]
User impact if declined: Cross-origin data leaks
Fix Landed on Version: Everywhere but 17 (the original fix wasn't sufficient)
Risk to taking this patch (and alternatives if risky): This changes whether or not an OPTIONS request is generated for cross origin loads in workers (currently a normal GET/POST request is generated). It's really hard to say what sites will break if this is fixed. I haven't seen any bug reports since the change landed on other branches, but I don't know way to be completely confident.
String or UUID changes made by this patch: None
Attachment #785478 - Flags: approval-mozilla-esr17?
Comment on attachment 785478 [details] [diff] [review]
Additional patch for esr17

No new risk as compared to the original, approved patch.
Attachment #785478 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
Comment on attachment 785478 [details] [diff] [review]
Additional patch for esr17

Is it ok to check this in?
Attachment #785478 - Flags: sec-approval?
Attachment #785478 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/releases/mozilla-esr17/rev/8d80b1912bd7
status-firefox-esr17: affected → fixed
Fix looks great. I'll mark it verified once I check it on the official build.
Whiteboard: [adv-main23+][adv-esr1708+] → [adv-main23+]
Verified 2013-08-06, 17.0.8esr.
Status: RESOLVED → VERIFIED
(In reply to Matt Wobensmith from comment #43)

Thanks Matt!
Thanks Ben! :)
Group: core-security
You need to log in before you can comment on or make changes to this bug.