Closed Bug 877478 Opened 8 years ago Closed 8 years ago

Remove JSContext-specific security managers

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox24 --- fixed
firefox-esr17 - wontfix
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- unaffected

People

(Reporter: bholley, Assigned: bholley)

References

(Depends on 1 open bug)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main24-])

Attachments

(10 files, 5 obsolete files)

3.68 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
20.19 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.24 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.94 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
18.19 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
18.51 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
6.98 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
10.04 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.19 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
9.85 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
These don't do anything anymore AFAICT, and the whole GetAppropriateSecurityManager dance doesn't play well with XPCCallContext removal. Let's just make everything use the default security manager (which itself doesn't do all that much).
This stuff shouldn't be necessary anymore. The default security manager should
do the right thing given for script running in the scope of a BackstagePass.
Attachment #755706 - Flags: review?(mrbkap)
The only consumers here were the shells, which we've now fixed.
Attachment #755707 - Flags: review?(mrbkap)
Comment on attachment 755706 [details] [diff] [review]
Part 1 - Remove FullTrustSecMan junk. v1

Hm, looks like this is orange. I'll look into it tomorrow.
Attachment #755706 - Flags: review?(mrbkap)
Attachment #755707 - Flags: review?(mrbkap)
Attachment #755708 - Flags: review?(mrbkap)
Attachment #755709 - Flags: review?(mrbkap)
Sigh, the orange is actually indicative of a security bug that our test suite depends on. :-(
Group: core-security
So, the basic issue here is that GetAppropriateSecurityManager actually returns null if !CallerTypeIsJavaScript(), which depends on whether NATIVE_CALLER or JS_CALLER was set on the ccx. But that flag close to meaningless, and there are lots of places where we pass NATIVE_CALLER that can be triggered by JS.

In this case, the big issue is the call to WrapNativeToJSVal in PrepareForUnwrapping creates a ccx with NATIVE_CALLER, effectively meaning that CanCreateInstance checks always succeed. So this means that, if content can get a reference to some chrome-only non-PreCreate (wrapper-per-scope) XPCOM object, wrapping it into the content compartment will succeed, creating a brand new instance of a forbidden object.

This means, for example, that if chrome passes Components.classes to a content window, that content window can use it. The same holds for any other XPCOM object that isn't supposed to be created for content and has one reflector per scope.

Marking as sec-moderate for now, since we don't yet know of an exploit vector. But moz_bug_r_a4 might be able to make this high or crit.
Keywords: sec-moderate
Depends on: 878325
Spent another day fighting the test suite, let's see if this one's green:
https://tbpl.mozilla.org/?tree=Try&rev=fb4b9a00a6cc
This is green modulo two failures from the patches in bug 878325. I don't think I actually depend on those patches at this point, so I'm just going to remove them from the queue for now.
These crashtests now throw, because we run XUL tests in content, and they create
forbidden objects and fail the CanCreateWrapper check. Unfortunately, the
crashtest harness isn't set up to catch onerror. And even if they were, they
wouldn't be testing what they wanted to, because all of the relevant work here
happens in onload.
Attachment #757622 - Flags: review?(mrbkap)
Attachment #755706 - Attachment is obsolete: true
Attachment #755707 - Attachment is obsolete: true
Attachment #755708 - Attachment is obsolete: true
Attachment #755709 - Attachment is obsolete: true
Attachment #757624 - Flags: review?(mrbkap)
Attachment #757625 - Attachment description: Part 2 - Fix mochitests with problematic callbacks to use the new API. v1 → Part 3 - Fix mochitests with problematic callbacks to use the new API. v1
|subject| is quite often an XPCOM object that's not accessible to content.
Attachment #757626 - Flags: review?(mrbkap)
This way, we get the callback wrapping behavior for free.
Attachment #757627 - Flags: review?(mrbkap)
PConnect generally throws when trying to create instances of non-DOM objects
in content. Due to some bugs this has historically worked in certain cases, but
we're fixing those now. So we need to fix the tests that do this sort of thing.
Attachment #757628 - Flags: review?(mrbkap)
This stuff shouldn't be necessary anymore. The default security manager should
do the right thing given for script running in the scope of a BackstagePass.
Attachment #757629 - Flags: review?(mrbkap)
The only consumers here were the shells, which we've now fixed.
Attachment #757631 - Flags: review?(mrbkap)
Fixed a small bug in the crashtest move.
Attachment #757622 - Attachment is obsolete: true
Attachment #757622 - Flags: review?(mrbkap)
Attachment #758061 - Flags: review?(mrbkap)
Attachment #758061 - Flags: review?(mrbkap) → review+
Comment on attachment 757624 [details] [diff] [review]
Part 2 - Add a SpecialPowers API for wrapping callbacks and callback objects. v1

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

::: testing/specialpowers/content/specialpowersAPI.js
@@ +477,5 @@
> +   * function strips any wrappers if they exist and compare the underlying
> +   * values.
> +   */
> +  compare: function(a, b) {
> +    return unwrapIfWrapped(a) == unwrapIfWrapped(b);

===?
Attachment #757624 - Flags: review?(mrbkap) → review+
Attachment #757625 - Flags: review?(mrbkap) → review+
Attachment #757626 - Flags: review?(mrbkap) → review+
Attachment #757627 - Flags: review?(mrbkap) → review+
Attachment #757628 - Flags: review?(mrbkap) → review+
Attachment #757629 - Flags: review?(mrbkap) → review+
Attachment #757631 - Flags: review?(mrbkap) → review+
Attachment #757634 - Flags: review?(mrbkap) → review+
Attachment #757635 - Flags: review?(mrbkap) → review+
Depends on: 880078
I'm going to assume ESR is unaffected by this issue. Is this correct?
(In reply to Al Billings [:abillings] from comment #29)
> I'm going to assume ESR is unaffected by this issue. Is this correct?

No, everything is affected, more or less.
Whiteboard: [adv-main24+]
Affected, but since it's sec-moderate this falls outside of the scope for ESR landing criteria.  Not tracking.
Whiteboard: [adv-main24+] → [adv-main24-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.