Chromium Code Reviews

Issue 22962009: Add access check for observed objects (Closed)

Created:
7 years, 4 months ago by adamk
Modified:
7 years, 3 months ago
Reviewers:
rossberg, abarth-chromium
CC:
v8-dev, rafaelw
Visibility:
Public.

Description

Add access check for observed objects This change is mostly straightforward: for 'normal' sorts of change records, simply don't deliver a changeRecord to a given observer callback if an access the callback's Context is not allowed to "GET" or "HAS" changeRecord.name on changeRecord.object, or if ACCESS_KEYS is disallowed. For 'splice' records, the question of whether to hand it to an observer is trickier, since there are multiple properties involved, and multiple types of possible information leakage. Given that access-checked objects are very rare (only two in Blink, Window and Location), and that they are not normally used as Arrays, it seems better to simply not emit any splice records for such objects rather than spending lots of logic to attempt to avoid information leakage for something that may never happen. BUG=v8:2778 R=rossberg@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=16663

Patch Set 1 #

Patch Set 2 : Working with test #

Patch Set 3 : Nearly complete #

Total comments: 14

Patch Set 4 : Add ACCESS_KEYS check, add tests using a blacklist #

Patch Set 5 : Added defineProperty code paths #

Patch Set 6 : Add test for modification via API #

Patch Set 7 : Add ACCESS_HAS checks, a few more tests #

Patch Set 8 : Added some test refactoring #

Patch Set 9 : Merged to trunk #

Unified diffs Side-by-side diffs Stats (+334 lines, -11 lines)
M src/object-observe.js View 3 chunks +21 lines, -5 lines 0 comments
M src/runtime.h View 2 chunks +3 lines, -1 line 0 comments
M src/runtime.cc View 2 chunks +36 lines, -0 lines 0 comments
M test/cctest/test-object-observe.cc View 2 chunks +274 lines, -5 lines 0 comments

Messages

Total messages: 19 (0 generated)
adamk
7 years, 4 months ago (2013-08-14 21:40:22 UTC) #1
rossberg
This looks good so far, but unfortunately, I don't think it's enough to just check ...
7 years, 4 months ago (2013-08-16 13:51:54 UTC) #2
adamk
On 2013/08/16 13:51:54, rossberg wrote: > This looks good so far, but unfortunately, I don't ...
7 years, 4 months ago (2013-08-16 21:48:47 UTC) #3
rossberg
On 2013/08/16 21:48:47, adamk wrote: > On 2013/08/16 13:51:54, rossberg wrote: > > This looks ...
7 years, 4 months ago (2013-08-19 10:19:59 UTC) #4
adamk
On 2013/08/19 10:19:59, rossberg wrote: > On 2013/08/16 21:48:47, adamk wrote: > > On 2013/08/16 ...
7 years, 4 months ago (2013-08-19 17:15:59 UTC) #5
adamk
On 2013/08/19 17:15:59, adamk wrote: > Do you know of any other embedders that use ...
7 years, 4 months ago (2013-08-19 17:38:40 UTC) #6
rossberg
On 2013/08/19 17:15:59, adamk wrote: > On 2013/08/19 10:19:59, rossberg wrote: > > On 2013/08/16 ...
7 years, 4 months ago (2013-08-19 17:51:53 UTC) #7
adamk
On 2013/08/19 17:51:53, rossberg wrote: > On 2013/08/19 17:15:59, adamk wrote: > > On 2013/08/19 ...
7 years, 4 months ago (2013-08-19 18:24:29 UTC) #8
abarth-chromium
On 2013/08/19 17:51:53, rossberg wrote: > There are several browsers and browser-like environments besides Chrome ...
7 years, 4 months ago (2013-08-19 18:58:57 UTC) #9
adamk
https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-observe.cc File test/cctest/test-object-observe.cc (right): https://codereview.chromium.org/22962009/diff/5001/test/cctest/test-object-observe.cc#newcode479 test/cctest/test-object-observe.cc:479: TEST(ObserveNamedAccessCheck) { On 2013/08/16 13:51:54, rossberg wrote: > Would ...
7 years, 4 months ago (2013-08-20 21:38:16 UTC) #10
adamk
Added some more tests, too; still interested in your thoughts on how much coverage is ...
7 years, 4 months ago (2013-08-20 22:09:20 UTC) #11
rossberg
On 2013/08/19 18:24:29, adamk wrote: > So, to go back to your initial response: would ...
7 years, 4 months ago (2013-08-21 09:31:50 UTC) #12
rossberg
On 2013/08/19 18:58:57, abarth wrote: > On 2013/08/19 17:51:53, rossberg wrote: > > There are ...
7 years, 4 months ago (2013-08-21 10:26:41 UTC) #13
rossberg
If you also add checking for ACCESS_HAS then I think the CL is good to ...
7 years, 4 months ago (2013-08-21 10:45:51 UTC) #14
adamk
On 2013/08/21 10:45:51, rossberg wrote: > If you also add checking for ACCESS_HAS then I ...
7 years, 4 months ago (2013-08-21 16:34:27 UTC) #15
adamk
Okay, added everything I think we needed. The tests have gotten a little verbose; let ...
7 years, 4 months ago (2013-08-21 17:18:36 UTC) #16
rossberg
> Okay, added everything I think we needed. The tests have gotten a little > ...
7 years, 4 months ago (2013-08-22 11:14:09 UTC) #17
adamk
On 2013/08/22 11:14:09, rossberg wrote: > > Okay, added everything I think we needed. The ...
7 years, 3 months ago (2013-09-11 19:44:48 UTC) #18
adamk
7 years, 3 months ago (2013-09-11 20:04:00 UTC) #19
Message was sent while issue was closed.
Committed patchset #9 manually as r16663 (presubmit successful).

Powered by Google App Engine