Last Comment Bug 879787 - (CVE-2013-1714) Cross Domain Policy override using webworkers
(CVE-2013-1714)
: Cross Domain Policy override using webworkers
Status: VERIFIED FIXED
[adv-main23+]
: sec-high
Product: Core
Classification: Components
Component: DOM: Workers (show other bugs)
: 18 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla25
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
: Matt Wobensmith [:mwobensmith][:matt:]
Mentors:
Depends on:
Blocks: 888974
  Show dependency treegraph
 
Reported: 2013-06-05 07:56 PDT by federico lanusse
Modified: 2014-07-24 16:08 PDT (History)
15 users (show)
abillings: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
+
verified
+
verified
verified
23+
fixed
?
fixed
wontfix
wontfix
fixed


Attachments
sorry, i needed to zip all together (98.45 KB, application/java-archive)
2013-06-05 07:56 PDT, federico lanusse
no flags Details
Patch, v1 (3.49 KB, patch)
2013-06-10 14:06 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Splinter Review
Patch, v1 [NOT FOR CHECKIN] (18.55 KB, patch)
2013-06-11 11:21 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
bugs: review+
abillings: sec‑approval+
Details | Diff | Splinter Review
Patch for mozilla-esr17 (12.53 KB, patch)
2013-07-01 10:05 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
Details | Diff | Splinter Review
Patch for mozilla-b2g18 (12.88 KB, patch)
2013-07-01 10:06 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
Details | Diff | Splinter Review
Patch for mozilla-beta (12.83 KB, patch)
2013-07-01 10:07 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
Details | Diff | Splinter Review
Patch for mozilla-aurora (12.83 KB, patch)
2013-07-01 10:08 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
Details | Diff | Splinter Review
Patch for mozilla-central (12.83 KB, patch)
2013-07-01 10:09 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
Details | Diff | Splinter Review
Patch for mozilla-esr17 (13.97 KB, patch)
2013-07-01 10:22 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
Details | Diff | Splinter Review
Patch for mozilla-b2g18 (14.32 KB, patch)
2013-07-01 10:50 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
Details | Diff | Splinter Review
Additional patch for esr17 (1.21 KB, patch)
2013-08-03 22:29 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
jonas: review+
akeybl: approval‑mozilla‑esr17+
abillings: sec‑approval+
Details | Diff | Splinter Review

Description federico lanusse 2013-06-05 07:56:02 PDT
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.
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-06-05 08:46:29 PDT
Does this happen in the latest version of Firefox (21 I believe)?
Comment 2 federico lanusse 2013-06-05 10:52:43 PDT
not sure, I have firefox 18 in my current fedora 16
Comment 3 Andrew McCreight [:mccr8] 2013-06-05 10:57:38 PDT
Matt is going to try it out on a more recent version.
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-06-05 12:08:47 PDT
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...
Comment 5 Matt Wobensmith [:mwobensmith][:matt:] 2013-06-06 11:42:32 PDT
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.
Comment 6 Matt Wobensmith [:mwobensmith][:matt:] 2013-06-06 11:46:25 PDT
Oh, and of course the obvious - yes, this does happen in the current FF21.
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-06-10 14:06:58 PDT
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.
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-06-10 16:59:55 PDT
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.
Comment 10 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-06-10 19:53:46 PDT
And tests/content/base/test/test_bug383430.html
Comment 11 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-06-11 11:21:53 PDT
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...
Comment 12 Olli Pettay [:smaug] (vacation Aug 25-28) 2013-06-11 12:15:15 PDT
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.
Comment 13 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-06-13 09:55:33 PDT
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.
Comment 14 Al Billings [:abillings] 2013-06-13 11:44:36 PDT
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).
Comment 15 Daniel Veditz [:dveditz] 2013-06-13 12:59:58 PDT
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.
Comment 16 Daniel Veditz [:dveditz] 2013-06-13 13:02:05 PDT
> The XHR code change point at workers,

does NOT point at workers, I meant.
Comment 18 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-06-17 16:25:30 PDT
(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.
Comment 19 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-01 10:05:28 PDT
Created attachment 769735 [details] [diff] [review]
Patch for mozilla-esr17
Comment 20 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-01 10:06:25 PDT
Created attachment 769736 [details] [diff] [review]
Patch for mozilla-b2g18
Comment 21 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-01 10:07:32 PDT
Created attachment 769737 [details] [diff] [review]
Patch for mozilla-beta
Comment 22 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-01 10:08:13 PDT
Created attachment 769738 [details] [diff] [review]
Patch for mozilla-aurora
Comment 23 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-01 10:09:50 PDT
Created attachment 769739 [details] [diff] [review]
Patch for mozilla-central
Comment 24 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-01 10:13:13 PDT
Filed bug 888974 for the additional tests.
Comment 25 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-01 10:18:22 PDT
Waiting on try server results for all these branch patches before they get checked in.
Comment 26 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-01 10:22:48 PDT
Created attachment 769744 [details] [diff] [review]
Patch for mozilla-esr17
Comment 27 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-01 10:50:40 PDT
Created attachment 769753 [details] [diff] [review]
Patch for mozilla-b2g18
Comment 29 Phil Ringnalda (:philor) 2013-07-04 10:19:17 PDT
https://hg.mozilla.org/mozilla-central/rev/51daaed82ceb
Comment 31 Matt Wobensmith [:mwobensmith][:matt:] 2013-08-02 16:20:38 PDT
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
Comment 32 Daniel Veditz [:dveditz] 2013-08-02 18:09:30 PDT
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?
Comment 33 Daniel Veditz [:dveditz] 2013-08-02 18:20:45 PDT
(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.
Comment 34 Daniel Veditz [:dveditz] 2013-08-02 18:35:29 PDT
(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.
Comment 35 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-02 23:19:27 PDT
(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...
Comment 36 Matt Wobensmith [:mwobensmith][:matt:] 2013-08-03 13:24:02 PDT
I use Charles for inspecting the HTTP traffic. I staged the tests on a personal web server.
Comment 37 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-03 22:29:01 PDT
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.
Comment 38 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-05 10:46:13 PDT
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
Comment 39 Alex Keybl [:akeybl] 2013-08-05 10:56:07 PDT
Comment on attachment 785478 [details] [diff] [review]
Additional patch for esr17

No new risk as compared to the original, approved patch.
Comment 40 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-05 11:00:58 PDT
Comment on attachment 785478 [details] [diff] [review]
Additional patch for esr17

Is it ok to check this in?
Comment 41 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-05 11:22:03 PDT
https://hg.mozilla.org/releases/mozilla-esr17/rev/8d80b1912bd7
Comment 42 Matt Wobensmith [:mwobensmith][:matt:] 2013-08-05 11:41:23 PDT
Fix looks great. I'll mark it verified once I check it on the official build.
Comment 43 Matt Wobensmith [:mwobensmith][:matt:] 2013-08-06 08:54:33 PDT
Verified 2013-08-06, 17.0.8esr.
Comment 44 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-08-06 08:59:20 PDT
(In reply to Matt Wobensmith from comment #43)

Thanks Matt!
Comment 45 Matt Wobensmith [:mwobensmith][:matt:] 2013-08-06 11:46:15 PDT
Thanks Ben! :)

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