879787 - (CVE-2013-1714) Cross Domain Policy override using webworkers

Status: VERIFIED FIXED

(Core :: DOM: Workers, defect)

Description

[federico lanusse]

Attachment: 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!)]

Does this happen in the latest version of Firefox (21 I believe)?

Comment 2

[federico lanusse]

not sure, I have firefox 18 in my current fedora 16

Comment 3

[Andrew McCreight [:mccr8]]

Matt is going to try it out on a more recent version.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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:]]

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:]]

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!)]

Attachment: Patch, v1

This happens basically because XHR is currently using "IsCallerChrome()" rather than "IsSystemXHR()" to make some security decisions.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://tbpl.mozilla.org/?tree=Try&rev=0f8dd485410d
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0f8dd485410d

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!)]

And tests/content/base/test/test_bug383430.html

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 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][[e-mail]]]

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!)]

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 - ex-MoCo]]

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]]

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]]

> 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!)]

(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!)]

Attachment: Patch for mozilla-esr17

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-b2g18

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-beta

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-aurora

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-central

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Filed bug 888974 for the additional tests.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!)]

Attachment: Patch for mozilla-esr17

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-b2g18

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/51daaed82ceb

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/39607fd11f6b

Comment 31

[Matt Wobensmith [:mwobensmith][:matt:]]

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]]

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]]

(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]]

(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!)]

(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:]]

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!)]

Attachment: 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!)]

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]]

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!)]

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!)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/8d80b1912bd7

Comment 42

[Matt Wobensmith [:mwobensmith][:matt:]]

Fix looks great. I'll mark it verified once I check it on the official build.

Comment 43

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified 2013-08-06, 17.0.8esr.

Comment 44

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Matt Wobensmith from comment #43)

Thanks Matt!

Comment 45

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Ben! :)