Closed Bug 927196 (CVE-2014-1503) Opened 6 years ago Closed 6 years ago

XMLHttpRequest({mozAnon: false, mozSystem: true}) seems to work when it shouldn't

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g -
Tracking Status
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
firefox-esr17 --- wontfix
firefox-esr24 --- wontfix
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- wontfix

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

()

Details

(Keywords: sec-moderate, Whiteboard: [adv-main28+])

Attachments

(1 file, 8 obsolete files)

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.
"
Attached patch mozanon.diff (obsolete) — Splinter Review
So I think you want it to behave like this, right?
Attachment #817518 - Flags: review?(jonas)
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)
This is a security issue :)
Group: core-security
Does this affect all web content? Or just privileged apps? This affects how we'd rate the security impact.
Just privileged apps, afaict.
Attached patch mozanon.diff (obsolete) — Splinter Review
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
Attachment #817518 - Attachment is obsolete: true
Attachment #817518 - Flags: review?(jonas)
Attachment #817993 - Flags: review?(jonas)
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
Attachment #817993 - Flags: review?(jonas) → review+
We should take this for 1.2.
blocking-b2g: --- → koi?
Thanks for fixing this!
Assignee: nobody → martijn.martijn
I assume this is something we only care about for b2g.
Attached patch mozanon.diff (for check-in) (obsolete) — Splinter Review
Attachment #817993 - Attachment is obsolete: true
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.
Keywords: checkin-needed
Let's uplift this for 1.2
blocking-b2g: koi? → koi+
Should the commit message be obscured more before I land this checkin-needed?
Removing checkin-needed pending comment 15.
Keywords: checkin-needed
Jonas, what do you think the checkin-needed comment should be here?  Do you think it is fine as is?
Flags: needinfo?(jonas)
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.
Flags: needinfo?(jonas)
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.
Oh, desktop mochitest-1 too, they just hadn't finished yet.
Attached patch mozanon.diff (obsolete) — Splinter Review
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.
Attachment #819397 - Attachment is obsolete: true
Attachment #825420 - Flags: review?(jonas)
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?
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.
Attached patch mozanon.diff (obsolete) — Splinter Review
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
Attachment #825420 - Attachment is obsolete: true
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.
Attachment #827527 - Flags: review?(jonas)
Keywords: checkin-needed
Bitrotted, please rebase.

Also, do we want this on b2g18 (v1.1) as well? If not, please set the status to wontfix.
Attached patch mozanon.diff (obsolete) — Splinter Review
Sorry, again asking for review.
The part in dom/workers/XMLHttpRequest.cpp was bitrotted, so I had to make some changes there.
Attachment #827527 - Attachment is obsolete: true
Attachment #830944 - Flags: review?(jonas)
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.
Attachment #830944 - Flags: review?(jonas) → review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e6a9c98d62d0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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.
Flags: needinfo?(martijn.martijn)
Depends on: 939269
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.)
Renom - we've already shipped a release with this bug & it's risky.
blocking-b2g: koi+ → koi?
Paul - Can you give input from a security perspective if this needs to block 1.2?
Flags: needinfo?(ptheriault)
Attached patch gaiamozanon.diff (obsolete) — Splinter Review
This fixes the consumers in gaia that use XMLHttpRequest with mozSystem: true.
I hope I have them all in this patch.
Attachment #8334631 - Flags: review?(jonas)
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.
Attachment #8334631 - Flags: review?(jonas) → review?(21)
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?
(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.
Flags: needinfo?(martijn.martijn) → needinfo?(jonas)
(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?
Flags: needinfo?(ptheriault)
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.
Flags: needinfo?(jonas)
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.
Attachment #8334631 - Flags: review?(21)
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.
(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
blocking-b2g: koi? → -
Attached patch 927196.diff (obsolete) — Splinter Review
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
Attachment #830944 - Attachment is obsolete: true
Attachment #8334631 - Attachment is obsolete: true
Comment on attachment 8341810 [details] [diff] [review]
927196.diff

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

Tryserver seems to be going well.
Attachment #8341810 - Flags: review?(jonas)
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;
Attachment #8341810 - Flags: review?(jonas) → review+
With review nit addressed.
Attachment #8341810 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bcda0491338b
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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?
Depends on: 956893
Depends on: 962377
Whiteboard: [adv-main28+]
Alias: CVE-2014-1503
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.