CSP: Don't perform NFC normalization prior to hashing

CSP: Don't perform NFC normalization prior to hashing

Normalization is lossy and not called for in the spec. Don't do it.

BUG=487510, 545678
R=jww@chromium.org

Committed: https://crrev.com/7b52a16c99ecf88701df2992d8e3616490aa997d
Cr-Commit-Position: refs/heads/master@{#357132}

[jsbell, 2015-10-27 21:12:47]

Description was changed from

==========
CSP: Don't perform NFC normalization prior to hashing

Normalization is lossy and not called for in the spec. Don't do it.
Remove now unused normalization paths.

BUG=487510,545678
R=jww
==========

to

==========
CSP: Don't perform NFC normalization prior to hashing

Normalization is lossy and not called for in the spec. Don't do it.
Remove now unused normalization paths.

BUG=487510,545678
R=jww
==========

[jsbell, 2015-10-27 21:12:47]

jsbell@chromium.org changed reviewers:
+ jshin@chromium.org

[jsbell, 2015-10-27 21:29:54]

Description was changed from

==========
CSP: Don't perform NFC normalization prior to hashing

Normalization is lossy and not called for in the spec. Don't do it.
Remove now unused normalization paths.

BUG=487510,545678
R=jww
==========

to

==========
CSP: Don't perform NFC normalization prior to hashing

Normalization is lossy and not called for in the spec. Don't do it.
Remove now unused normalization paths.

BUG=487510,545678
R=jww@chromium.org,jshin@chromium.org
==========

[jsbell, 2015-10-27 23:22:27]

Huzzah, we have a test for this!

http/tests/security/contentSecurityPolicy/1.1/scripthash-unicode-normalization.html

jww@ - your call on whether or not to proceed. It looks like these were
upstreamed to web-platform-tests - is there any feedback from other
implementers?

[jww, 2015-10-28 00:20:57]

On 2015/10/27 23:22:27, jsbell wrote:
> Huzzah, we have a test for this!
> 
>
http/tests/security/contentSecurityPolicy/1.1/scripthash-unicode-normalization.html
> 
> jww@ - your call on whether or not to proceed. It looks like these were
> upstreamed to web-platform-tests - is there any feedback from other
> implementers?

Not that I'm aware of. Taking another look, for script tags at least, we're
supposed to run the algorithm over "the script block's source" which is defined
here: http://www.w3.org/TR/html5/scripting-1.html#the-script-block's-source. It
looks pretty straightforward to me that this doesn't include normalization. Can
you take a look and confirm? If so, we should probably bring it up on the list
to confirm, but then I'd be good making this change.

[jsbell, 2015-10-28 18:03:57]

On 2015/10/28 00:20:57, jww wrote:
> Not that I'm aware of. Taking another look, for script tags at least, we're
> supposed to run the algorithm over "the script block's source" which is
defined
> here: http://www.w3.org/TR/html5/scripting-1.html#the-script-block. It
> looks pretty straightforward to me that this doesn't include normalization.
Can
> you take a look and confirm?

Agreed, I see nothing in the spec that calls for it.

> If so, we should probably bring it up on the list
> to confirm, but then I'd be good making this change.

Since you're Mister CSP at the moment, can you?

You can point at discussion on crbug.com/545383 as to why we're auditing the use
of normalization in Blink and thoughts by i18n folks there.

[jww, 2015-10-28 19:46:49]

On 2015/10/28 18:03:57, jsbell wrote:
> On 2015/10/28 00:20:57, jww wrote:
> > Not that I'm aware of. Taking another look, for script tags at least, we're
> > supposed to run the algorithm over "the script block's source" which is
> defined
> > here: http://www.w3.org/TR/html5/scripting-1.html#the-script-block. It
> > looks pretty straightforward to me that this doesn't include normalization.
> Can
> > you take a look and confirm?
> 
> Agreed, I see nothing in the spec that calls for it.
> 
> > If so, we should probably bring it up on the list
> > to confirm, but then I'd be good making this change.
> 
> Since you're Mister CSP at the moment, can you?
> 
> You can point at discussion on crbug.com/545383 as to why we're auditing the
use
> of normalization in Blink and thoughts by i18n folks there.

Sent. Let's wait for the response before we do anything further, but assuming
it's in the affirmative, I'll remove the test from web-platform-tests and we
should move forward with this CL.

[jsbell, 2015-10-29 18:50:39]

I'll split this CL so it just deals with CSP and doesn't update wtf/text

I'll also invert the sense of our local copy of the test (so that it verifies
normalization is NOT performed)

[jww, 2015-10-29 18:52:27]

Great. I'll postpone my review until then. I'll also separately update the
web-platform-test.
--Joel

On Thu, Oct 29, 2015 at 11:50 AM <jsbell@chromium.org> wrote:

> I'll split this CL so it just deals with CSP and doesn't update wtf/text
>
> I'll also invert the sense of our local copy of the test (so that it
> verifies
> normalization is NOT performed)
>
>
> https://codereview.chromium.org/1420483005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[jww, 2015-10-29 18:52:28]

Great. I'll postpone my review until then. I'll also separately update the
web-platform-test.
--Joel

On Thu, Oct 29, 2015 at 11:50 AM <jsbell@chromium.org> wrote:

> I'll split this CL so it just deals with CSP and doesn't update wtf/text
>
> I'll also invert the sense of our local copy of the test (so that it
> verifies
> normalization is NOT performed)
>
>
> https://codereview.chromium.org/1420483005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jsbell, 2015-10-29 19:42:35]

Okie dokie - test updated, wtf/text changes backed out. I think I matched the
style used in other tests, but feedback welcome.

[jsbell, 2015-10-29 19:42:56]

Description was changed from

==========
CSP: Don't perform NFC normalization prior to hashing

Normalization is lossy and not called for in the spec. Don't do it.
Remove now unused normalization paths.

BUG=487510,545678
R=jww@chromium.org,jshin@chromium.org
==========

to

==========
CSP: Don't perform NFC normalization prior to hashing

Normalization is lossy and not called for in the spec. Don't do it.
Remove now unused normalization paths.

BUG=487510,545678
R=jww@chromium.org
==========

[jsbell, 2015-10-29 19:42:56]

jsbell@chromium.org changed reviewers:
- jshin@chromium.org

[jww, 2015-10-29 21:22:14]

lgtm % my comment about the test. Let me know what you want to do about it.

https://codereview.chromium.org/1420483005/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-unicode-normalization.html
(right):

https://codereview.chromium.org/1420483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-unicode-normalization.html:18:
alert('PASS (1/1)');
Unfortunately, I don't think this tests what we actually want to verify. For the
test to work, it should really be the same exact script except for the one code
point we expect would be normalized in NFC normalization. I jumped through some
hoops to test this in my new version of the web-platform-test:
https://github.com/w3c/web-platform-tests/pull/2287

You might want to consider just copying that or, alternatively, I'm happy to
copy that test over if you just want to delete this test in this CL.

https://codereview.chromium.org/1420483005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/1420483005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:29:
StringUTF8Adaptor utf8Content(content);
Since we're supposed to use the "script block's source" (for script elements;
it's similar for stylesheets, too), I don't think that technically we should be
using utf8 here necessarily
(http://www.w3.org/TR/html5/scripting-1.html#the-script-block's-source). But
that's not your problem, so I'll try address that in a future CL.

[jsbell, 2015-10-29 22:01:46]

D'oh, thanks for catching that. Test reworked - what do you think?

https://codereview.chromium.org/1420483005/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-unicode-normalization.html
(right):

https://codereview.chromium.org/1420483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-unicode-normalization.html:18:
alert('PASS (1/1)');
On 2015/10/29 21:22:14, jww wrote:
> Unfortunately, I don't think this tests what we actually want to verify. For
the
> test to work, it should really be the same exact script except for the one
code
> point we expect would be normalized in NFC normalization.

Oh, duh, thanks. Of course, they started off that way, and then diverged when I
tried to make the output more human-readable.

What do you think about the approach in the latest PS? (Eventually we can just
delete this in favor of the imported one...) Verified that the test fails in the
expected way if the C++ change is backed out.

https://codereview.chromium.org/1420483005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/1420483005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:29:
StringUTF8Adaptor utf8Content(content);
On 2015/10/29 21:22:14, jww wrote:
> Since we're supposed to use the "script block's source" (for script elements;
> it's similar for stylesheets, too), I don't think that technically we should
be
> using utf8 here necessarily
> (http://www.w3.org/TR/html5/scripting-1.html#the-script-block). But
> that's not your problem, so I'll try address that in a future CL.

Acknowledged.

[jww, 2015-10-29 22:06:46]

https://codereview.chromium.org/1420483005/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-unicode-normalization.html
(right):

https://codereview.chromium.org/1420483005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-unicode-normalization.html:18:
alert('PASS (1/1)');
On 2015/10/29 22:01:46, jsbell wrote:
> On 2015/10/29 21:22:14, jww wrote:
> > Unfortunately, I don't think this tests what we actually want to verify. For
> the
> > test to work, it should really be the same exact script except for the one
> code
> > point we expect would be normalized in NFC normalization.
> 
> Oh, duh, thanks. Of course, they started off that way, and then diverged when
I
> tried to make the output more human-readable.
> 
> What do you think about the approach in the latest PS? (Eventually we can just
> delete this in favor of the imported one...) Verified that the test fails in
the
> expected way if the C++ change is backed out.

Looks good!

[jsbell, 2015-10-29 22:08:32]

jsbell@chromium.org changed reviewers:
+ chrishtr@chromium.org

[jsbell, 2015-10-29 22:08:32]

chrishtr@ - can you OWNERS review?

(almost done with these, I swear!)

[jsbell, 2015-10-29 22:08:54]

Description was changed from

==========
CSP: Don't perform NFC normalization prior to hashing

Normalization is lossy and not called for in the spec. Don't do it.
Remove now unused normalization paths.

BUG=487510,545678
R=jww@chromium.org
==========

to

==========
CSP: Don't perform NFC normalization prior to hashing

Normalization is lossy and not called for in the spec. Don't do it.

BUG=487510,545678
R=jww@chromium.org
==========

[chrishtr, 2015-10-30 16:39:57]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-10-30 16:39:58]

lgtm

[chrishtr, 2015-10-30 16:39:58]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org
Link to the patchset: https://codereview.chromium.org/1420483005/#ps60001
(title: "Rework test to ensure tests are identical when normalized")

[commit-bot: I haz the power, 2015-10-30 16:40:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420483005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420483005/60001

[commit-bot: I haz the power, 2015-10-30 18:27:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-30 18:30:29]

Patchset 4 (id:??) landed as
https://crrev.com/7b52a16c99ecf88701df2992d8e3616490aa997d
Cr-Commit-Position: refs/heads/master@{#357132}