Closed Bug 998083 Opened 10 years ago Closed 10 years ago

isSafeJSObject always returns true

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox29 unaffected, firefox30+ fixed, firefox31+ fixed, firefox-esr24 unaffected, b2g-v1.2 unaffected, b2g-v1.3 unaffected)

RESOLVED FIXED
Firefox 31
Tracking Status
firefox29 --- unaffected
firefox30 + fixed
firefox31 + fixed
firefox-esr24 --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: regression, sec-moderate, Whiteboard: [qa-])

Attachments

(1 file)

I discovered this while trying to remove nsIScriptSecurityManager::GetObjectPrincipal, which never should have been exposed to script.

Bug 952190 did the following:

>  this.isSafeJSObject = function isSafeJSObject(aObj) {
>    if (Cu.getGlobalForObject(aObj) ==
>        Cu.getGlobalForObject(isSafeJSObject)) {
>      return true; // aObj is not a cross-compartment wrapper.
>    }
>  
> +  let principal = Services.scriptSecurityManager.getObjectPrincipal(aObj);
> +  if (Services.scriptSecurityManager.isSystemPrincipal(principal)) {
> +    return true; // allow chrome objects
> +  }
> +
>    return Cu.isXrayWrapper(aObj);
>  };

This will always return true, because this work happens in the scope of the Services JSM, so any objects will be wrapped into that scope, and GetObjectPrincipal doesn't unwrap wrappers.

You really should have flagged me for review on that part. :-(
Flags: needinfo?(mihai.sucan)
Keywords: regression
Nevermind, this is actually my fault (well, gabor's, but I reviewed it). Bug 952192 exposed this to script, but didn't make it handle cross-compartment wrappers, which makes it all boil down to |return mSystemPrincipal;|. Apologies.

I think we should stop trying to use the same machinery from C++ and JS. I'm going to hoist the script-accessible version to Cu, and remove this entirely from SSM.
Assignee: nobody → bobbyholley
Flags: needinfo?(mihai.sucan)
Comment on attachment 8408755 [details] [diff] [review]
Introduce Cu.getObjectPrincipal and kill nsIScriptSecurityManager::GetObjectPrincipal. v1

I meant to flag gabor for review.
Attachment #8408755 - Flags: review?(gkrizsanits)
Comment on attachment 8408755 [details] [diff] [review]
Introduce Cu.getObjectPrincipal and kill nsIScriptSecurityManager::GetObjectPrincipal. v1

Review of attachment 8408755 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for finding/fixing this.
Attachment #8408755 - Flags: review?(gkrizsanits) → review+
Keywords: sec-moderate
Comment on attachment 8408755 [details] [diff] [review]
Introduce Cu.getObjectPrincipal and kill nsIScriptSecurityManager::GetObjectPrincipal. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 952190
User impact if declined: security bugs
Testing completed (on m-c, etc.): Just pushed to inbound
Risk to taking this patch (and alternatives if risky): Low risk. Remove something from nsIScriptSecurityManager, but that function was only introduced in Firefox 30.
String or IDL/UUID changes made by this patch: UUID changes in nsIScriptSecurityMananger and Components.utils. This should be fine to take on aurora.
Attachment #8408755 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/65aa67280bfb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Comment on attachment 8408755 [details] [diff] [review]
Introduce Cu.getObjectPrincipal and kill nsIScriptSecurityManager::GetObjectPrincipal. v1

Yup, UUID changes OK for Aurora - uplift approved.
Attachment #8408755 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thank you Bobby!
(In reply to Mihai Sucan [:msucan] from comment #10)
> Thank you Bobby!

No problem - sorry for not catching this during review!

Do you have an easy way to add test coverage for this to make sure it's doing the right thing now?
(In reply to Bobby Holley (:bholley) from comment #11)
> (In reply to Mihai Sucan [:msucan] from comment #10)
> > Thank you Bobby!
> 
> No problem - sorry for not catching this during review!
> 
> Do you have an easy way to add test coverage for this to make sure it's
> doing the right thing now?

Please check toolkit/devtools/server/tests. You should be able to add a test for isSafeJSObject(). Not sure what is 'easy' - look for prior art in the folders.
(In reply to Mihai Sucan [:msucan] from comment #13)
> Please check toolkit/devtools/server/tests. You should be able to add a test
> for isSafeJSObject(). Not sure what is 'easy' - look for prior art in the
> folders.

Would you mind doing that? I'm pretty overloaded right now, and would rather not dive into all that code.
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Bobby Holley (:bholley) from comment #14)
> (In reply to Mihai Sucan [:msucan] from comment #13)
> > Please check toolkit/devtools/server/tests. You should be able to add a test
> > for isSafeJSObject(). Not sure what is 'easy' - look for prior art in the
> > folders.
> 
> Would you mind doing that? I'm pretty overloaded right now, and would rather
> not dive into all that code.

Can you please file a follow-up bug? Unfortunately, I am currently taking a break from work for personal reasons. I am not sure if I will be able to write the test too soon.
Marking [qa-] for verification purposes, in absence of test case. Feel free to bump one to us if you'd like us to test it, thanks.
Whiteboard: [qa-]
Group: core-security
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.