Closed
Bug 1015540
(CVE-2014-1583)
Opened 10 years ago
Closed 10 years ago
Alarms API performs unsafe toJSON conversions on content objects
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bzbarsky, Assigned: selin)
References
Details
(Keywords: sec-high, Whiteboard: [adv-main33+][adv-esr31.2+])
Attachments
(2 files, 4 obsolete files)
7.38 KB,
patch
|
Details | Diff | Splinter Review | |
8.36 KB,
patch
|
abillings
:
approval-mozilla-esr31+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Comment 1•10 years ago
|
||
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
Updated•10 years ago
|
Flags: needinfo?(gene.lian)
Comment 2•10 years ago
|
||
(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)
Comment 3•10 years ago
|
||
(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?
Updated•10 years ago
|
Assignee: nobody → gene.lian
![]() |
Reporter | |
Comment 4•10 years ago
|
||
> 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.
Comment 5•10 years ago
|
||
(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?
![]() |
Reporter | |
Comment 6•10 years ago
|
||
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.
Comment 7•10 years ago
|
||
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)
![]() |
Reporter | |
Comment 8•10 years ago
|
||
> 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)
Comment 9•10 years ago
|
||
Is this fixed now that bug 901114 has landed, or does further work need to be done?
![]() |
Reporter | |
Comment 10•10 years ago
|
||
Unfortunately, this is not fixed. The data is passed through as "any" in the webidl.
Comment 11•10 years ago
|
||
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
Assignee | ||
Comment 12•10 years ago
|
||
Trying to make a patch based on the discussion above.
Attachment #8451431 -
Flags: review?(bzbarsky)
![]() |
Reporter | |
Comment 13•10 years ago
|
||
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-
Assignee | ||
Comment 14•10 years ago
|
||
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)
![]() |
Reporter | |
Comment 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
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
Assignee | ||
Comment 17•10 years ago
|
||
Try run for this patch. FYI. https://tbpl.mozilla.org/?tree=Try&rev=e7684e943373
Keywords: checkin-needed
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf143afd3029
Keywords: checkin-needed
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cf143afd3029 Should this have had sec-approval before landing?
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox33:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 20•10 years ago
|
||
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.
status-firefox31:
--- → ?
status-firefox32:
--- → affected
tracking-firefox31:
--- → ?
tracking-firefox32:
--- → +
tracking-firefox33:
--- → +
Flags: needinfo?(selin)
Keywords: regressionwindow-wanted
Comment 21•10 years ago
|
||
This appears to have broken mozAlarms, per bug 1037079.
Comment 22•10 years ago
|
||
For reference, the affected app in 1037079 invokes mozAlarms.add with the following JSON: { type: 'timer' }
![]() |
Reporter | |
Comment 23•10 years ago
|
||
> 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.
Assignee | ||
Comment 24•10 years ago
|
||
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)
Comment 25•10 years ago
|
||
31 is affected based on comment #20
Updated•10 years ago
|
status-firefox-esr24:
--- → unaffected
Comment 26•10 years ago
|
||
Tracking for esr 31.
status-firefox-esr31:
--- → affected
tracking-firefox-esr31:
--- → +
Updated•10 years ago
|
Comment 27•10 years ago
|
||
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)
Assignee | ||
Comment 28•10 years ago
|
||
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)
Assignee | ||
Comment 29•10 years ago
|
||
Minor fix since Alarms API hasn't been migrated to WebIDL yet in ESR31.
Attachment #8481107 -
Attachment is obsolete: true
Assignee | ||
Comment 30•10 years ago
|
||
Sorry, the previous version was attached by mistake. Uploading the correct one.
Attachment #8482093 -
Attachment is obsolete: true
Assignee | ||
Comment 31•10 years ago
|
||
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
Assignee | ||
Comment 32•10 years ago
|
||
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?
Comment 33•10 years ago
|
||
Why is this needed on esr31? This whole API is b2g-only AFAICT.
Comment 34•10 years ago
|
||
(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?
Comment 35•10 years ago
|
||
(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.
Comment 36•10 years ago
|
||
Trying to figure out if this is b2g only or if it affects Desktop. Can you give more information?
Flags: needinfo?(selin)
Assignee | ||
Comment 37•10 years ago
|
||
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)
Comment 38•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8482097 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Whiteboard: Please only check in "Patch ESR31" to ESR31 branch.
Comment 39•10 years ago
|
||
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.
Comment 40•10 years ago
|
||
(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...
Comment 42•10 years ago
|
||
(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)
Updated•10 years ago
|
Attachment #8482097 -
Flags: approval-mozilla-esr31+
Updated•10 years ago
|
Whiteboard: [adv-main33+][adv-esr31.2+]
Updated•10 years ago
|
Alias: CVE-2014-1583
Comment 44•10 years ago
|
||
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-
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Keywords: regressionwindow-wanted
Updated•9 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•