Remove CheckAccess hooks from SpiderMonkey and CAPS

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla29
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments)

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
Assignee

Description

6 years ago
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.
Assignee

Comment 1

6 years ago
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
Assignee

Updated

6 years ago
Blocks: 960820
Assignee

Updated

6 years ago
Depends on: 950407
\o/  I *never* understood this one.
Assignee

Comment 3

6 years ago
(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
Assignee

Comment 5

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

Comment 6

6 years ago
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.
Assignee

Comment 7

6 years ago
But how will we call from Gecko into the JS engine to query CAPS via a callback?
Attachment #8363100 - Flags: review?(mrbkap)
Assignee

Comment 8

6 years ago
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)
Assignee

Comment 9

6 years ago
Thankfully, this case was only taking the JSACC_PROTO, which is significantly
simpler than the alternative.
Attachment #8363102 - Flags: review?(mrbkap)
Assignee

Comment 10

6 years ago
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)
Assignee

Comment 12

6 years ago
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)
Assignee

Comment 13

6 years ago
Attachment #8363106 - Flags: review?(mrbkap)
Assignee

Updated

6 years ago
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+
Assignee

Comment 17

6 years ago
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.