Closed Bug 1015540 (CVE-2014-1583) Opened 6 years ago Closed 6 years ago

Alarms API performs unsafe toJSON conversions on content objects

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 + wontfix
firefox32 + wontfix
firefox33 + fixed
firefox-esr24 --- unaffected
firefox-esr31 33+ fixed

People

(Reporter: bzbarsky, Assigned: selin)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main33+][adv-esr31.2+])

Attachments

(2 files, 4 obsolete files)

The "data" argument of AlarmsManager.add() is converted to a string using toJSON, which will run with chrome privileges and hence be able to grab the .href even from cross-origin location objects (esp once we convert Location to WebIDL).  Then we expose that data back to the page via getAll(), as far as I can tell.
Keywords: sec-high
OS: Mac OS X → All
Hardware: x86 → All
Gene, are you the right person to own this bug?

This API should definitely be converted to WebIDL. What we do after that is an interesting question. Are the field names for |data| known? The "spec" at [1] doesn't seem super detailed in this regard. If they are known, we could use a dictionary. If they aren't, but the _types_ of the values are known/consistent, we could use a mozMap. Alternatively, we could implement that [StructuredCloned] extended attribute that we were talking about in bug 899753 comment 16. Or something else?

[1] https://wiki.mozilla.org/WebAPI/AlarmAPI

Naively, it seems like we should convert this to WebIDL and use a dictionary for |data|. Does that make sense given the API? I can't find any spec for API, or what |data| is spposed
Flags: needinfo?(gene.lian)
(In reply to Boris Zbarsky [:bz] from comment #0)
> The "data" argument of AlarmsManager.add() is converted to a string using
> toJSON, which will run with chrome privileges and hence be able to grab the
> .href even from cross-origin location objects (esp once we convert Location
> to WebIDL).  Then we expose that data back to the page via getAll(), as far
> as I can tell.

Actually the current implementation of getAll() will only return the alarms that have been added by the *same* app (manifestURL is the same). Does that already avoid this security issue? Or it doesn't because even in the same app, add() and getAll() can be called on the different pages?
Flags: needinfo?(gene.lian)
(In reply to Bobby Holley (:bholley) from comment #1)
> Gene, are you the right person to own this bug?
> 
> This API should definitely be converted to WebIDL.

Yeap, this is an on-going task at bug 901114.

> What we do after that is
> an interesting question. Are the field names for |data| known? The "spec" at
> [1] doesn't seem super detailed in this regard. If they are known, we could
> use a dictionary. If they aren't, but the _types_ of the values are
> known/consistent, we could use a mozMap. Alternatively, we could implement
> that [StructuredCloned] extended attribute that we were talking about in bug
> 899753 comment 16. Or something else?

The |data| is a user-determined JSON object which will be stored with the alarm added by add(). The content (i.e., properties and values) of |data| will also be carried in the return of getAll() or in the system message when alarm fires. Therefore, I think we cannot use dictionary or interface to define that. Is my understanding correct?
Assignee: nobody → gene.lian
> Or it doesn't because even in the same app, add() and getAll() can be called on the
> different pages?

Can apps load cross-site iframes?  If so, should they be allowed to exfiltrate their locations?

If the answers are "yes" and "no" respectively, then that's where the security issue comes in: passing a cross-origin Location object to add() allows then extracting information from getAll() that was not available to the original script.
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #3)
> The |data| is a user-determined JSON object which will be stored with the
> alarm added by add(). The content (i.e., properties and values) of |data|
> will also be carried in the return of getAll() or in the system message when
> alarm fires. Therefore, I think we cannot use dictionary or interface to
> define that. Is my understanding correct?

Option 1: If the values are all of a few limited types (say, string or int), then you could use a MozMap, which would avoid the problems here.

Option 2: Introduce a [CloneArguments] extended attribute that causes the binding machinery to do a structured clone under the hood before passing the result to JS.

Option 3: Just have the API implementation do the stringification in a content-principaled sandbox.

Boris, what are your thoughts?
If I understand the API constrains here correctly, option 1 is not viable.

Option 3 is the simple and backportable thing to do.  We should still think about option 2 long-term.
Hi Boris, a couple of questions:

1. Based on the comment #0, I don't understand what do you mean by "converted to a string using toJSON". Actually, we didn't explicitly call any toJSON function in the codes. Which point are you mentioning about?

2. Would you please give me some directions/examples about option #3 in comment #5? I don't understand what "content-principaled sandbox" means. It sounds like the add() has do JSON.stringify(aData) on the child process side and pass the string around between child/b2g processes. It won't be restored to a JSON object until we want to return getAll() on the child process. Correct?
Flags: needinfo?(bzbarsky)
> Actually, we didn't explicitly call any toJSON function in the codes. Which point are you
> mentioning about?

I meant the moral equivalent of JSON.stringify, whether that's called from JS or C++.  If that's not called, how _is_ the value converted to a string?

> I don't understand what "content-principaled sandbox" means.

It means to create a sandbox using the principal of the content window the data came from, hand the value to the sandbox, and then evalInSandbox("JSON.stringify(value)") to get back a string.

> It sounds like the add() has do JSON.stringify(aData) on the child process side

Yes, correct.  I mean, that's what happens right now anyway, right?  But just with the system principal
Flags: needinfo?(bzbarsky)
Is this fixed now that bug 901114 has landed, or does further work need to be done?
Unfortunately, this is not fixed.  The data is passed through as "any" in the webidl.
Sean can do better than me on this one and has been working out a patch! Let him take it over.
Assignee: gene.lian → selin
Attached patch Patch v1 (obsolete) — Splinter Review
Trying to make a patch based on the discussion above.
Attachment #8451431 - Flags: review?(bzbarsky)
Comment on attachment 8451431 [details] [diff] [review]
Patch v1

Please add a test that would catch this bug?

In particular, I would expect that this test would show the patch doesn't work, since it's using a system-principal sandbox.  You want to use a sandbox with a principal that matches the web page calling you.
Attachment #8451431 - Flags: review?(bzbarsky) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Using a sandbox with the principal of the content window and adding a test case.
Attachment #8451431 - Attachment is obsolete: true
Attachment #8452200 - Flags: review?(bzbarsky)
Comment on attachment 8452200 [details] [diff] [review]
Patch v2

I believe this is still a security hole, though now it's more subtle.  

In particular, if I grab an AlarmsManager from some window, then navigate that window, then call into this code, then this._window.document will be the new document, whereas we want the old one.

In add() you could just use Cu.getWebIDLCallerPrincipal().  But in receiveMessage, you really need to know what principal you're associated with.  So you presumably want to save this._window.document.nodePrincipal in a member in init().  Looks like init() is getting the principal already anyway.

>+    var data = document.getElementById('ifr').contentWindow;

Does test fail without your patch and pass with it?

r=me assuming it does and if you fix the principal timing issue above.
Attachment #8452200 - Flags: review?(bzbarsky) → review+
Attached patch Patch v3Splinter Review
Minor updates based on the comments of the r+ review.

Besides, I've also verified the new test case fails without the patch and passes with it.
Attachment #8452200 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/cf143afd3029

Should this have had sec-approval before landing?
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Based on the filing date this affects mozilla32 at least, but how much earlier than that? Do we need to uplift to Beta (Firefox 31) or b2g28/b2g30? These are among the questions that would have been answered during the sec-approval process
https://wiki.mozilla.org/Security/Bug_Approval_Process

If it needs uplifting to 31 we're almost at the end of the line and need to do it ASAP.
Flags: needinfo?(selin)
This appears to have broken mozAlarms, per bug 1037079.
For reference, the affected app in 1037079 invokes mozAlarms.add with the following JSON:

{ type: 'timer' }
Depends on: 1037079
Depends on: 1037101
> This appears to have broken mozAlarms, per bug 1037079.

Do we not have unit tests for this?  :(

The place I'd start looking is in the AlarmsManager:GetAll:Return:OK bit.  It's not clear to me why we needed the sandbox there, and it's possible that Cu.cloneInto fails on the cross-compartment wrappers it ends up with there.
I just made a patch for review in bug 1037079 to fix the introduced issue. Please refer to that bug for more follow-ups.
Flags: needinfo?(selin)
No longer depends on: 1037101
Tracking for esr 31.
This was not uplifted to 32 or 31. Updating status flags.

Sean - Looks like this is needed on ESR31. Can you please create a branch patch (if required) and request approval to uplift?
Flags: needinfo?(selin)
Attached patch Patch ESR31 (obsolete) — Splinter Review
I just combined the fix for this bug and the one for bug 1037079 into the patch for ESR31 only.
Attachment #8481107 - Flags: review+
Flags: needinfo?(selin)
Attached patch Patch ESR31, v2 (obsolete) — Splinter Review
Minor fix since Alarms API hasn't been migrated to WebIDL yet in ESR31.
Attachment #8481107 - Attachment is obsolete: true
Attached patch Patch ESR31, v3Splinter Review
Sorry, the previous version was attached by mistake. Uploading the correct one.
Attachment #8482093 - Attachment is obsolete: true
Try run for the latest ESR31 patch. [1]

Some issues appear already in current ESR31 code base which fails some e10s Mochitest and Marionette tests. But they don't seem triggered by this patch since the try run without the patch has similar results. [2]

[1] https://tbpl.mozilla.org/?tree=Try&rev=aac1de79d172
[2] https://tbpl.mozilla.org/?tree=Try&rev=1e310cff9e69
Comment on attachment 8482097 [details] [diff] [review]
Patch ESR31, v3

[Security approval request comment]
Based on comment 27, it looks the patch is needed for ESR31 and need sec-approval.

* How easily could an exploit be constructed based on the patch?
Use a sandbox with a principal that matches the calling web page to prevent toJSON operation on cross-origin location objects.

* 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?
ESR31. (The fix has already been landed to Mozilla central.)

* If not all supported branches, which bug introduced the flaw?
It has been in the code base since Alarms API was implemented.

* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
* How likely is this patch to cause regressions; how much testing does it need?
It has been try-run with the ESR31 version. The patch for ESR31 is small and quite similar to the patch landed in Mozilla central. (There's one line difference since Alarms API in ESR31 hasn't been migrated to WebIDL.) And there are correspondent test cases included in this patch. So it's less likely to be risky.
Attachment #8482097 - Flags: sec-approval?
Why is this needed on esr31? This whole API is b2g-only AFAICT.
(In reply to Bobby Holley (:bholley) from comment #33)
> Why is this needed on esr31? This whole API is b2g-only AFAICT.

Is this true?
(In reply to Al Billings [:abillings] from comment #34)
> (In reply to Bobby Holley (:bholley) from comment #33)
> > Why is this needed on esr31? This whole API is b2g-only AFAICT.
> 
> Is this true?

http://mxr.mozilla.org/mozilla-central/search?string=dom.mozAlarms.enabled

Looks like the WebAppRT is also affected, not sure if that matters.
Trying to figure out if this is b2g only or if it affects Desktop. Can you give more information?
Flags: needinfo?(selin)
Actually I don't know much about WebAppRT. But based on the description "/webapprt: desktop stub executable launcher and shell" in https://wiki.mozilla.org/Apps/WebRT#Source_Code, it appears desktop relevant.

Hi Sylvestre,

I noticed the esr31 flag was added by you. Could you provide more info about why it's affected?
Flags: needinfo?(selin) → needinfo?(sledru)
Sean, ESR31 is affected because we marked Firefox 31 as "Won't Fix" which means it is affected but we chose not to fix it (or didn't get patches done in time). Since this is a sec-high fixed elsewhere, we would normally port it to affected ESR branches. 

This should have had sec-approval before it was checked in first, by the way.
Flags: needinfo?(sledru)
Attachment #8482097 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
Whiteboard: Please only check in "Patch ESR31" to ESR31 branch.
sec-approval isn't approval to land. You still need to nominate this for esr31 approval.
https://wiki.mozilla.org/Security/Bug_Approval_Process
Keywords: checkin-needed
Whiteboard: Please only check in "Patch ESR31" to ESR31 branch.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #39)
> sec-approval isn't approval to land. 

Well, it is approval to land on trunk...
Ryan,

I need this landed as I need to GTB.
Flags: needinfo?(ryanvm)
(In reply to Al Billings [:abillings] from comment #40)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #39)
> > sec-approval isn't approval to land. 
> 
> Well, it is approval to land on trunk...

It was already landed on trunk and he was setting checkin-needed for an uplift. As of now, this *still* doesn't have an esr31 approval on it. Can we please stick to following practices?
Flags: needinfo?(ryanvm)
Attachment #8482097 - Flags: approval-mozilla-esr31+
Whiteboard: [adv-main33+][adv-esr31.2+]
Alias: CVE-2014-1583
I ran the mochitest in the first patch against Fx31 and Fx32 to see if it fails there, but it appears to pass in both. Also passes in Fx33. So, I can't really say that I've verified this bug as fixed. If anyone would like more testing on this, please feel free to give me more info, but for now, I'm going to mark [qe-verify-].
Flags: qe-verify-
Depends on: 1090896
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.