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+

Thomas Sepez
Reported 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.
Attachments
work-in-progress (10.71 KB, patch)
2011-05-17 15:09 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (33.14 KB, patch)
2011-05-17 16:29 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (33.21 KB, patch)
2011-05-17 16:54 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (9.96 KB, patch)
2011-05-18 08:33 PDT, Yury Semikhatsky 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ec2-cr-linux-01 (1.26 MB, application/zip)
2011-05-18 09:16 PDT, WebKit Review Bot 		no flags
Details
Works in JSC (15.73 KB, patch)
2011-05-18 11:14 PDT, Adam Barth 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ec2-cr-linux-03 (1.23 MB, application/zip)
2011-05-18 11:42 PDT, WebKit Review Bot 		no flags
Details
Patch with Adam's implementation of JSC binding (15.35 KB, patch)
2011-05-19 04:41 PDT, Yury Semikhatsky 		pfeldman: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 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.
Pavel Feldman
Comment 2 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.
Adam Barth
Comment 3 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.
Pavel Feldman
Comment 4 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.
Adam Barth
Comment 5 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.
Pavel Feldman
Comment 6 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.
Yury Semikhatsky
Comment 7 2011-05-16 02:09:01 PDT
Related Chromium issue: http://code.google.com/p/chromium/issues/detail?id=82212
Yury Semikhatsky
Comment 8 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.
Adam Barth
Comment 9 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.
Yury Semikhatsky
Comment 10 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.
Adam Barth
Comment 11 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.
Adam Barth
Comment 12 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.
Adam Barth
Comment 13 2011-05-17 15:09:09 PDT
Created attachment 93826 [details] work-in-progress
Adam Barth
Comment 14 2011-05-17 16:29:49 PDT
Created attachment 93845 [details] Patch
Early Warning System Bot
Comment 15 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
WebKit Review Bot
Comment 16 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
Adam Barth
Comment 17 2011-05-17 16:54:43 PDT
Created attachment 93848 [details] Patch
Yury Semikhatsky
Comment 18 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.
Yury Semikhatsky
Comment 19 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.
Adam Barth
Comment 20 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.
Yury Semikhatsky
Comment 21 2011-05-18 08:33:38 PDT
Created attachment 93919 [details] Patch
WebKit Review Bot
Comment 22 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
Yury Semikhatsky
Comment 23 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?
WebKit Review Bot
Comment 24 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
WebKit Review Bot
Comment 25 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
Adam Barth
Comment 26 2011-05-18 09:32:04 PDT
> Any ideas how to implement this in the JSC binding? I'll take a look.
Adam Barth
Comment 27 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).
Adam Barth
Comment 28 2011-05-18 11:14:13 PDT
Created attachment 93943 [details] Works in JSC
WebKit Review Bot
Comment 29 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
WebKit Review Bot
Comment 30 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
Yury Semikhatsky
Comment 31 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!
Yury Semikhatsky
Comment 32 2011-05-19 04:47:31 PDT
Committed r86837: <http://trac.webkit.org/changeset/86837>
Note You need to log in before you can comment on or make changes to this bug.