957688 - Remove CheckAccess hooks from SpiderMonkey and CAPS

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

This stuff is mostly garbage these days. The two main deps of this bug I've found are:

(1) nsISecurityCheckedComponent
(2) Our current implementation of [Unforgeable] for Location.

Marking these deps appropriately.

Comment 1

[Bobby Holley (:bholley)]

With efaust's hack in bug 950407, we can get rid of the second dep. And the first one is coming very soon.

Comment 2

[Luke Wagner [:luke]]

\o/  I *never* understood this one.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #2)
> \o/  I *never* understood this one.

Me neither.

https://tbpl.mozilla.org/?tree=Try&rev=31bbfd1c86a0

Comment 4

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=f83068dbc016

Comment 5

[Bobby Holley (:bholley)]

This is green modulo one jit-test that I've fixed. Uploading patches and flagging for review.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Make ComputeStackString pass a principal and remove checkAccess call. v1

Now that we have the principal-based filtering for stack walking, we can do this.
This isn't technically equivalent to the old behavior, since a stack that goes:

A -> B -> A

would previous have only seen the second set of |A| frames, whereas now we'd
see both sets. But this seems strictly better (also, it doesn't happen on the
web).

As noted, I've filed a bug for making this context- and saveFrameChain-agnostic.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove JS_CheckAccess. v1

But how will we call from Gecko into the JS engine to query CAPS via a callback?

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove side-effect-free calls to js::CheckAccess. v1

js::CheckAccess has all sorts of crazy side-effects on its parameters. Luckily,
they mostly happen on dead values.

We have to alter a jit-test that previously threw, and doesn't anymore. I have
confirmed that the reason for throwing was not the security check itself, but
rather the lookupGeneric call that happens inside js::CheckAccess, which ends
up throwing 'undefined is not a function'. It seems like this is just an issue
of calling lookupGeneric when we shouldn't, and that the correct behavior here
is not to throw.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove side-effect-y call to js::CheckAccess, and remove js::CheckAccess itself. v1

Thankfully, this case was only taking the JSACC_PROTO, which is significantly
simpler than the alternative.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 5 - Remove checkObjectAccess security callback. v1

There's no need for the JS shell stuff either, since vm/Runtime.cpp already
sets up NullSecurityCallbacks by default.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove checkAccess nsIXPCScriptable hook. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 7 - Remove checkaccess junk from the XBL compilation scope. v1

I have no idea what this is supposed to be doing, given that the compilation
scope doesn't run script. We should make sure this is reviewed by someone
who remembers why this was written.

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 8 - Kill off checkAccess. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 9 - Remove nsIScriptSecurityManager::CheckPropertyAccess. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 10 - Remove nsIXPCSecurityManager::CanAccess and nsScriptSecurityManager::CheckPropertyAccessImpl. v1

\o/

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8363109 [details] [diff] [review]
Part 10 - Remove nsIXPCSecurityManager::CanAccess and nsScriptSecurityManager::CheckPropertyAccessImpl. v1

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

\o/

Comment 17

[Bobby Holley (:bholley)]

Thanks for the reviews Blake!

Final all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=08db96c278ab

Comment 18

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f6fb2082c996
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/08b7f4b5b665
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/048746b43ad3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3c42f192f4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1a85688d37ea
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed1607eac3a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4030b1beb0d4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac97770b63c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5db4e6d1dc02
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cc32cb2daf27

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f6fb2082c996
https://hg.mozilla.org/mozilla-central/rev/08b7f4b5b665
https://hg.mozilla.org/mozilla-central/rev/048746b43ad3
https://hg.mozilla.org/mozilla-central/rev/4e3c42f192f4
https://hg.mozilla.org/mozilla-central/rev/1a85688d37ea
https://hg.mozilla.org/mozilla-central/rev/7ed1607eac3a
https://hg.mozilla.org/mozilla-central/rev/4030b1beb0d4
https://hg.mozilla.org/mozilla-central/rev/5ac97770b63c
https://hg.mozilla.org/mozilla-central/rev/5db4e6d1dc02
https://hg.mozilla.org/mozilla-central/rev/cc32cb2daf27

Comment 20

[Jason Orendorff [:jorendorff]]

*applause*