search

svnlabs / google-caja

Automatically exported from code.google.com/p/google-caja
0 stars 1 forks source link

SES needs to repair when v8 ignores writable flag on arrays #1931

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
<script>
  var a = [ 0, 1, 2 ];
  Object.defineProperty( a, 0, {
    writable : false
  } );
  a.shift();
  console.log(a);
</script>​​​​​​​​​​​​​​​​​​​

On Chrome, I see the following in the console:

SES initialization ses-single-frame.js?debug=1:969
2
 Problem ignored by configuration (Unsafe spec violation): Extending an array can modify read-only array length ses-single-frame.js?debug=1:948
 Repaired: Non-deletable RegExp statics are a global communication channel ses-single-frame.js?debug=1:948
 Repaired: Date.prototype is a global communication channel ses-single-frame.js?debug=1:948
 Repaired: Setting a function's prototype with defineProperty doesn't change its value ses-single-frame.js?debug=1:948
 Not repaired: Extending an array can modify read-only array length ses-single-frame.js?debug=1:948
 Not repaired: [[ThrowTypeError]] has normal function properties ses-single-frame.js?debug=1:948
 Max Severity: Safe spec violation(1). ses-single-frame.js?debug=1:948
 440 Apparently fine ses-single-frame.js?debug=1:948
 50 Deleted ses-single-frame.js?debug=1:948
 3 Frozen harmless ses-single-frame.js?debug=1:948
 1 Skipped ses-single-frame.js?debug=1:948
 Max Severity: Safe spec violation(1). ses-single-frame.js?debug=1:948
 initSES succeeded. ses-single-frame.js?debug=1:948

[1, 2]

Calling shift() should have thrown.  Seems related to issue 1816, but the 
resolution to that one I think only tested for frozenness.

Original issue reported on code.google.com by metaw...@gmail.com on 14 Aug 2014 at 10:22

GoogleCodeExporter commented 9 years ago 
Note "Problem ignored by configuration (Unsafe spec violation):". I think this 
is a known and explicitly permitted problem. Please try loading SES without 
Caja configuration ( src/com/google/caja/ses/explicit.html ) and see if it 
fails when run in the console there.

We deliberately ignore array mutability bugs beyond complete frozenness, to 
gain performance, given that the Caja sandbox only relies on immutability when 
the objects are fully frozen.

Original comment by kpreid@google.com on 14 Aug 2014 at 10:41

GoogleCodeExporter commented 9 years ago 
At the debugger statement in fake1.js (I think that's in an SES environment) 
the test case still violates the invariant.

Original comment by metaw...@gmail.com on 14 Aug 2014 at 10:51

GoogleCodeExporter commented 9 years ago 
What's fake1.js?

Original comment by kpreid@google.com on 14 Aug 2014 at 10:54

GoogleCodeExporter commented 9 years ago 
I don't know; it's part of the tests run by explicit.html.  Just have your 
console open when you refresh the page and it'll break there.

If I open the console after explicit.html is done and run the code, it still 
shifts 0 off the array.

Original comment by metaw...@gmail.com on 14 Aug 2014 at 10:57

GoogleCodeExporter commented 9 years ago 
Ah, I was looking at the wrong version. fake1.js is a filename made up by 
explicit.html. This does seem to be a bug which is not repaired and not 
reported as a problem.

MarkM, can you take this? I don't have the time currently.

Original comment by kpreid@google.com on 14 Aug 2014 at 11:10

GoogleCodeExporter commented 9 years ago 
Yup. This is indeed a bug. I'll take it.

Original comment by erights@google.com on 14 Aug 2014 at 11:16

GoogleCodeExporter commented 9 years ago 
Michael, Andreas (cc'ed),

This is caused by https://code.google.com/p/v8/issues/detail?id=3356 . Since 
that bug is publicly visible, I ask here instead: What is the status of v8 
issue 3356?

(Please don't discuss publicly at this time. Thanks.)

Original comment by erights on 14 Aug 2014 at 11:24

GoogleCodeExporter commented 9 years ago 
On Canary at least, unshift, pop, and reverse suffer from this problem as well, 
but splice and sort do not seem to.

Michael, Andreas, 

Are splice and sort really safe, or did we just miss the problematic case?

Would it be safe for us to repair shift, unshift, and pop by redefining them in 
terms of splice?

Does anything else suffer from this problem besides these four?

All,

Once this is public, we can now ask the community to contribute test262 tests 
for these. (We can already ask for all the public SES tests that don't yet have 
test262 tests, and should do so.)

Original comment by erights on 15 Aug 2014 at 12:31

GoogleCodeExporter commented 9 years ago 
It looks like I was mistaken about reverse. AFAICT, it does not suffer from 
this bug. AFAICT, that leaves only shift, unshift, and pop. If true, this is 
nice, because all of these can be redefined in terms of splice.

Original comment by erights on 15 Aug 2014 at 1:59

GoogleCodeExporter commented 9 years ago 
Nevermind. I'm making this public and unmarking it as a security issue, as all 
the problems go away if the property is made non-writable, non-configurable. 
Only the latter is security significant.

I am not closing this yet, since we should still have tests and repairs for the 
significant case. I have started on these.

Original comment by erights on 15 Aug 2014 at 1:20

GoogleCodeExporter commented 9 years ago 
Sigh, and damn. I was looking at the wrong thing. Even for non-writable, 
non-configurable, we do have this problem for shift and unshift only. In any 
case repairs coming soon.

Original comment by erights on 15 Aug 2014 at 2:43

GoogleCodeExporter commented 9 years ago 
Remarking as Security / High. It is of course too late to remark as Private :(

Original comment by erights on 15 Aug 2014 at 2:44

GoogleCodeExporter commented 9 years ago 
The only real issues are shift and unshift. All other flags I raise above were 
due to my confusion.

See https://code.google.com/p/v8/issues/detail?id=3356#c3 comment 3.

Original comment by erights on 18 Aug 2014 at 2:13

GoogleCodeExporter commented 9 years ago 
v8 properly prevents popping in the following, but issues the wrong error:

a=[7,8,9];
Object.defineProperty(a,2,{configurable:false});
a.pop();
// throws TypeError: Cannot define property:2, object is not extensible.
// instead of TypeError: Cannot delete property '2' of [object Array]

Original comment by metaw...@gmail.com on 18 Aug 2014 at 4:42

GoogleCodeExporter commented 9 years ago 
@r5692

Changed title of this issue to reflect what has been fixed here. The underlying 
v8 problem remains, but is a v8 issue.

Original comment by erights on 19 Aug 2014 at 4:16