Enforce ScriptForbiddenScope and make it non-fatal.

Enforce ScriptForbiddenScope for user scripts.

ScriptForbiddenScope used to trigger a fatal assert if Blink tried to
execute script in a forbidden scope. This was problematic because
plugins often tried to run scripts inside these scopes.

To prevent these crashes, AllowSuperUnsafeScripts was added as an opt
out to allow plugin-triggered scripts to execute script anyway in these
dangerous contexts. This lead to bugs like https://crbug.com/367210,
where something that should be "impossible" wasn't.

In order to prevent random code from having to work around edge cases
that only happen when script shouldn't even be running, the entry points
for executing user script have been updated to first check if scripting
is forbidden. If it is, they just return undefined or an empty object as
appropriate.

A followup patch will attempt to apply a similar approach for handling
internal scripts.

BUG=363099, 371084
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183650

[dcheng, 2014-10-13 22:40:06]

dcheng@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org

[dcheng, 2014-10-13 22:56:35]

dcheng@chromium.org changed reviewers:
+ eseidel@chromium.org

[dcheng, 2014-10-13 22:56:35]

This is inspired from the discussion at
https://groups.google.com/a/chromium.org/forum/#!topic/site-isolation-dev/GFI...
and the linked bugs. Originally, what I wanted to do was add a
ScriptForbiddenScope while Frame::detach() is running to suppress script from
running there. While investigating this, I ran into (at least) two problems:
1) If ScriptForbiddenScope is being enforced, it's actually a fatal error, since
there's a RELEASE_ASSERT in V8RecursionScope.
2) Even if it was being enforced, we temporarily allow "super unsafe scripting"
in ~WebPluginContainerImpl (https://codereview.chromium.org/403133002), which is
the guilty stack for some of the stranger frame detach crashes.

I think (but I'm not sure) it might be hard to fix some of the issues from the
plugins side, so I wanted to make it possible to actually enforce this policy
and remove AllowSuperUnsafeScript. Before I spend too much time pursuing this...
I'm curious if this non-fatal approach has already been tried. Finally--is there
a performance concern from making V8RecursionScope heap-allocated? If so, it's
always possible to push the checks up a level, but it'd be nice to avoid
redundant code if possible.

[esprehn, 2014-10-13 23:53:29]

This definitely can't be heap allocated.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dcheng, 2014-10-14 01:02:36]

I've made this stack allocated once more, and added checks to
ScriptForbiddenScope::isScriptForbidden() before trying to create a
V8RecursionScope.

[haraken, 2014-10-14 01:04:30]

> Finally--is there a performance concern from making V8RecursionScope
heap-allocated?

I don't have much concern about this. The overhead of executing script would be
much larger than the overhead of allocating V8RecursionScope.

https://codereview.chromium.org/648423003/diff/130001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8RecursionScope.h (right):

https://codereview.chromium.org/648423003/diff/130001/Source/bindings/core/v8...
Source/bindings/core/v8/V8RecursionScope.h:63: static
PassOwnPtr<V8RecursionScope> create(v8::Isolate* isolate)

Shall we rename create => createIfScriptAllowed ?

https://codereview.chromium.org/648423003/diff/130001/Source/bindings/core/v8...
Source/bindings/core/v8/V8RecursionScope.h:88: class MicrotaskSuppression {

You need to take care of MicrotaskSuppression as well.

[haraken, 2014-10-14 01:06:39]

On 2014/10/14 01:04:30, haraken wrote:
> > Finally--is there a performance concern from making V8RecursionScope
> heap-allocated?
> 
> I don't have much concern about this. The overhead of executing script would
be
> much larger than the overhead of allocating V8RecursionScope.

That being said, I prefer keeping V8RecursionScope stack-allocated for
readability. It looks strange to allocate a Scope object on-heap.

[haraken, 2014-10-14 01:09:18]

The patch set 4 looks good except that you need to take care of
MicrotaskSuppression.

https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8RecursionScope.h (right):

https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
Source/bindings/core/v8/V8RecursionScope.h:97:
ASSERT(!ScriptForbiddenScope::isScriptForbidden());

Can we try to make this RELEASE_ASSERT in a follow-up CL?

[dcheng, 2014-10-14 02:46:28]

On 2014/10/14 at 01:09:18, haraken wrote:
> The patch set 4 looks good except that you need to take care of
MicrotaskSuppression.

Hmm. I took a look at MicrotaskSuppression... and it seems like it'd be a lot
trickier.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
shows two problem cases:
- It's initialized as a member of ConstructorMode, so now we need to push up
ScriptForbiddenScope checks there as well. This is kind of ugly =(
- More problematic, it's exposed through WebScopedMicrotaskSuppression. Maybe we
can assume that those places are implicitly safe to allow scripting though...

> 
>
https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
> File Source/bindings/core/v8/V8RecursionScope.h (right):
> 
>
https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
> Source/bindings/core/v8/V8RecursionScope.h:97:
ASSERT(!ScriptForbiddenScope::isScriptForbidden());
> 
> Can we try to make this RELEASE_ASSERT in a follow-up CL?

Before I try to do that though, I have some stupid questions because I'm not
that familiar with microtasks.

1) If I don't handle MicrotaskSuppression, will it actually cause crashes? I
looked into the bugs linked to the AllowSuperUnsafeScript CL, and it seemed like
all of them would have been caught by checking before V8RecursionScope (since
they all go through ScriptController::executeScriptAndReturnValue). So maybe the
problematic stacks from plugins are only going through
executeScriptAndReturnValue anyway?

2) What is the purpose of microtask suppression? I searched around a bit but I'm
honestly not sure why it's important (from
https://code.google.com/p/chromium/issues/detail?id=369503, it seems this can
cause some internal fields to not be initialized?)

[haraken, 2014-10-14 05:06:00]

On 2014/10/14 02:46:28, dcheng wrote:
> On 2014/10/14 at 01:09:18, haraken wrote:
> > The patch set 4 looks good except that you need to take care of
> MicrotaskSuppression.
> 
> Hmm. I took a look at MicrotaskSuppression... and it seems like it'd be a lot
> trickier.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> shows two problem cases:
> - It's initialized as a member of ConstructorMode, so now we need to push up
> ScriptForbiddenScope checks there as well. This is kind of ugly =(
> - More problematic, it's exposed through WebScopedMicrotaskSuppression. Maybe
we
> can assume that those places are implicitly safe to allow scripting though...
> 
> > 
> >
>
https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
> > File Source/bindings/core/v8/V8RecursionScope.h (right):
> > 
> >
>
https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
> > Source/bindings/core/v8/V8RecursionScope.h:97:
> ASSERT(!ScriptForbiddenScope::isScriptForbidden());
> > 
> > Can we try to make this RELEASE_ASSERT in a follow-up CL?
> 
> Before I try to do that though, I have some stupid questions because I'm not
> that familiar with microtasks.
> 
> 1) If I don't handle MicrotaskSuppression, will it actually cause crashes? I
> looked into the bugs linked to the AllowSuperUnsafeScript CL, and it seemed
like
> all of them would have been caught by checking before V8RecursionScope (since
> they all go through ScriptController::executeScriptAndReturnValue). So maybe
the
> problematic stacks from plugins are only going through
> executeScriptAndReturnValue anyway?
> 
> 2) What is the purpose of microtask suppression? I searched around a bit but
I'm
> honestly not sure why it's important (from
> https://code.google.com/p/chromium/issues/detail?id=369503, it seems this can
> cause some internal fields to not be initialized?)

V8RecursionScope is a scope for executing user scripts, and MicroTaskSuppression
is a scope for executing internal scripts. The difference is whether we call
V8RecursionScope::didLeaveScriptContext() or not.

Even if you don't handle MicroTaskSuppression, you're not making anything worse
than today.

If you can handle MicroTaskSuppression (i.e., add the RELEASE_ASSERT to
MicroTaskSuppression), you're making the situation better. Given that
isScriptForbidden() means that *any* script should not be executed, it makes
more sense to forbid execution of both user scripts and internal scripts. I
think you can try this in a follow-up :)

[dcheng, 2014-10-14 05:47:30]

OK, thank you for the explanation. I've updated the patch description to better
reflect the intent of this patch and document some of the rationale behind
making this change. I will try to do the microtask change in a separate CL.

https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8RecursionScope.h (right):

https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
Source/bindings/core/v8/V8RecursionScope.h:97:
ASSERT(!ScriptForbiddenScope::isScriptForbidden());
On 2014/10/14 at 01:09:17, haraken wrote:
> Can we try to make this RELEASE_ASSERT in a follow-up CL?

I think it makes sense to try this, but I would like to separate it into a
separate patch, since I don't know how tricky it will be to land this patch.

[haraken, 2014-10-14 05:55:30]

LGTM

It makes sense to address MicroTaskSuppression in a follow-up.

https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:180: return v8::Undefined(isolate);

v8::Undefined(isolate) => return v8::Local<v8::Value>()

https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:227: return v8::Undefined(isolate);

v8::Undefined(isolate) => return v8::Local<v8::Value>()

[dcheng, 2014-10-14 06:42:09]

https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:180: return v8::Undefined(isolate);
On 2014/10/14 05:55:29, haraken wrote:
> 
> v8::Undefined(isolate) => return v8::Local<v8::Value>()

Done.

https://codereview.chromium.org/648423003/diff/40002/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:227: return v8::Undefined(isolate);
On 2014/10/14 05:55:29, haraken wrote:
> 
> v8::Undefined(isolate) => return v8::Local<v8::Value>()

Done.

[dcheng, 2014-10-14 06:42:13]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-10-14 06:42:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/648423003/230001

[commit-bot: I haz the power, 2014-10-14 07:51:16]

Committed patchset #5 (id:230001) as 183650