1015540 - (CVE-2014-1583) Alarms API performs unsafe toJSON conversions on content objects

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Gene Lian (I already quit Mozilla)]

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

Comment 3

[Gene Lian (I already quit Mozilla)]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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

[Bobby Holley (:bholley)]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

[Gene Lian (I already quit Mozilla)]

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?

Comment 8

[Boris Zbarsky [: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

Comment 9

[Andrew McCreight [:mccr8]]

Is this fixed now that bug 901114 has landed, or does further work need to be done?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Unfortunately, this is not fixed.  The data is passed through as "any" in the webidl.

Comment 11

[Gene Lian (I already quit Mozilla)]

Sean can do better than me on this one and has been working out a patch! Let him take it over.

Comment 12

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v1

Trying to make a patch based on the discussion above.

Comment 13

[Boris Zbarsky [: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.

Comment 14

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v2

Using a sandbox with the principal of the content window and adding a test case.

Comment 15

[Boris Zbarsky [: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.

Comment 16

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v3

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.

Comment 17

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Try run for this patch. FYI.
https://tbpl.mozilla.org/?tree=Try&rev=e7684e943373

Comment 18

[Gene Lian (I already quit Mozilla)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Should this have had sec-approval before landing?

Comment 20

[Daniel Veditz [:dveditz]]

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.

Comment 21

[Marcus Cavanaugh [:mcav]]

This appears to have broken mozAlarms, per bug 1037079.

Comment 22

[Marcus Cavanaugh [:mcav]]

For reference, the affected app in 1037079 invokes mozAlarms.add with the following JSON:

{ type: 'timer' }

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Sean Lin [:seanlin] (inactive from Mar 2016)]

I just made a patch for review in bug 1037079 to fix the introduced issue. Please refer to that bug for more follow-ups.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

31 is affected based on comment #20

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Tracking for esr 31.

Comment 27

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 28

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch ESR31

I just combined the fix for this bug and the one for bug 1037079 into the patch for ESR31 only.

Comment 29

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch ESR31, v2

Minor fix since Alarms API hasn't been migrated to WebIDL yet in ESR31.

Comment 30

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch ESR31, v3

Sorry, the previous version was attached by mistake. Uploading the correct one.

Comment 31

[Sean Lin [:seanlin] (inactive from Mar 2016)]

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 32

[Sean Lin [:seanlin] (inactive from Mar 2016)]

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.

Comment 33

[Bobby Holley (:bholley)]

Why is this needed on esr31? This whole API is b2g-only AFAICT.

Comment 34

[Al Billings [:abillings - ex-MoCo]]

(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

[Bobby Holley (:bholley)]

(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

[Al Billings [:abillings - ex-MoCo]]

Trying to figure out if this is b2g only or if it affects Desktop. Can you give more information?

Comment 37

[Sean Lin [:seanlin] (inactive from Mar 2016)]

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?

Comment 38

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

sec-approval isn't approval to land. You still need to nominate this for esr31 approval.
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 40

[Al Billings [:abillings - ex-MoCo]]

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

[Benjamin Kerensa [:bkerensa]]

Ryan,

I need this landed as I need to GTB.

Comment 42

[Ryan VanderMeulen [: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?

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/c2a095b714f2

Comment 44

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

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