Bug 94331 – JSC should throw a more descriptive exception when blocking 'eval' via CSP.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Mike West]

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'">.

[Mike West]

Created attachment 159100 [details]
Patch

[Mike West]

Created attachment 159125 [details]
Resetting tests.

[Mike West]

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!

[Geoffrey Garen]

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.

[Mike West]

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. :)

[Mike West]

Created attachment 159159 [details]
ggaren's feedback.

[Mike West]

Friendly ping, ggaren. :)

Would you mind taking a look at this patch?

Thanks!

[Mike West]

(In reply to comment #7)
> Friendly ping, ggaren. :)
> 
> Would you mind taking a look at this patch?
> 
> Thanks!

Ping. :)

[Mike West]

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!

[Mike West]

Created attachment 162831 [details]
Rebasing onto ToT.

[Mike West]

Created attachment 163662 [details]
Rebase onto ToT.

[Mike West]

Hey Sam, Adam suggested you as a good reviewer for this sort of change. Would you mind taking a look at this patch?

Thanks!

[Early Warning System Bot]

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

[Geoffrey Garen]

Comment on attachment 163662 [details]
Rebase onto ToT.

r=me

[Geoffrey Garen]

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()'

[Mike West]

Created attachment 164211 [details]
Rebased.

[Mike West]

Comment on attachment 164211 [details]
Rebased.

Thanks, much appreciated!

Latest iteration of the patch looks green. CQ?

[WebKit Review Bot]

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

[Mike West]

(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. :(

[Mike West]

Created attachment 164229 [details]
rebasing.

[Mike West]

(In reply to comment #20)
> Created an attachment (id=164229) [details]
> rebasing.

Try again (or have I actually missed something in the ChangeLog)?

[Geoffrey Garen]

Comment on attachment 164229 [details]
rebasing.

r=me

[WebKit Review Bot]

Comment on attachment 164229 [details]
rebasing.

Clearing flags on attachment: 164229

Committed r128670: <http://trac.webkit.org/changeset/128670>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.