Bug 60800

Summary: InjectedScriptSource.js - "Don't be eval()."
Product: WebKit Reporter: Thomas Sepez <tsepez>
Component: Web Inspector (Deprecated)Assignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, arv, dglazkov, pfeldman, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 53572    
Attachments:
Description Flags
work-in-progress
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Works in JSC
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
Patch with Adam's implementation of JSC binding pfeldman: review+

Description Thomas Sepez 2011-05-13 13:51:11 PDT
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.
Comment 1 Adam Barth 2011-05-13 13:53:13 PDT
@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.
Comment 2 Pavel Feldman 2011-05-13 14:16:55 PDT
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.
Comment 3 Adam Barth 2011-05-13 14:19:21 PDT
Can we give InjectedScriptHost its own JSON object from a fresh v8::Context?  CSP just disables eval on a per-context basis.
Comment 4 Pavel Feldman 2011-05-13 14:28:56 PDT
(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.
Comment 5 Adam Barth 2011-05-13 14:32:47 PDT
> 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.
Comment 6 Pavel Feldman 2011-05-13 14:38:50 PDT
> > 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.
Comment 7 Yury Semikhatsky 2011-05-16 02:09:01 PDT
Related Chromium issue: http://code.google.com/p/chromium/issues/detail?id=82212
Comment 8 Yury Semikhatsky 2011-05-16 02:29:03 PDT
(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.
Comment 9 Adam Barth 2011-05-16 02:31:53 PDT
> 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.
Comment 10 Yury Semikhatsky 2011-05-16 02:41:16 PDT
(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.
Comment 11 Adam Barth 2011-05-16 02:49:13 PDT
> 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.
Comment 12 Adam Barth 2011-05-17 12:33:36 PDT
(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.
Comment 13 Adam Barth 2011-05-17 15:09:09 PDT
Created attachment 93826 [details]
work-in-progress
Comment 14 Adam Barth 2011-05-17 16:29:49 PDT
Created attachment 93845 [details]
Patch
Comment 15 Early Warning System Bot 2011-05-17 16:45:57 PDT
Comment on attachment 93845 [details]
Patch

Attachment 93845 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8707356
Comment 16 WebKit Review Bot 2011-05-17 16:47:14 PDT
Comment on attachment 93845 [details]
Patch

Attachment 93845 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8708347
Comment 17 Adam Barth 2011-05-17 16:54:43 PDT
Created attachment 93848 [details]
Patch
Comment 18 Yury Semikhatsky 2011-05-18 00:02:00 PDT
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.
Comment 19 Yury Semikhatsky 2011-05-18 00:06:19 PDT
I'm working on a change that introduces InjectedScriptHost.evaluate it should work for JSON parsing as well.
Comment 20 Adam Barth 2011-05-18 00:25:28 PDT
(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.
Comment 21 Yury Semikhatsky 2011-05-18 08:33:38 PDT
Created attachment 93919 [details]
Patch
Comment 22 WebKit Review Bot 2011-05-18 08:40:22 PDT
Comment on attachment 93919 [details]
Patch

Attachment 93919 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8705705
Comment 23 Yury Semikhatsky 2011-05-18 08:43:29 PDT
(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?
Comment 24 WebKit Review Bot 2011-05-18 09:16:05 PDT
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
Comment 25 WebKit Review Bot 2011-05-18 09:16:10 PDT
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
Comment 26 Adam Barth 2011-05-18 09:32:04 PDT
> Any ideas how to implement this in the JSC binding?

I'll take a look.
Comment 27 Adam Barth 2011-05-18 10:38:03 PDT
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).
Comment 28 Adam Barth 2011-05-18 11:14:13 PDT
Created attachment 93943 [details]
Works in JSC
Comment 29 WebKit Review Bot 2011-05-18 11:42:23 PDT
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
Comment 30 WebKit Review Bot 2011-05-18 11:42:28 PDT
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
Comment 31 Yury Semikhatsky 2011-05-19 04:41:49 PDT
Created attachment 94061 [details]
Patch with Adam's implementation of JSC binding

Adam, thanks for you help with JSC!
Comment 32 Yury Semikhatsky 2011-05-19 04:47:31 PDT
Committed r86837: <http://trac.webkit.org/changeset/86837>