Bug 33616

Summary: Inspector interface allows insecure data transfer
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: WebCore JavaScriptAssignee: Yury Semikhatsky <yurys>
Status: RESOLVED FIXED    
Severity: Normal CC: bweinstein, joepeck, keishi, pfeldman, pmuellr, rik, timothy, ukai, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch pfeldman: review+

Description Geoffrey Garen 2010-01-13 12:22:12 PST
From https://bugs.webkit.org/show_bug.cgi?id=33469:

> 2. I don't think interfaces like this are safe:
> 
> +    ScriptObject injectedScriptFor(ScriptState*);
> +    ScriptObject injectedScriptForId(long);
> 
> Because you're giving out a generic object, the client of the interface can do
> anything with the object, including passing it unsafe data. It would be better
> if the C++ object wrapping the "injected script" controlled all interaction
> with it, to ensure that no non-primitive data leaked across the boundary.
Eventually we will create a typed interface for that object. Currently it is
used only as ScriptFunctionCall argument but we should restrict the set of
functions that can be called on that object, it would make the injected script
interface clearer. We've already done similar thing in InspectorFrontend.cpp
Comment 1 Yury Semikhatsky 2010-02-03 05:17:47 PST
Created attachment 48019 [details]
patch
Comment 2 Pavel Feldman 2010-02-03 05:55:20 PST
Comment on attachment 48019 [details]
patch

> +    ScriptFunctionCall function(scriptState, m_injectedScriptObject, "getCallFrames");

"getCallFrames" -> "callFrames" ?

> +    ScriptValue callFramesValue = function.call();
> +    return callFramesValue.toString(scriptState);
> +}
> +
> +String InjectedScript::wrapAndStringify(ScriptValue value)
> +{
> +    ASSERT(!hasNoValue());
> +    ScriptState* scriptState = m_injectedScriptObject.scriptState();
> +    ScriptFunctionCall wrapFunction(scriptState, m_injectedScriptObject, "wrapAndStringifyObject");
> +    wrapFunction.appendArgument(value);
> +    wrapFunction.appendArgument("console");

Should this be reflected in the method name? wrapAndStringifyForConsole?
Comment 3 Yury Semikhatsky 2010-02-03 10:12:06 PST
(In reply to comment #2)
> (From update of attachment 48019 [details])
> > +    ScriptFunctionCall function(scriptState, m_injectedScriptObject, "getCallFrames");
> 
> "getCallFrames" -> "callFrames" ?
> 
Done.

> > +    ScriptValue callFramesValue = function.call();
> > +    return callFramesValue.toString(scriptState);
> > +}
> > +
> > +String InjectedScript::wrapAndStringify(ScriptValue value)
> > +{
> > +    ASSERT(!hasNoValue());
> > +    ScriptState* scriptState = m_injectedScriptObject.scriptState();
> > +    ScriptFunctionCall wrapFunction(scriptState, m_injectedScriptObject, "wrapAndStringifyObject");
> > +    wrapFunction.appendArgument(value);
> > +    wrapFunction.appendArgument("console");
> 
> Should this be reflected in the method name? wrapAndStringifyForConsole?
Done.


Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.pro
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/bindings/js/JSInjectedScriptHostCustom.cpp
	M	WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp
	A	WebCore/inspector/InjectedScript.cpp
	A	WebCore/inspector/InjectedScript.h
	M	WebCore/inspector/InjectedScriptHost.cpp
	M	WebCore/inspector/InjectedScriptHost.h
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/front-end/InjectedScript.js
Committed r54285
Comment 4 Yury Semikhatsky 2010-02-04 00:12:35 PST
Reverted because it broke compilation on Linux GTK.
Comment 5 Yury Semikhatsky 2010-02-04 01:39:45 PST
(In reply to comment #4)
> Reverted because it broke compilation on Linux GTK.

Committed r54334 with the new files added to WebCore/GNUmakefile.am