Bug 60800 – InjectedScriptSource.js - "Don't be eval()."

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Thomas Sepez]

There are a number of calls to the javascript eval() function inside InjectedScriptSource.js.  This has recently become problematic with the introduction of Content-Security-Policy (CSP) which limits the contexts in which code generated from strings can execute.  When firing up the inspector against a page with CSP enabled, I am seeing an error (chromium example):

ASSERTION FAILED: !hadException
third_party/WebKit/Source/WebCore/inspector/InjectedScript.cpp(110) : WebCore::Node* WebCore::InjectedScript::nodeForObjectId(const WTF::String&)
[1873:1873:3971452761588:ERROR:CONSOLE(119)] "Uncaught Error: Code generation from strings disallowed for this context"

Tracking this down, I see that this operates by cobbling up a string of the form '{"injectedScriptId":1,"id":1}' and passing this into the JS engine as a string via ScriptFunctionCall::call() to the JS function named "nodeToObjectId".  

This is found in WebCore/Inspector/InjectedScriptSource.js, which tries to parse the string by the function _parseObjectId(objectId), in the same file:

    _parseObjectId: function(objectId)
    {
        return eval("(" + objectId + ")");

    },


I was able to circumvent this particular issue by changing the eval to a JSON.parse(objectId).  For the cases where this will work, this is a cleaner approach than the wide-open eval() calls, even though the contents of the string may be completely controlled by the inspector itself.

A layout test running the inspector against CSP-enabled pages would be required here.

[Adam Barth]

@tsepez: Would you be willing to post a patch replacing eval with JSON.parse in a bug blocking this one?  That way we can use this bug as a meta bug to track all these issues.

[Pavel Feldman]

Replacing eval with JSON.parse is a no go since inspector would depend on prototype's JSON.parse for the pages like cnn.com. Instead, we might need to add eval into the InjectedScriptHost binding and grant appropriate permissions to the operation.

[Adam Barth]

Can we give InjectedScriptHost its own JSON object from a fresh v8::Context?  CSP just disables eval on a per-context basis.

[Pavel Feldman]

(In reply to comment #3)
> Can we give InjectedScriptHost its own JSON object from a fresh v8::Context?  CSP just disables eval on a per-context basis.

We can do that if it does not turn into some kind of context nightmare as the one we had with the utility context. There are two use cases for eval usage in injected script:

1) JSON parsing. We can work around it by means of native JSON parsing that we use for the inspector protocol. It is just that we were lazy and used eval

2) window.eval for console expression evaluation. I am not quite sure what you are suggesting to do with this one.

[Adam Barth]

> 1) JSON parsing. We can work around it by means of native JSON parsing that we use for the inspector protocol. It is just that we were lazy and used eval

Ok.  That sounds like it's under control.

> 2) window.eval for console expression evaluation. I am not quite sure what you are suggesting to do with this one.

I've lost track of how this all fits together, but can we add a binding to InjectedScriptHost to call ScriptController::evaluate ?  It's just the JavaScript eval function/operator that's blocked by CSP.

[Pavel Feldman]

> > 2) window.eval for console expression evaluation. I am not quite sure what you are suggesting to do with this one.
> 
> I've lost track of how this all fits together, but can we add a binding to InjectedScriptHost to call ScriptController::evaluate ?  It's just the JavaScript eval function/operator that's blocked by CSP.

Sure, I don't see why not.

[Yury Semikhatsky]

Related Chromium issue: http://code.google.com/p/chromium/issues/detail?id=82212

[Yury Semikhatsky]

(In reply to comment #5)
> > 2) window.eval for console expression evaluation. I am not quite sure what you are suggesting to do with this one.
> 
> I've lost track of how this all fits together, but can we add a binding to InjectedScriptHost to call ScriptController::evaluate ?  It's just the JavaScript eval function/operator that's blocked by CSP.

Providing a binding to InjectedScriptHost to call ScriptController::evaluate wouldn't help here if ScriptController::disableEval has already been called for the context. We need a way to enable evaluations only for expressions provided by inspector.

One possible approach would be for the native part of the inspector to temporarily allow 'eval' using e.g. callback set with V8::SetAllowCodeGenerationFromStringsCallback that allows overriding eval permissions. It would work for synchronous evals but not for things like 'setTimeout("eval('var x = 10;')",0)'.

Another way of dealing with CSP-enabled Pages would be to ask user whether he would like to disable CSP when he tries to evaluate something in console/performs  other inspector actions that require JS evaluation.

[Adam Barth]

> Providing a binding to InjectedScriptHost to call ScriptController::evaluate wouldn't help here if ScriptController::disableEval has already been called for the context. We need a way to enable evaluations only for expressions provided by inspector.

Why is that?  disableEval shouldn't affect ScriptController::evaluate.

[Yury Semikhatsky]

(In reply to comment #9)
> > Providing a binding to InjectedScriptHost to call ScriptController::evaluate wouldn't help here if ScriptController::disableEval has already been called for the context. We need a way to enable evaluations only for expressions provided by inspector.
> 
> Why is that?  disableEval shouldn't affect ScriptController::evaluate.

Oh, I see it uses Script::Compile/Run so it should work fine. But what about asynchronous evaluations when user types something like this in inspector console:
setTimeout("eval('var x = 10;')",0)

It is probably OK to fail in this case with a reasonable error message.

[Adam Barth]

> It is probably OK to fail in this case with a reasonable error message.

Yeah, that will be blocked, just like it would be if run by the page itself.

[Adam Barth]

(In reply to comment #5)
> > 1) JSON parsing. We can work around it by means of native JSON parsing that we use for the inspector protocol. It is just that we were lazy and used eval
> 
> Ok.  That sounds like it's under control.

I've got this almost working.  There doesn't seem to be a way to convert from an InspectorValue back to a native JavaScript value.  Looks like I need to add that.

[Adam Barth]

Created attachment 93826 [details]
work-in-progress

[Adam Barth]

Created attachment 93845 [details]
Patch

[Early Warning System Bot]

Comment on attachment 93845 [details]
Patch

Attachment 93845 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8707356

[WebKit Review Bot]

Comment on attachment 93845 [details]
Patch

Attachment 93845 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8708347

[Adam Barth]

Created attachment 93848 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 93848 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93848&action=review

> Source/WebCore/bindings/v8/V8InspectorValue.cpp:40
> +v8::Handle<v8::Value> toV8(InspectorValue* inspectorValue)

InspectorValue is used for constructing JSON objects in native parts of the inspector. It should not be exposed to the JavaScript code.

> Source/WebCore/inspector/InjectedScriptHost.idl:47
> +        // FIXME: parseJSON actually returns an arbitray value.

Should its return type be DOMObject then?

> Source/WebCore/inspector/InjectedScriptHost.idl:48
> +        [Custom] void parseJSON(in DOMString json);

This solves only part of the problem. We still need to support evaluation in the inspected page for inspector console to work. We need to implement InjectedScriptHost.evaluate instead. This would cover both JSON.parse and console evaluations. Additionally it wouldn't require bindings for InspectorValues.

[Yury Semikhatsky]

I'm working on a change that introduces InjectedScriptHost.evaluate it should work for JSON parsing as well.

[Adam Barth]

(In reply to comment #19)
> I'm working on a change that introduces InjectedScriptHost.evaluate it should work for JSON parsing as well.

That approach will also work.  Thanks for working on this bug.

[Yury Semikhatsky]

Created attachment 93919 [details]
Patch

[WebKit Review Bot]

Comment on attachment 93919 [details]
Patch

Attachment 93919 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8705705

[Yury Semikhatsky]

(In reply to comment #21)
> Created an attachment (id=93919) [details]
> Patch

I don't use ScriptController::evaluate because we need access to exceptions and return values.

The fix works fine for V8 but it makes inspector tests hang in case of JSC. Also in debug mode they crash and it seems that the problem is that  JSMainThreadExecState::evaluate assumes that JS stack is empty when it's called. I'd tried alternative approach with creating EvalExecutable but since evals are disabled by a property on JSGlobalObject this didn't work without adding an option for EvalExecutable to bypass JSGlobalObject::isEvalEnabled() check. Any ideas how to implement this in the JSC binding?

[WebKit Review Bot]

Comment on attachment 93919 [details]
Patch

Attachment 93919 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8705712

New failing tests:
inspector/debugger/event-listener-breakpoints.html

[WebKit Review Bot]

Created attachment 93926 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

> Any ideas how to implement this in the JSC binding?

I'll take a look.

[Adam Barth]

The problem appears to be that JSC::evaluate only works with the global scope, but the inspector appears to need to evaluate things in a non-global scope (at least in these tests).

[Adam Barth]

Created attachment 93943 [details]
Works in JSC

[WebKit Review Bot]

Comment on attachment 93943 [details]
Works in JSC

Attachment 93943 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8705771

New failing tests:
inspector/debugger/event-listener-breakpoints.html

[WebKit Review Bot]

Created attachment 93956 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yury Semikhatsky]

Created attachment 94061 [details]
Patch with Adam's implementation of JSC binding

Adam, thanks for you help with JSC!

[Yury Semikhatsky]

Committed r86837: <http://trac.webkit.org/changeset/86837>