Bug 94331

Summary: JSC should throw a more descriptive exception when blocking 'eval' via CSP.
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore JavaScriptAssignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dbates, ggaren, haraken, japhet, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 93198    
Attachments:
Description Flags
Patch
none
Resetting tests.
none
ggaren's feedback.
none
Rebasing onto ToT.
none
Rebase onto ToT.
none
Rebased.
none
rebasing. none

Description Mike West 2012-08-17 04:17:33 PDT
When 'eval' and 'eval'-like constructs are blocked by a 'script-src' Content Security Policy directive, JSC currently throws an exception reading "Eval is disabled". This isn't bad, but it would be nice if that message included more detail about what exactly had gone wrong. I'd like to see a string more along the lines of <Refused to evaluate a string as JavaScript because it violates the following Content Security Policy directive: "script-src 'unsafe-inline'">.
Comment 1 Mike West 2012-08-17 05:30:45 PDT
Created attachment 159100 [details]
Patch
Comment 2 Mike West 2012-08-17 08:09:53 PDT
Created attachment 159125 [details]
Resetting tests.
Comment 3 Mike West 2012-08-17 08:16:42 PDT
Hey Geoffrey, would you mind taking a look at the patch I've just uploaded? It's a cleaned up version of the one you skimmed yesterday that pipes an error message down from ContentSecurityPolicy to the JSGlobalObject for use in thrown exceptions.

Adam, if you wouldn't mind peeking at the CSP bits as well, I'd appreciate it. :)

Thanks!
Comment 4 Geoffrey Garen 2012-08-17 10:02:39 PDT
Comment on attachment 159125 [details]
Resetting tests.

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

Patch looks generally good. I have some small style comments, and a question about security policy policy.

> Source/WebCore/page/ContentSecurityPolicy.cpp:807
> +        String message = makeString("Refused to evaluate a string as JavaScript because it violates the following Content Security Policy directive: \"", directives->operativeDirective(directives->m_scriptSrc.get())->text(), "\".\n");

The proper tense here is "violated", not "violates", since "Refused" is in the past tense.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1353
> +String ContentSecurityPolicy::evalDisabledErrorMessage() const
> +{
> +    for (size_t i = 0; i < m_policies.size(); ++i) {

This is a linear search each time we make a new document. Is m_policies guaranteed to be very small?

Why did you choose the message from the first non-eval policy? What if there are more non-eval policies?

> Source/WebCore/page/ContentSecurityPolicy.cpp:1354
> +        if (!m_policies[i].get()->allowEval(0, SuppressReport))

No need for .get() on a smart pointer: operator-> does the right thing.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1355
> +            return m_policies[i].get()->evalDisabledErrorMessage();

Ditto.
Comment 5 Mike West 2012-08-17 10:46:05 PDT
Comment on attachment 159125 [details]
Resetting tests.

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

Thanks for your comments! I've addressed them in a patch I'll upload in a moment.

CSP discussion inline below:

>> Source/WebCore/page/ContentSecurityPolicy.cpp:807
>> +        String message = makeString("Refused to evaluate a string as JavaScript because it violates the following Content Security Policy directive: \"", directives->operativeDirective(directives->m_scriptSrc.get())->text(), "\".\n");
> 
> The proper tense here is "violated", not "violates", since "Refused" is in the past tense.

Hrm. Yeah. I see your point, however evaluating a string as JavaScript continues to be a violation. The policy is still in effect. Also, I don't like the way "violated" sounds. :)

I've changed it entirely to avoid this discussion of tense, and replace it with another: "Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive". WDYT?

>> Source/WebCore/page/ContentSecurityPolicy.cpp:1353
>> +    for (size_t i = 0; i < m_policies.size(); ++i) {
> 
> This is a linear search each time we make a new document. Is m_policies guaranteed to be very small?
> 
> Why did you choose the message from the first non-eval policy? What if there are more non-eval policies?

m_policies has one entry per Content Security Policy HTTP header. We expect to only have one, but it's possible for misconfigured sets of servers and proxies to end up with more than one. A site with any more than one or two headers would be an outlier.

I choose the first one simply because it's trivial. :) We deal with multiple headers by ensuring that all of them allow whatever action is being taken. If any of the policies block 'eval()', for instance, we don't have to check the rest of them: it's blocked.

>> Source/WebCore/page/ContentSecurityPolicy.cpp:1354
>> +        if (!m_policies[i].get()->allowEval(0, SuppressReport))
> 
> No need for .get() on a smart pointer: operator-> does the right thing.

Ah. Cool. :)
Comment 6 Mike West 2012-08-17 10:51:04 PDT
Created attachment 159159 [details]
ggaren's feedback.
Comment 7 Mike West 2012-08-21 07:45:44 PDT
Friendly ping, ggaren. :)

Would you mind taking a look at this patch?

Thanks!
Comment 8 Mike West 2012-08-27 12:28:58 PDT
(In reply to comment #7)
> Friendly ping, ggaren. :)
> 
> Would you mind taking a look at this patch?
> 
> Thanks!

Ping. :)
Comment 9 Mike West 2012-09-07 09:25:05 PDT
Hey Geoffrey, it seems like you're pretty booked up these days.

I haven't had much luck tracking down an alternate reviewer the few times I've tried on IRC (probably due to massive time zone differences). Perhaps you could recommend someone else who might be able to review this patch? Thanks!
Comment 10 Mike West 2012-09-07 11:30:18 PDT
Created attachment 162831 [details]
Rebasing onto ToT.
Comment 11 Mike West 2012-09-12 11:24:51 PDT
Created attachment 163662 [details]
Rebase onto ToT.
Comment 12 Mike West 2012-09-12 11:53:56 PDT
Hey Sam, Adam suggested you as a good reviewer for this sort of change. Would you mind taking a look at this patch?

Thanks!
Comment 13 Early Warning System Bot 2012-09-12 12:02:05 PDT
Comment on attachment 163662 [details]
Rebase onto ToT.

Attachment 163662 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13819864
Comment 14 Geoffrey Garen 2012-09-14 11:59:41 PDT
Comment on attachment 163662 [details]
Rebase onto ToT.

r=me
Comment 15 Geoffrey Garen 2012-09-14 12:00:19 PDT
Please fix before landing: 

/usr/bin/gold: /storage/WebKit-qt-wk2-ews/WebKitBuild/Release/Source/WebCore/release/libWebCore.a(SinkDocument.o): in function vtable for WebCore::SinkDocument:SinkDocument.cpp(.data.rel.ro._ZTVN7WebCore12SinkDocumentE+0x188): error: undefined reference to 'WebCore::Document::disableEval()'
/usr/bin/gold: /storage/WebKit-qt-wk2-ews/WebKitBuild/Release/Source/WebCore/release/libWebCore.a(SinkDocument.o): in function vtable for WebCore::SinkDocument:SinkDocument.cpp(.data.rel.ro._ZTVN7WebCore12SinkDocumentE+0x218): error: undefined reference to 'non-virtual thunk to WebCore::Document::disableEval()'
/usr/bin/gold: /storage/WebKit-qt-wk2-ews/WebKitBuild/Release/Source/WebCore/release/libWebCore.a(SharedWorkerContext.o): in function vtable for WebCore::SharedWorkerContext:SharedWorkerContext.cpp(.data.rel.ro._ZTVN7WebCore19SharedWorkerContextE+0x28): error: undefined reference to 'WebCore::WorkerContext::disableEval()'
Comment 16 Mike West 2012-09-14 12:57:48 PDT
Created attachment 164211 [details]
Rebased.
Comment 17 Mike West 2012-09-14 14:08:11 PDT
Comment on attachment 164211 [details]
Rebased.

Thanks, much appreciated!

Latest iteration of the patch looks green. CQ?
Comment 18 WebKit Review Bot 2012-09-14 14:33:38 PDT
Comment on attachment 164211 [details]
Rebased.

Rejecting attachment 164211 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Kit/chromium/third_party/yasm/source/patched-yasm --revision 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
50>At revision 154708.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13859330
Comment 19 Mike West 2012-09-14 14:40:04 PDT
(In reply to comment #18)
> (From update of attachment 164211 [details])
> Rejecting attachment 164211 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> Kit/chromium/third_party/yasm/source/patched-yasm --revision 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> 50>At revision 154708.
> 
> ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> 
> ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
> Updating webkit projects from gyp files...
> 
> Full output: http://queues.webkit.org/results/13859330

Merge conflict during commit: Conflict at '/trunk/LayoutTests/ChangeLog' at /usr/lib/git-core/git-svn line 570

My favourite kind of merge conflict. :(
Comment 20 Mike West 2012-09-14 14:44:51 PDT
Created attachment 164229 [details]
rebasing.
Comment 21 Mike West 2012-09-14 14:45:34 PDT
(In reply to comment #20)
> Created an attachment (id=164229) [details]
> rebasing.

Try again (or have I actually missed something in the ChangeLog)?
Comment 22 Geoffrey Garen 2012-09-14 15:07:18 PDT
Comment on attachment 164229 [details]
rebasing.

r=me
Comment 23 WebKit Review Bot 2012-09-14 16:32:58 PDT
Comment on attachment 164229 [details]
rebasing.

Clearing flags on attachment: 164229

Committed r128670: <http://trac.webkit.org/changeset/128670>
Comment 24 WebKit Review Bot 2012-09-14 16:33:03 PDT
All reviewed patches have been landed.  Closing bug.