Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(72)

Issue 26481005: Implementation of script hashes for CSP. (Closed)

Created:
7 years, 3 months ago by jww
Modified:
7 years, 2 months ago
CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, adamk+blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Implementation of script hashes for CSP. BUG=277905 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160866

Patch Set 1 #

Total comments: 20

Patch Set 2 : Fixes and improvements. #

Patch Set 3 : Added tests #

Total comments: 18

Patch Set 4 : Fixed abarth's comments, added tests, and added short circuit for unused hashes. #

Total comments: 16

Patch Set 5 : Updated with nits from mkwst. #

Patch Set 6 : More fixes from mkwst comments #

Total comments: 14

Patch Set 7 : Changes from abarth, including splitting hash into struct #

Total comments: 6

Patch Set 8 : Fixed nits from abarth #

Patch Set 9 : Rebase on lkgr #

Patch Set 10 : Rebase on tip of tree #

Unified diffs Side-by-side diffs Delta from patch set Stats (+272 lines, -18 lines) Patch
A + LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-allowed.html View 1 2 1 chunk +4 lines, -7 lines 0 comments Download
A LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-allowed-expected.txt View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-basic-blocked.html View 1 2 1 chunk +25 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-basic-blocked-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-unicode-normalization.html View 1 2 3 1 chunk +31 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scripthash-unicode-normalization-expected.txt View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scriptnonce-and-scripthash.html View 1 2 1 chunk +30 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/contentSecurityPolicy/1.1/scriptnonce-and-scripthash-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +8 lines, -0 lines 0 comments Download
M Source/core/dom/ScriptLoader.cpp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M Source/core/frame/ContentSecurityPolicy.h View 1 2 3 4 5 6 7 8 4 chunks +16 lines, -1 line 0 comments Download
M Source/core/frame/ContentSecurityPolicy.cpp View 1 2 3 4 5 6 7 8 9 20 chunks +136 lines, -9 lines 0 comments Download
M Source/wtf/text/Base64.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M Source/wtf/text/Base64.cpp View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 23 (0 generated)
jww
This is my initial implementation of script hashes. It still needs some work (tests, SHA256 ...
7 years, 3 months ago (2013-10-16 01:55:09 UTC) #1
Mike West
This is a great start, Joel! Thanks for putting it together... I look forward to ...
7 years, 3 months ago (2013-10-16 14:53:02 UTC) #2
jww
On 2013/10/16 14:53:02, Mike West wrote: > This is a great start, Joel! Thanks for ...
7 years, 3 months ago (2013-10-16 21:47:18 UTC) #3
jww
https://codereview.chromium.org/26481005/diff/1/Source/core/dom/ScriptLoader.cpp File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/1/Source/core/dom/ScriptLoader.cpp#newcode311 Source/core/dom/ScriptLoader.cpp:311: bool shouldBypassMainWorldContentSecurityPolicy = (frame && frame->script()->shouldBypassMainWorldContentSecurityPolicy()) || elementDocument->contentSecurityPolicy()->allowScriptNonce(m_element->fastGetAttribute(HTMLNames::nonceAttr)) || ...
7 years, 3 months ago (2013-10-16 21:49:20 UTC) #4
jww
Okay, I've made the changes I mentioned in my reply to Mike, I've added some ...
7 years, 3 months ago (2013-10-17 02:08:40 UTC) #5
abarth-chromium
https://codereview.chromium.org/26481005/diff/11001/Source/core/dom/ScriptLoader.cpp File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/11001/Source/core/dom/ScriptLoader.cpp#newcode313 Source/core/dom/ScriptLoader.cpp:313: bool allowScriptHash = elementDocument->contentSecurityPolicy()->allowHash(sourceCode.source()); We shouldn't call either of ...
7 years, 3 months ago (2013-10-17 02:28:08 UTC) #6
jww
I've made the fixes abarth asked for. Also, could someone address my previous comment about ...
7 years, 3 months ago (2013-10-18 22:58:04 UTC) #7
abarth-chromium
On 2013/10/18 22:58:04, jww wrote: > I've made the fixes abarth asked for. Also, could ...
7 years, 3 months ago (2013-10-18 23:05:51 UTC) #8
Mike West
This looks great. I just have a few small comments, but otherwise LGTM. That said, ...
7 years, 3 months ago (2013-10-21 07:11:55 UTC) #9
jww
https://codereview.chromium.org/26481005/diff/18001/Source/core/dom/ScriptLoader.cpp File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/18001/Source/core/dom/ScriptLoader.cpp#newcode315 Source/core/dom/ScriptLoader.cpp:315: bool shouldBypassMainWorldContentSecurityPolicy = frameContentSecurityPolicyBypass || allowScriptNonce || allowScriptHash; On ...
7 years, 3 months ago (2013-10-21 19:18:03 UTC) #10
Mike West
https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/ContentSecurityPolicy.cpp File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/ContentSecurityPolicy.cpp#newcode313 Source/core/frame/ContentSecurityPolicy.cpp:313: HashSet<String> m_hashes; On 2013/10/21 19:18:04, jww wrote: > On ...
7 years, 3 months ago (2013-10-21 19:28:47 UTC) #11
jww
On 2013/10/21 19:28:47, Mike West wrote: > https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/ContentSecurityPolicy.cpp > File Source/core/frame/ContentSecurityPolicy.cpp (right): > > https://codereview.chromium.org/26481005/diff/18001/Source/core/frame/ContentSecurityPolicy.cpp#newcode313 ...
7 years, 3 months ago (2013-10-21 20:22:43 UTC) #12
abarth-chromium
https://codereview.chromium.org/26481005/diff/158001/Source/core/dom/ScriptLoader.cpp File Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/26481005/diff/158001/Source/core/dom/ScriptLoader.cpp#newcode315 Source/core/dom/ScriptLoader.cpp:315: bool shouldBypassMainWorldContentSecurityPolicy = frameContentSecurityPolicyBypass || allowScriptNonce || allowScriptHash; This ...
7 years, 3 months ago (2013-10-22 17:46:49 UTC) #13
jww
I think I've addressed all of abarth's comments and issues. The biggest change in this ...
7 years, 3 months ago (2013-10-28 19:36:22 UTC) #14
abarth-chromium
It doesn't look like you've uploaded a new patchset to this CL.
7 years, 2 months ago (2013-10-29 05:19:09 UTC) #15
jww
Indeed, my mistake. New CL should be uploaded now.
7 years, 2 months ago (2013-10-29 05:45:40 UTC) #16
abarth-chromium
LGTM. Thanks for iterating on this CL. Below are just some aesthetic nits. https://codereview.chromium.org/26481005/diff/238001/Source/core/frame/ContentSecurityPolicy.cpp File ...
7 years, 2 months ago (2013-10-29 05:56:26 UTC) #17
jww
https://codereview.chromium.org/26481005/diff/238001/Source/core/frame/ContentSecurityPolicy.cpp File Source/core/frame/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/26481005/diff/238001/Source/core/frame/ContentSecurityPolicy.cpp#newcode443 Source/core/frame/ContentSecurityPolicy.cpp:443: ContentSecurityPolicy::HashFunctions hashFunction = ContentSecurityPolicy::HashFunctionsNone; On 2013/10/29 05:56:26, abarth wrote: ...
7 years, 2 months ago (2013-10-29 17:47:04 UTC) #18
jww
7 years, 2 months ago (2013-10-29 18:33:38 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/26481005/328001
7 years, 2 months ago (2013-10-29 18:35:39 UTC) #20
commit-bot: I haz the power
Failed to apply patch for Source/core/frame/ContentSecurityPolicy.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 2 months ago (2013-10-29 18:36:10 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/26481005/368001
7 years, 2 months ago (2013-10-29 18:56:09 UTC) #22
commit-bot: I haz the power
7 years, 2 months ago (2013-10-29 21:35:07 UTC) #23
Message was sent while issue was closed.
Change committed as 160866

Powered by Google App Engine
This is Rietveld 408576698