Closed Bug 957688 Opened 7 years ago Closed 7 years ago

Remove CheckAccess hooks from SpiderMonkey and CAPS

Categories

(Core :: Security: CAPS, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(10 files)

2.27 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.99 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.86 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.26 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
10.99 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
13.19 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.26 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
51.12 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.87 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
15.19 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
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.
With efaust's hack in bug 950407, we can get rid of the second dep. And the first one is coming very soon.
No longer depends on: 832014
Blocks: 960820
Depends on: 950407
\o/  I *never* understood this one.
(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
Assignee: nobody → bobbyholley+bmo
This is green modulo one jit-test that I've fixed. Uploading patches and flagging for review.
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.
But how will we call from Gecko into the JS engine to query CAPS via a callback?
Attachment #8363100 - Flags: review?(mrbkap)
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.
Attachment #8363101 - Flags: review?(mrbkap)
Thankfully, this case was only taking the JSACC_PROTO, which is significantly
simpler than the alternative.
Attachment #8363102 - Flags: review?(mrbkap)
There's no need for the JS shell stuff either, since vm/Runtime.cpp already
sets up NullSecurityCallbacks by default.
Attachment #8363103 - Flags: review?(mrbkap)
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.
Attachment #8363105 - Flags: review?(mrbkap)
Attachment #8363106 - Flags: review?(mrbkap)
Attachment #8363099 - Flags: review?(mrbkap)
Attachment #8363099 - Flags: review?(mrbkap) → review+
Attachment #8363100 - Flags: review?(mrbkap) → review+
Attachment #8363101 - Flags: review?(mrbkap) → review+
Attachment #8363102 - Flags: review?(mrbkap) → review+
Attachment #8363103 - Flags: review?(mrbkap) → review+
Attachment #8363104 - Flags: review?(mrbkap) → review+
Attachment #8363105 - Flags: review?(mrbkap) → review+
Attachment #8363106 - Flags: review?(mrbkap) → review+
Attachment #8363108 - Flags: review?(mrbkap) → review+
Comment on attachment 8363109 [details] [diff] [review]
Part 10 - Remove nsIXPCSecurityManager::CanAccess and nsScriptSecurityManager::CheckPropertyAccessImpl. v1

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

\o/
Attachment #8363109 - Flags: review?(mrbkap) → review+
Thanks for the reviews Blake!

Final all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=08db96c278ab
*applause*
No longer blocks: 961963
You need to log in before you can comment on or make changes to this bug.