CSP script-src should block string arguments to setTimeout and setInterval

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

CSP script-src should block string arguments to setTimeout and setInterval

[Adam Barth]

Created attachment 87220 [details]
Patch

[Darin Adler]

Comment on attachment 87220 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        The changes to V8 and JSC aren't as symmetrical as I would like because
> +        this code path is factored differently in the two bindings.  I've added
> +        a FIXME to synchronize the implementations.

The DOMWindow functions should be usable by the V8 version.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:736
> +    if (exec->hadException() || !action)

So the design here is to silently do nothing if there is a violation?

> Source/WebCore/bindings/js/ScheduledAction.cpp:57
> +        if (activeContext->isDocument() && !static_cast<Document*>(activeContext)->contentSecurityPolicy()->allowScriptFromString())
> +            return 0;

Do we want to have a contentSecurityPolicy for workers too, longer term?

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 87220 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87220&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        The changes to V8 and JSC aren't as symmetrical as I would like because
> > +        this code path is factored differently in the two bindings.  I've added
> > +        a FIXME to synchronize the implementations.
> 
> The DOMWindow functions should be usable by the V8 version.

Yep!

> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:736
> > +    if (exec->hadException() || !action)
> 
> So the design here is to silently do nothing if there is a violation?

Normall these functions return a non-zero value upon success (the ID to use to cancel the timeout).  In this patch, we're returning undefined, which isn't really a silent failure.  Another option is to throw an exception.  I'll update the implementation to match whatever the working group decides.

> > Source/WebCore/bindings/js/ScheduledAction.cpp:57
> > +        if (activeContext->isDocument() && !static_cast<Document*>(activeContext)->contentSecurityPolicy()->allowScriptFromString())
> > +            return 0;
> 
> Do we want to have a contentSecurityPolicy for workers too, longer term?

We could, although the use cases aren't very compelling.  CSP is mostly about blocking XSS and it's pretty hard to XSS yourself in a worker (although it can be done).

If we want to do that, we'll move the ContentSecurityPolicy object onto ScriptExecutionContext from Document.

Thanks for the review!

[Darin Adler]

(In reply to comment #3)
> > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:736
> > > +    if (exec->hadException() || !action)
> > 
> > So the design here is to silently do nothing if there is a violation?
> 
> Normall these functions return a non-zero value upon success (the ID to use to cancel the timeout).  In this patch, we're returning undefined, which isn't really a silent failure.  Another option is to throw an exception.  I'll update the implementation to match whatever the working group decides.

Returning null or 0 are other options.

[Adam Barth]

(In reply to comment #4)
> (In reply to comment #3)
> > > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:736
> > > > +    if (exec->hadException() || !action)
> > > 
> > > So the design here is to silently do nothing if there is a violation?
> > 
> > Normall these functions return a non-zero value upon success (the ID to use to cancel the timeout).  In this patch, we're returning undefined, which isn't really a silent failure.  Another option is to throw an exception.  I'll update the implementation to match whatever the working group decides.
> 
> Returning null or 0 are other options.

Yep.  Do you have a preference among these options?  The behavior is far from set in stone at this point.

[Darin Adler]

(In reply to comment #5)
> Do you have a preference among these options?

I don’t have a preference.

For a special value, null seems a good choice. The undefined value seems like more of a special artifact so probably less good than null. Returning the number 0 seems a language-independent choice, since the function returns an integer in the other cases. An exception also seems OK.

[WebKit Commit Bot]

Comment on attachment 87220 [details]
Patch

Rejecting attachment 87220 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'build-..." exit_code: 2

Last 500 characters of output:
/span ..............
fast/multicol/vertical-lr ..............
fast/multicol/vertical-rl ..............
fast/overflow ...................................................
fast/parser ...............................................................................
fast/parser/iframe-sets-parent-to-javascript-url.html -> crashed

Exiting early after 1 failures. 10321 tests run.
385.43s total testing time

10320 test cases (99%) succeeded
1 test case (<1%) crashed
6 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8285265

[Adam Barth]

I fixed this in another patch.