Bug 94332 – V8 should throw a more descriptive exception when blocking 'eval' via CSP.

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Mike West]

When 'eval' and 'eval'-like constructs are blocked by a 'script-src' Content Security Policy directive, V8 currently throws an exception reading "Code generation from strings disallowed for this context", which is fairly miserable. 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]

Hey Ulan! We talked about this change a few weeks back; https://bugs.webkit.org/show_bug.cgi?id=94331 is a patch to change the message in JSC. I'd like to get your help changing the message in V8.

Could you take a look at the JSC patch and help me evaluate how to make it work in V8?

Thanks!

[Ulan Degenbaev]

Created attachment 159662 [details]
Patch

[Ulan Degenbaev]

Hi Mike,

I uploaded a draft patch that depends on your patch and on this V8 CL: https://chromiumcodereview.appspot.com/10837358/

At the moment it passes tests, but there is one call site in WebKit where we need to pass the error message:

WebKit/Source/WebCore/bindings/v8/ScriptState.cpp : setEvalEnabled(ScriptState* scriptState, bool enabled)

It is used from inspector. Do you know how we can pass error message there?

[Mike West]

Thanks Ulan. Let's let the JSC change land first (https://bugs.webkit.org/show_bug.cgi?id=94331); the patch to wire things up to the new V8 patch you've put together should then be quite small.

It looks like fixing the Web Inspector bit should be pretty straightforward as well. I'll just add some code to the bindings to expose the ErrorMessageForCodeGenerationFromStrings method that you've added, and then we should be able to save/rewrite that rather than just toggling the boolean.

Thank you!

[Mike West]

Hey Ulan! The JSC patch just landed last night. *phew* :)

I'll rebase this patch on top of the current trunk. Would you mind taking another look at landing https://codereview.chromium.org/10837358/ in the meantime? I think we'll need that in place before we can land the WebKit side.

[Mike West]

Created attachment 164279 [details]
Patch

[Mike West]

(In reply to comment #6)
> Created an attachment (id=164279) [details]
> Patch

I'll rebaseline the tests once the V8 portion of this change lands and is rolled into WebKit.

It doesn't yet take care of ScriptState.cpp, as I think we'll need to chat about what the V8 API needs to look like. I'll poke you on Monday, Uan. :) Thanks!

[Eric Seidel (no email)]

Comment on attachment 164279 [details]
Patch

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

Dr. Barth is your man.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:373
> +            context->AllowCodeGenerationFromStrings(m_frame->document()->contentSecurityPolicy()->allowEval(0, ContentSecurityPolicy::SuppressReport), v8String(m_frame->document()->contentSecurityPolicy()->evalDisabledErrorMessage()));

A ContentSecurityPolicy local would make this line much shorter. :)

> Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:89
> +    , m_disableEvalPending(String())

I can never remember if we have a nullString() which wer'e supposed to use instead these days.

[Adam Barth]

Comment on attachment 164279 [details]
Patch

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

>> Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:89
>> +    , m_disableEvalPending(String())
> 
> I can never remember if we have a nullString() which wer'e supposed to use instead these days.

You can actually skip this line.  The compiler calls the default constructor for you.  nullAtom is only needed when you're returning something by reference.

[Adam Barth]

Comment on attachment 164279 [details]
Patch

Patch looks fine.

[Mike West]

Created attachment 164357 [details]
New method.

[Mike West]

Hey Ulan, here's an updated version of the patch to match the API changes you've made. It doesn't compile without those changes, so... Let's hope it compiles on your machine. :)

[Ulan Degenbaev]

(In reply to comment #12)
> Hey Ulan, here's an updated version of the patch to match the API changes you've made. It doesn't compile without those changes, so... Let's hope it compiles on your machine. :)

Yep, it compiles and produces the following outputs for 
eval-blocked-in-about-blank-iframe.html, eval-blocked.html, function-constructor-blocked.html:


CONSOLE MESSAGE: line 1: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'".

CONSOLE MESSAGE: line 12: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'".

CONSOLE MESSAGE: line 15: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'".

ONSOLE MESSAGE: line 12: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'".

[Mike West]

(In reply to comment #13)
> Yep, it compiles and produces the following outputs for 
> eval-blocked-in-about-blank-iframe.html, eval-blocked.html, function-constructor-blocked.html:

Looks good. Would you mind landing the V8 side of the patch? Once it gets rolled into the Chromium port, I'll see about landing this side.

Thanks, Ulan!

[Ulan Degenbaev]

I landed the V8 CL as http://code.google.com/p/v8/source/detail?r=12525 to V8 bleeding_edge. It looks like we won't roll V8 until Chrome branches (V8 trunk is frozen).

[Mike West]

(In reply to comment #15)
> I landed the V8 CL as http://code.google.com/p/v8/source/detail?r=12525 to V8 bleeding_edge. It looks like we won't roll V8 until Chrome branches (V8 trunk is frozen).

Makes sense. So, what timeframe are we looking at? Beginning of next weekish?

[Ulan Degenbaev]

> So, what timeframe are we looking at? Beginning of next weekish?
Yes. Probably, next week Wed.

[Mike West]

Created attachment 164926 [details]
Rebaselining.

[Mike West]

(In reply to comment #18)
> Created an attachment (id=164926) [details]
> Rebaselining.

I've rolled DEPS locally and rebaselined the tests. I think we should be able to land this directly after the V8 change rolls into WebKit ToT next week.

[Mike West]

Created attachment 165266 [details]
Patch

[Mike West]

Comment on attachment 165266 [details]
Patch

Dropping the r?, throwing to the bots. If the bots are happy, I'll carry Adam's r+ over and ask for CQ?. :)

[Ulan Degenbaev]

V8 roll that this patch depends on is going to be reverted because of high crash rate :(

[Mike West]

(In reply to comment #22)
> V8 roll that this patch depends on is going to be reverted because of high crash rate :(

Ah, too bad!

Good that I waited until Monday. :)

[Mike West]

(In reply to comment #23)
> (In reply to comment #22)
> > V8 roll that this patch depends on is going to be reverted because of high crash rate :(

Looks like the next try at a roll was reverted too. :(

[Mike West]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > V8 roll that this patch depends on is going to be reverted because of high crash rate :(
> 
> Looks like the next try at a roll was reverted too. :(

And the next. I'm putting this on ice until the V8 roll happens and sticks. Probably next week? Bleh. :/

[Mike West]

Created attachment 168159 [details]
Rebasing onto ToT

[Mike West]

Created attachment 169126 [details]
Rebased again, for the last time (maybe).

[Mike West]

V8 has rolled in the fix I was waiting for, and a fix to that fix. Things work locally; throwing this into the queue. :)

[WebKit Review Bot]

Comment on attachment 169126 [details]
Rebased again, for the last time (maybe).

Clearing flags on attachment: 169126

Committed r131573: <http://trac.webkit.org/changeset/131573>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.