Bug 57278 - CSP script-src should block string arguments to setTimeout and setInterval
Summary: CSP script-src should block string arguments to setTimeout and setInterval
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 53572
  Show dependency treegraph
 
Reported: 2011-03-28 15:29 PDT by Adam Barth
Modified: 2011-04-26 09:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.55 KB, patch)
2011-03-28 15:31 PDT, Adam Barth
darin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-03-28 15:29:08 PDT
CSP script-src should block string arguments to setTimeout and setInterval
Comment 1 Adam Barth 2011-03-28 15:31:44 PDT
Created attachment 87220 [details]
Patch
Comment 2 Darin Adler 2011-03-28 16:06:31 PDT
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?
Comment 3 Adam Barth 2011-03-28 16:12:43 PDT
(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!
Comment 4 Darin Adler 2011-03-28 18:14:15 PDT
(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.
Comment 5 Adam Barth 2011-03-28 18:18:18 PDT
(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.
Comment 6 Darin Adler 2011-03-28 20:08:42 PDT
(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.
Comment 7 WebKit Commit Bot 2011-03-29 05:55:45 PDT
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
Comment 8 Adam Barth 2011-04-26 09:55:22 PDT
I fixed this in another patch.