code evaled from javascript component is not privileged

VERIFIED FIXED in Firefox 6

Status

()

Core
JavaScript Engine
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Georg Koppen, Assigned: luke)

Tracking

({regression})

6 Branch
mozilla8
x86
All
regression
Points:
---

Firefox Tracking Flags

(firefox6+ fixed, firefox7+ fixed, firefox8+ fixed)

Details

(Whiteboard: [regression from bug 656171 ?] [khuey nom'd for add-on compat][qa-])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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

6 years ago
Did you try with a nightly ?
Your testcase works correctly for me in latest nightly on linux.
(Reporter)

Comment 2

6 years ago
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?
(Reporter)

Comment 3

6 years ago
Created attachment 546335 [details]
Contains the code that works flawlessly up to FF 8.0a1
(Reporter)

Comment 4

6 years ago
Created attachment 546336 [details]
Contains the obfuscated code only working up to (and including FF5)
(Reporter)

Comment 5

6 years ago
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

6 years ago
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).
Assignee: nobody → general
Component: XPConnect → JavaScript Engine
Keywords: regression
QA Contact: xpconnect → general
Summary: Can't create instances of XPCOM components using obfuscated JS code → code evaled from javascript component is not privilegied.

Updated

6 years ago
Blocks: 656171
(Reporter)

Comment 7

6 years ago
Ah, okay. That makes sense. Thanks for looking into it!
tracking-firefox6: --- → ?
tracking-firefox7: --- → ?
tracking-firefox8: --- → ?
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

6 years ago
Whiteboard: [regression from bug 656171 ?] [khuey's shotgun nom]
fwiw, I nominated this for tracking because it may break addons.

Comment 9

6 years ago
Thanks for the additional information, kyle. That's really helpful. We'll evaluate this at Monday's 2PM PT Beta triage session.
Whiteboard: [regression from bug 656171 ?] [khuey's shotgun nom] → [regression from bug 656171 ?] [khuey nom'd for add-on compat]
np, I should have been more verbose the first time
(Assignee)

Comment 11

6 years ago
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?
(Assignee)

Comment 12

6 years ago
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...
Attachment #546628 - Flags: review?(mrbkap)
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.
Attachment #546628 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 14

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

Updated

6 years ago
tracking-firefox6: ? → +
tracking-firefox7: ? → +
tracking-firefox8: ? → ---
http://hg.mozilla.org/mozilla-central/rev/1a41c02a2612
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/82f49f622e9d
http://hg.mozilla.org/releases/mozilla-beta/rev/2c2a8a2007ff
It looks like this patch didn't actually get approval for the branches. Did you mistake tracking+ for approval+?
Summary: code evaled from javascript component is not privilegied. → code evaled from javascript component is not privileged
(Assignee)

Comment 18

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

Updated

6 years ago
Attachment #546628 - Flags: approval-mozilla-beta?
Attachment #546628 - Flags: approval-mozilla-aurora?
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.
If this broke android why are all the tests green ...?
(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.
bug 672787. Yesterday's Aurora works, today's crashes.

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

Comment 23

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

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

Comment 25

6 years ago
Did the crashes end up being bug 654049?
(Assignee)

Comment 26

6 years ago
Oops, I saw that it was backed out b/c of Android, but I missed that it was a "Talos reression".
Blocks: 672787
No longer blocks: 672787
(In reply to comment #25)
> Did the crashes end up being bug 654049?

And it was never on Aurora
Backed out of beta: http://hg.mozilla.org/releases/mozilla-beta/rev/eaacac2007f4
backed out of aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/9cdc7d59fa11

Updated

6 years ago
Attachment #546628 - Flags: approval-mozilla-beta?
Attachment #546628 - Flags: approval-mozilla-beta-
Attachment #546628 - Flags: approval-mozilla-aurora?
Attachment #546628 - Flags: approval-mozilla-aurora-

Updated

6 years ago
tracking-firefox8: --- → +
(Assignee)

Comment 30

6 years ago
http://hg.mozilla.org/mozilla-central/rev/7a1c228f8f85
(Assignee)

Comment 31

6 years ago
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.
Assignee: general → luke
Attachment #546628 - Attachment is obsolete: true
Status: RESOLVED → REOPENED
Attachment #547573 - Flags: review?(mrbkap)
Resolution: FIXED → ---
(Assignee)

Comment 32

6 years ago
Green on try (whatever that means ;-)

Updated

6 years ago
Attachment #547573 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 33

6 years ago
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.
Attachment #547573 - Flags: approval-mozilla-beta?
Attachment #547573 - Flags: approval-mozilla-aurora?
Can we verify that this doesn't break mobile first?
(Assignee)

Comment 35

6 years ago
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?
(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).
(Assignee)

Comment 37

6 years ago
Thanks!
(Assignee)

Comment 38

6 years ago
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.
http://hg.mozilla.org/mozilla-central/rev/6c423d80fe27
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 40

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

Approved for releases/mozilla-aurora and releases/mozilla-beta. Please land asap
Attachment #547573 - Flags: approval-mozilla-beta?
Attachment #547573 - Flags: approval-mozilla-beta+
Attachment #547573 - Flags: approval-mozilla-aurora?
Attachment #547573 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 41

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

Updated

6 years ago
status-firefox6: --- → fixed
status-firefox7: --- → fixed

Updated

6 years ago
status-firefox8: --- → fixed

Comment 42

6 years ago
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
qa- as no further verification necessary
Whiteboard: [regression from bug 656171 ?] [khuey nom'd for add-on compat] → [regression from bug 656171 ?] [khuey nom'd for add-on compat][qa-]

Comment 44

6 years ago
Closing bug per the above comments (comment #42 and #43).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.