672026 - code evaled from javascript component is not privileged

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Georg Koppen]

While starting an add-on that contains obfuscated JS code that in turn creates an instance of a XPCOM component I get the error: "Error: Permission denied to create instance of class." That does not happen if I include the code without obfuscation.

E.g. if I include: 

var x = Components.classes["@mozilla.org/filepicker;1"].
  createInstance(Components.interfaces.nsIFilePicker);

everything works fine. But if I replace that with the obfuscated code:

eval(function(p,a,c,k,e,d){e=function(c){return c};if(!''.replace(/^/,String)){while(c--)d[c]=k[c]||c;k=[function(e){return d[e]}];e=function(){return'\\w+'};c=1};while(c--)if(k[c])p=p.replace(new RegExp('\\b'+e(c)+'\\b','g'),k[c]);return p}('10 9=0.8["@7.6/5;1"].4(0.3.2);',10,11,'Components||nsIFilePicker|interfaces|createInstance|filepicker|org|mozilla|classes|x|var'.split('|'),0,{}))

I get "Error: Permission denied to create instance of class. CID={bd57cee8-1dd1-11b2-9fe7-95cf4709aea3}" in my error console.

This seems only to happen if I put it in my add-on. If I evaluate both the obfuscated and the not obfuscated code in the error console everything works as expected.

I can reproduce the problem both on Linux and Windows using Firefox 6 (and above). It is working with Firefox 5 and below.

Comment 1

[arno renevier]

Did you try with a nightly ?
Your testcase works correctly for me in latest nightly on linux.

Comment 2

[Georg Koppen]

Mmhhh... Yes I did and got the same results. How did you test the code? Did you put it into an extension? If so, where into a component/module or just chrome?

Comment 3

[Georg Koppen]

Attachment: Contains the code that works flawlessly up to FF 8.0a1

Comment 4

[Georg Koppen]

Attachment: Contains the obfuscated code only working up to (and including FF5)

Comment 5

[Georg Koppen]

Okay. The problem does not occur if I put the code into chrome but is still reproducible (at least for me) if I have it in a component. I attached two minimal extensions showing the issue (hopefully). They are basically the same but one contains the obfuscated code and one the code without obfuscation. Regarding the basic obfuscated extension I get a new (but probably related) error:

Error: Permission denied to create wrapper for object of class XPCComponents_Classes

Comment 6

[arno renevier]

This has nothing to do with all the obsfucation stuff. This happens because of the eval. It looks like this has been caused by bug #656171 (unapplying the patch fixes the problem).

Comment 7

[Georg Koppen]

Ah, okay. That makes sense. Thanks for looking into it!

Comment 8

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

fwiw, I nominated this for tracking because it may break addons.

Comment 9

[Asa Dotzler [:asa]]

Thanks for the additional information, kyle. That's really helpful. We'll evaluate this at Monday's 2PM PT Beta triage session.

Comment 10

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

np, I should have been more verbose the first time

Comment 11

[Luke Wagner [:luke]]

For context: bug 656171 switched PrincipalsForCompiledCode (back) to using the object-principals-finder (instead of cx->compartment->principals, which, until compartment-per-global happens, is wrong in case of document.domain).

In the case of the attached obfuscated addon, this JS is being run very early in startup (nsXREDirProvider::DoStartup calls NS_CreateServicesFromCategory(category="profile-after-change")) and JS_GetSecurityCallbacks(cx) points to nsScriptSecurityManager::Init()::securityCallbacks, which has a NULL findObject principals.  Thus, we get a NULL principals instead of cx->compartment->principals which is the system principals.  I think this craps out in CheckXPCPermissions where null doGetObjectPrincipal(aJSObject) denies access.

The naive "right" fix is to set findObjectPrincipals in nsScriptSecurityManager::Init().  Currently, its set in nsJSRuntime::Init(), but all that ObjectPrincipalsFinder does is call nsScriptSecurityManager::GetObjectPrincipal.  The only rub is getting the right static singleton which seems hackable.

Sound reasonable Blake?

Comment 12

[Luke Wagner [:luke]]

Attachment: a fix

What comment 11 says.  The attached obfuscated addon works.  Pushing to try.

Looking forward to mrbkap killing caps and findObjectPrincipals altogether...

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 546628 [details] [diff] [review]
a fix

Looks good. Only comment is that we already have a gScriptSecMan (and associated nsScriptSecurityManager::GetScriptSecurityManager) that you can use instead of rolling your own sDefaultSecurityManager.

Comment 14

[Luke Wagner [:luke]]

I'll ask for beta approval once try is green.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/1a41c02a2612

Comment 16

[Luke Wagner [:luke]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/82f49f622e9d
http://hg.mozilla.org/releases/mozilla-beta/rev/2c2a8a2007ff

Comment 17

[:Gavin Sharp [email: [e-mail]]]

It looks like this patch didn't actually get approval for the branches. Did you mistake tracking+ for approval+?

Comment 18

[Luke Wagner [:luke]]

Oh dear, yes, that is exactly what happened.  I'm sorry for the mistake.  Can we request now and, if not approved, back out?

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Fennec Aurora is crashing on startup (on Android). This was the only Aurora checkin I see that could have done something. There were only 2 checkins into Aurora.

Comment 20

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

If this broke android why are all the tests green ...?

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #20)
> If this broke android why are all the tests green ...?

Looks like the Android crashes are related to cairo/pixman and don't crash all devices, just many.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

bug 672787. Yesterday's Aurora works, today's crashes.

https://crash-stats.mozilla.com/report/index/bp-502110f9-b010-4911-86f8-edc292110720

Comment 23

[Luke Wagner [:luke]]

Do the corresponding beta/nightly builds also crash?

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #23)
> Do the corresponding beta/nightly builds also crash?

nightly crashes early, for other reasons. no word on beta.

Comment 25

[Luke Wagner [:luke]]

Did the crashes end up being bug 654049?

Comment 26

[Luke Wagner [:luke]]

Oops, I saw that it was backed out b/c of Android, but I missed that it was a "Talos reression".

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #25)
> Did the crashes end up being bug 654049?

And it was never on Aurora

Comment 28

[Josh Matthews [:jdm]]

Backed out of beta: http://hg.mozilla.org/releases/mozilla-beta/rev/eaacac2007f4

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

backed out of aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/9cdc7d59fa11

Comment 30

[Luke Wagner [:luke]]

http://hg.mozilla.org/mozilla-central/rev/7a1c228f8f85

Comment 31

[Luke Wagner [:luke]]

Attachment: alternate, simpler fix

dougt and I were unable to get a local build to repro the problem or even get a tinderbox build under a debugger.  Fortunately, having spent a good bit of time considering the patch, I realized there is a significantly simpler and more attractive fix.

Comment 32

[Luke Wagner [:luke]]

Green on try (whatever that means ;-)

Comment 33

[Luke Wagner [:luke]]

Comment on attachment 547573 [details] [diff] [review]
alternate, simpler fix

Requesting approval for aurora/beta (properly, this time :).  I think think the risk is pretty low risk and the reward is not breaking eval() for JS that run code early in startup.

Comment 34

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

Can we verify that this doesn't break mobile first?

Comment 35

[Luke Wagner [:luke]]

That's reasonable.  I built an opt build with this patch applied to mozilla-aurora tip:
  http://tbpl.mozilla.org/?tree=Try&rev=c9b41a83a0d1
Here's the apk link:
 http://stage.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c9b41a83a0d1/try-android/fennec-7.0a2.en-US.eabi-arm.apk

Perhaps a kind soul with a device that was crashing before test out this build?

Comment 36

[Matt Brubeck (:mbrubeck)]

(In reply to comment #35)
>  http://stage.mozilla.org/pub/mozilla.org/firefox/try-builds/lwagner@mozilla.
> com-c9b41a83a0d1/try-android/fennec-7.0a2.en-US.eabi-arm.apk
> 
> Perhaps a kind soul with a device that was crashing before test out this
> build?

I ran this build without seeing any crashes on two Android devices (Galaxy Tab and T-Mobile G2).

Comment 37

[Luke Wagner [:luke]]

Thanks!

Comment 38

[Luke Wagner [:luke]]

Btw, for reference: it looks like the actual bug that caused the backed-out patch to crash has been tracked down: bug 672787 comment 18.

Comment 39

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/6c423d80fe27

Comment 40

[LegNeato]

Comment on attachment 547573 [details] [diff] [review]
alternate, simpler fix

Approved for releases/mozilla-aurora and releases/mozilla-beta. Please land asap

Comment 41

[Luke Wagner [:luke]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/e9fd2c639c31
http://hg.mozilla.org/releases/mozilla-beta/rev/4a1e595c3a96

Comment 42

[Ioana (away)]

Verified on:
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0

I installed the add-on that contains obfuscated JS code from comment #4. After Firefox restarted, the add-on was enabled. No errors were displayed. Is this enough to verify this bug or do I need to do anything else?

I couldn't verify the fix on Fx8 because the add-on I mentioned above is not compatible with Aurora 8.0a2

Comment 43

[u279076]

qa- as no further verification necessary

Comment 44

[Ioana (away)]

Closing bug per the above comments (comment #42 and #43).