Closed
Bug 998083
Opened 10 years ago
Closed 10 years ago
isSafeJSObject always returns true
Categories
(DevTools :: General, defect)
DevTools
General
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)
9.50 KB,
patch
|
gkrizsanits
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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. :-(
Assignee | ||
Updated•10 years ago
|
status-firefox28:
--- → unaffected
status-firefox29:
--- → unaffected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
tracking-firefox30:
--- → ?
tracking-firefox31:
--- → ?
Flags: needinfo?(mihai.sucan)
Keywords: regression
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f1a3bebec71f
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: sec-moderate
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/65aa67280bfb
Assignee | ||
Comment 7•10 years ago
|
||
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?
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/65aa67280bfb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Target Milestone: --- → Firefox 31
Comment 9•10 years ago
|
||
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+
Comment 10•10 years ago
|
||
Thank you Bobby!
Assignee | ||
Comment 11•10 years ago
|
||
(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?
Comment 12•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/a85092fe2528
status-b2g-v1.2:
--- → unaffected
status-b2g-v1.3:
--- → unaffected
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
status-firefox-esr24:
--- → unaffected
Comment 13•10 years ago
|
||
(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.
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Updated•10 years ago
|
Updated•10 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 15•10 years ago
|
||
(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.
Comment 16•10 years ago
|
||
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-]
Updated•9 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•