Last Comment Bug 672026 - code evaled from javascript component is not privileged
: code evaled from javascript component is not privileged
Status: VERIFIED FIXED
[regression from bug 656171 ?] [khuey...
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 6 Branch
: x86 All
: -- normal (vote)
: mozilla8
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks: 656171
  Show dependency treegraph
 
Reported: 2011-07-16 04:24 PDT by Georg Koppen
Modified: 2011-09-26 02:33 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed


Attachments
Contains the code that works flawlessly up to FF 8.0a1 (1.41 KB, application/octet-stream)
2011-07-16 10:10 PDT, Georg Koppen
no flags Details
Contains the obfuscated code only working up to (and including FF5) (1.58 KB, application/octet-stream)
2011-07-16 10:11 PDT, Georg Koppen
no flags Details
a fix (4.42 KB, patch)
2011-07-18 13:59 PDT, Luke Wagner [:luke]
mrbkap: review+
christian: approval‑mozilla‑aurora-
christian: approval‑mozilla‑beta-
Details | Diff | Splinter Review
alternate, simpler fix (647 bytes, patch)
2011-07-21 17:31 PDT, Luke Wagner [:luke]
mrbkap: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Georg Koppen 2011-07-16 04:24:29 PDT
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 2011-07-16 08:43:45 PDT
Did you try with a nightly ?
Your testcase works correctly for me in latest nightly on linux.
Comment 2 Georg Koppen 2011-07-16 08:56:48 PDT
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 2011-07-16 10:10:05 PDT
Created attachment 546335 [details]
Contains the code that works flawlessly up to FF 8.0a1
Comment 4 Georg Koppen 2011-07-16 10:11:08 PDT
Created attachment 546336 [details]
Contains the obfuscated code only working up to (and including FF5)
Comment 5 Georg Koppen 2011-07-16 10:13:58 PDT
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 2011-07-16 12:49:27 PDT
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 2011-07-16 12:59:20 PDT
Ah, okay. That makes sense. Thanks for looking into it!
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-17 21:54:35 PDT
fwiw, I nominated this for tracking because it may break addons.
Comment 9 Asa Dotzler [:asa] 2011-07-17 21:56:36 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-17 21:57:31 PDT
np, I should have been more verbose the first time
Comment 11 Luke Wagner [:luke] 2011-07-18 12:15:26 PDT
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] 2011-07-18 13:59:34 PDT
Created attachment 546628 [details] [diff] [review]
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) 2011-07-18 14:11:31 PDT
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] 2011-07-18 14:29:39 PDT
I'll ask for beta approval once try is green.
Comment 15 Marco Bonardo [::mak] 2011-07-19 08:15:32 PDT
http://hg.mozilla.org/mozilla-central/rev/1a41c02a2612
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-19 17:59:36 PDT
It looks like this patch didn't actually get approval for the branches. Did you mistake tracking+ for approval+?
Comment 18 Luke Wagner [:luke] 2011-07-19 18:13:44 PDT
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?) 2011-07-20 06:39:07 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-20 07:45:54 PDT
If this broke android why are all the tests green ...?
Comment 21 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-20 08:00:51 PDT
(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?) 2011-07-20 08:13:41 PDT
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] 2011-07-20 09:55:45 PDT
Do the corresponding beta/nightly builds also crash?
Comment 24 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-20 12:00:30 PDT
(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] 2011-07-21 09:23:26 PDT
Did the crashes end up being bug 654049?
Comment 26 Luke Wagner [:luke] 2011-07-21 09:25:43 PDT
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?) 2011-07-21 12:04:41 PDT
(In reply to comment #25)
> Did the crashes end up being bug 654049?

And it was never on Aurora
Comment 28 Josh Matthews [:jdm] 2011-07-21 12:09:55 PDT
Backed out of beta: http://hg.mozilla.org/releases/mozilla-beta/rev/eaacac2007f4
Comment 29 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-21 13:17:25 PDT
backed out of aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/9cdc7d59fa11
Comment 30 Luke Wagner [:luke] 2011-07-21 16:43:10 PDT
http://hg.mozilla.org/mozilla-central/rev/7a1c228f8f85
Comment 31 Luke Wagner [:luke] 2011-07-21 17:31:40 PDT
Created attachment 547573 [details] [diff] [review]
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] 2011-07-22 08:52:57 PDT
Green on try (whatever that means ;-)
Comment 33 Luke Wagner [:luke] 2011-07-22 09:59:19 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-22 10:00:14 PDT
Can we verify that this doesn't break mobile first?
Comment 35 Luke Wagner [:luke] 2011-07-22 11:48:42 PDT
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/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?
Comment 36 Matt Brubeck (:mbrubeck) 2011-07-22 12:32:53 PDT
(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] 2011-07-22 15:01:11 PDT
Thanks!
Comment 38 Luke Wagner [:luke] 2011-07-22 15:07:07 PDT
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] 2011-07-25 06:08:13 PDT
http://hg.mozilla.org/mozilla-central/rev/6c423d80fe27
Comment 40 christian 2011-07-25 14:50:42 PDT
Comment on attachment 547573 [details] [diff] [review]
alternate, simpler fix

Approved for releases/mozilla-aurora and releases/mozilla-beta. Please land asap
Comment 42 Ioana (away) 2011-08-29 07:10:17 PDT
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 16:09:51 PDT
qa- as no further verification necessary
Comment 44 Ioana (away) 2011-09-26 02:33:02 PDT
Closing bug per the above comments (comment #42 and #43).

Note You need to log in before you can comment on or make changes to this bug.