Bug 33616 - Inspector interface allows insecure data transfer
Summary: Inspector interface allows insecure data transfer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-13 12:22 PST by Geoffrey Garen
Modified: 2010-02-04 01:40 PST (History)
9 users (show)

See Also:


Attachments
patch (32.46 KB, patch)
2010-02-03 05:17 PST, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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