927196 - (CVE-2014-1503) XMLHttpRequest({mozAnon: false, mozSystem: true}) seems to work when it shouldn't

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

I added this in bug 901343, in place of createSystemXHR, but I had a problem with not being able to use mozAnon: true in combination with mozSystem: true in XMLHtppRequest. See bug 901343, comment 12.

I just tested it again, and it seemed to work fine now. I don't know why I did get these errors, but I'll change the affected test files to mozAnon: true.

But now the real bug is that mozAnon: false seems to work with mozSystem: true.
According to the documentation:
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest?redirectlocale=en-US&redirectslug=DOM%2FXMLHttpRequest#XMLHttpRequest%28%29
"
 mozSystem
    Boolean: Setting this flag to true allows making cross-site connections without requiring the server to opt-in using CORS. Requires setting mozAnon: true. I.e. this can't be combined with sending cookies or other user credentials. This only works in privileged (reviewed) apps; it does not work on arbitrary webpages loaded in Firefox.
"

Comment 1

[Martijn Wargers (dead)]

Attachment: mozanon.diff

So I think you want it to behave like this, right?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 817518 [details] [diff] [review]
mozanon.diff

No, I want us to throw an exception if someone does

xhr = new XHR({ mozAnon: false, mozSystem: true});

This patch makes us silently ignore both properties.

Would you mind changing so that we throw? (and obviously include a test to ensure that we throw)

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is a security issue :)

Comment 5

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

Does this affect all web content? Or just privileged apps? This affects how we'd rate the security impact.

Comment 6

[Martijn Wargers (dead)]

Just privileged apps, afaict.

Comment 7

[Martijn Wargers (dead)]

Attachment: mozanon.diff

Like this? This makes it throw, but I don't know which kind of error you would like to throw.

Also, I added in this patch the ability to use mozAnon: false, with mozSystem: false. Because that should be possible, afaik:
http://www.w3.org/TR/XMLHttpRequest/#constructors

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 817993 [details] [diff] [review]
mozanon.diff

Review of attachment 817993 [details] [diff] [review]:
-----------------------------------------------------------------

Yay! Thanks!!

::: content/base/src/nsXMLHttpRequest.cpp
@@ -390,5 @@
>    }
>  
>    // Chrome is always allowed access, so do the permission check only
>    // for non-chrome pages.
> -  if (!IsSystemXHR()) {

Whoa! Good catch!

::: content/base/test/test_bug927196.html
@@ +28,5 @@
> +  }
> +
> +  req = new XMLHttpRequest({mozAnon: true});
> +  is(req.mozAnon, true, "XMLHttpRequest should be mozAnon");
> +  is(req.mozSystem, false, "XMLHttpRequest should be mozSystem");

...should *not* be...

@@ +38,5 @@
> +  try {
> +    req = new XMLHttpRequest({mozAnon: false, mozSystem: true});
> +    ok(false, "Should not be reached");
> +  } catch(e) {
> +    is(e.name, "SecurityError", "XMLHttpRequest should not be mozSystem");

"Should throw SecurityError" or some such

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We should take this for 1.2.

Comment 10

[Andrew McCreight [:mccr8]]

Thanks for fixing this!

Comment 11

[Andrew McCreight [:mccr8]]

I assume this is something we only care about for b2g.

Comment 12

[Martijn Wargers (dead)]

Attachment: mozanon.diff (for check-in)

Comment 13

[Martijn Wargers (dead)]

I addressed the review comments with that patch. I assume I can't test the patch on tryserver, since this is a security bug?
In any case, it should be pretty safe to land, I don't expect it to cause failures (but I said that before). Anyway, I'm on vacation tomorrow and I won't be able to communicate at all during that time, probably. Just so you know.

Comment 14

[Andrew Overholt [:overholt]]

Let's uplift this for 1.2

Comment 15

[Ed Morley [:emorley]]

Should the commit message be obscured more before I land this checkin-needed?

Comment 16

[Ed Morley [:emorley]]

Removing checkin-needed pending comment 15.

Comment 17

[Andrew McCreight [:mccr8]]

Jonas, what do you think the checkin-needed comment should be here?  Do you think it is fine as is?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't know what our policies around checkin comments are these days. I'll defer to the security team.

This bug only permits someone to have more access than they should once they have submitted a privileged app to the marketplace and gotten a review by the marketplace team. And at that point only affects platforms that support privileged apps, i.e. only Firefox OS.

It would permit the attacker to read/write data from other websites that the user has logged in to in the privileged app. I.e. the user has to log in to his bank, or to facebook, inside the attackers app. The attacker could then read data from, or post data to, the bank or facebook. And circumvent CSRF protection mechanisms while doing so.

Comment 19

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df17cff05b54

Comment 20

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/93e3355e9a6e - desktop mochitest-3, test_systemXHR.html timed out (which is Android mochitest-6 apparently), and Android mochitest-1, test_XHR_parameters.html had multiple failures and test_XHR_system.html had one.

Comment 21

[Phil Ringnalda (:philor)]

Oh, desktop mochitest-1 too, they just hadn't finished yet.

Comment 22

[Martijn Wargers (dead)]

Attachment: mozanon.diff

Sorry, I screwed up big time. Every xmlhttprequest with mozSystem: true needed to have mozAnon: true added with this patch.
Also, xmlhttprequest.cpp for workers had to be fixed.

Comment 23

[Martijn Wargers (dead)]

If I manage to get a patch with r+, I really need it to test against tryserver, because not doing that caused utter failure. But that seems against the policy of security sensitive bugs, isn't it, Paul?

Comment 24

[Andrew McCreight [:mccr8]]

It already landed once, so don't worry about it too much.  Anybody already watching logs has seen the patch already.  Just make sure the commit message is empty when you push to try.

Comment 25

[Martijn Wargers (dead)]

Ok, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=74cac96a61d9

Comment 26

[Martijn Wargers (dead)]

Attachment: mozanon.diff

Sorry, there was 1 mochitest failure in b2g that I managed to miss. This is very likely happening, because of the systemXHR permission in the manifest. I removed that permission in this patch.
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=35fa26c07175

Comment 27

[Martijn Wargers (dead)]

Comment on attachment 827527 [details] [diff] [review]
mozanon.diff

Review of attachment 827527 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, everything green on try. Only change from last path is the removal of systemXHR from the manifest file.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Bitrotted, please rebase.

Also, do we want this on b2g18 (v1.1) as well? If not, please set the status to wontfix.

Comment 29

[Martijn Wargers (dead)]

Attachment: mozanon.diff

Sorry, again asking for review.
The part in dom/workers/XMLHttpRequest.cpp was bitrotted, so I had to make some changes there.

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 830944 [details] [diff] [review]
mozanon.diff

Review of attachment 830944 [details] [diff] [review]:
-----------------------------------------------------------------

If all you are doing is sync to tip you don't need to as for review again.

Comment 31

[Martijn Wargers (dead)]

Well, dom/workers/XMLHttpRequest.cpp was changed quite a bit, but I don't think I really changed anything essential with the new patch, so adding checkin-needed keyword.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e6a9c98d62d0

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e6a9c98d62d0

Comment 34

[Ryan VanderMeulen [:RyanVM]]

I took a stab at applying this to b2g26, but it's going to need some fixing up from someone who understands the tests better than me.

Comment 35

[Ed Morley [:emorley]]

I was asked by Rik in #b2g to back this out for causing bug 939269.

Backout:
remote:   https://hg.mozilla.org/mozilla-central/rev/beddd6d4bcdf

Comment 36

[Andrew Sutherland [:asuth] (he/him)]

As noted on bug 939269 (and the email breakage in bug 939769), this will require some changes to Gaia.  Do we have automatic approval to make changes in our apps and uplift to all appropriate Gaia branches, or do we plan to use a safer version for uplifts?  (AKA one that just silently turns on mozAnon when mozSystem is used.)

Comment 37

[Jason Smith [:jsmith]]

Renom - we've already shipped a release with this bug & it's risky.

Comment 38

[Jason Smith [:jsmith]]

Paul - Can you give input from a security perspective if this needs to block 1.2?

Comment 39

[Martijn Wargers (dead)]

Attachment: gaiamozanon.diff

This fixes the consumers in gaia that use XMLHttpRequest with mozSystem: true.
I hope I have them all in this patch.

Comment 40

[Andrew McCreight [:mccr8]]

Comment on attachment 8334631 [details] [diff] [review]
gaiamozanon.diff

Review of attachment 8334631 [details] [diff] [review]:
-----------------------------------------------------------------

You need to get a review from a Gaia peer here, not Jonas.

Comment 41

[Anthony Ricaud (:rik)]

As I said in bug 939269 comment 20, changing this will break 3rd party apps (it already broke Gaia apps). If 3rd party apps shouldn't use mozAnon: false with mozSystem: true, why aren't we making mozAnon default to true when not specified?

Comment 42

[Martijn Wargers (dead)]

(In reply to Anthony Ricaud (:rik) from comment #41)
> As I said in bug 939269 comment 20, changing this will break 3rd party apps
> (it already broke Gaia apps). If 3rd party apps shouldn't use mozAnon: false
> with mozSystem: true, why aren't we making mozAnon default to true when not
> specified?

That sounds like a good idea to me, Jonas Sicking can answer that question, I think.

Comment 43

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Jason Smith [:jsmith] from comment #38)
> Paul - Can you give input from a security perspective if this needs to block
> 1.2?

As Jonas described above in comment 18, it allows apps which have requested the SystemXHR permission to bypass same-origin policy (a usually 'critical' security vulnerability). I don't think relying on marketplace to catch apps using this API dangerously or malicious is a wise strategy so we really need to uplift this patch as far as we can.


(In reply to Martijn Wargers [:mwargers] (QA) from comment #42)
> (In reply to Anthony Ricaud (:rik) from comment #41)
> > As I said in bug 939269 comment 20, changing this will break 3rd party apps
> > (it already broke Gaia apps). If 3rd party apps shouldn't use mozAnon: false
> > with mozSystem: true, why aren't we making mozAnon default to true when not
> > specified?
> 
> That sounds like a good idea to me, Jonas Sicking can answer that question,
> I think.

That sounds good to me - actually that is how I thought this API worked we reviewed this previously. Can we just change the logic to be: if mozSystem==true then mozAnon is set to false, and raise an exception someone attempts to set both mozAnon & mozSystem to true?

That means the old code with work, and newer can use {mozAnon:true, mozSystem:true}, is that correct? Or am I misunderstanding and the existing code actually relies upon the mozAnon:false behavior?

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's a great idea to always set mozAnon to true when mozSystem is set to true. I.e. when mozSystem is set to true I think we can ignore what's passed in mozAnon and always pretend true was passed for that too.

Comment 45

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8334631 [details] [diff] [review]
gaiamozanon.diff

Seems like I don't need to review this as it should be implicit in the platform.

Comment 46

[Lucas Adamski [:ladamski]]

There is significant risk of regressions given we already hit one in Gaia and other privileged apps have likely used this not realizing they had to request the systemXHR permission.  Without a way of doing a pass in marketplace to verify how many apps would be broken I'm inclined to let this ride the trains for 1.3 instead.

Comment 47

[Alex Keybl [:akeybl]]

(In reply to Lucas Adamski [:ladamski] (plz needinfo) from comment #46)
> There is significant risk of regressions given we already hit one in Gaia
> and other privileged apps have likely used this not realizing they had to
> request the systemXHR permission.  Without a way of doing a pass in
> marketplace to verify how many apps would be broken I'm inclined to let this
> ride the trains for 1.3 instead.

koi- in that case, thanks Lucas

Comment 48

[Martijn Wargers (dead)]

Attachment: 927196.diff

Ok, updated the patch to always set mozAnon to true when mozAnon is true.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=f5b9f5badb3e

Comment 49

[Martijn Wargers (dead)]

Comment on attachment 8341810 [details] [diff] [review]
927196.diff

Review of attachment 8341810 [details] [diff] [review]:
-----------------------------------------------------------------

Tryserver seems to be going well.

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8341810 [details] [diff] [review]
927196.diff

Review of attachment 8341810 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsXMLHttpRequest.h
@@ +238,5 @@
>    {
> +    if (aSystem)
> +      mIsAnon = true;
> +    else
> +      mIsAnon = aAnon;

mIsAnon = aAnon || aSystem;

Comment 51

[Martijn Wargers (dead)]

Attachment: 927196.diff (for check-in)

With review nit addressed.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/bcda0491338b

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bcda0491338b

Comment 54

[Antonio Manuel Amaya Calvo (:amac)]

Sorry for piping in this late. I just discovered this because it broke the Facebook logout on contacts (bug 956893). And... I thought the whole idea behind mozSystem was precisely allowing bypassing CORS and similar measures for some apps. That is, allowing some apps to behave as if they weren't inside a browser session/frame. This change breaks that assumption, and... Why do we need two flags? mozAnom could imply not setting the origin field also, since I will only be able to fetch what's completely public. 

I think this change makes mozSystem kinda useless. And we have another permission (TCPSocket) that's also privileged and that would allow the same kind of attacks with some more work. 

Can we get back a real mozSystem permission for certified apps at least?

Comment 55

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Jonas is on vacation until Tuesday.