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

Issue 2502783004: Don't skip security checks for javascript: URLs when the JS stack is empty. (Closed)

Created:
2 years, 11 months ago by Mariusz Mlynski
Modified:
2 years, 10 months ago
CC:
blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't skip security checks for javascript: URLs when the JS stack is empty. Before this patch, HTMLFrameElementBase::isURLAllowed would skip the security check if there were no JavaScript frames on the stack. This could lead to UXSS bugs if an attacker managed to trick the parser into attaching a frame element with a cross-origin document and the src attribute set to a javascript: URL. After this patch, the security context of the frame's containing document is used to verify if the URL is allowed. Nothing else (expect for some other same-origin context) could've set the src attribute, so we assume it's the most logical choice in the absence of the current JavaScript context. BUG=117226 Committed: https://crrev.com/783e19486cab2b7485b4a19c02a2eb0369f3b350 Cr-Commit-Position: refs/heads/master@{#436449}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Don't skip security checks for javascript: URLs when the JS stack is empty. #

Total comments: 2

Patch Set 3 : Simplified. #

Total comments: 2

Patch Set 4 : Added comment. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -17 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/ScriptController.h View 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp View 2 chunks +0 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp View 1 2 3 2 chunks +13 lines, -4 lines 0 comments Download

Messages

Total messages: 47 (17 generated)
Mariusz Mlynski
This mitigates 117226, 117550, 143439, 456518, 464552, 516377, 519558, 541206, 560011, 605766, 621362, 628942, 630870, ...
2 years, 11 months ago (2016-11-15 07:30:53 UTC) #2
dcheng
This patch LGTM. I think there are still some underlying bugs (elsewhere) that we must ...
2 years, 11 months ago (2016-11-15 08:14:21 UTC) #8
esprehn
Not lgtm, that bool is named wrong and also looks very suspect. https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp ...
2 years, 11 months ago (2016-11-15 08:27:12 UTC) #10
dcheng
On 2016/11/15 08:27:12, esprehn wrote: > Not lgtm, that bool is named wrong and also ...
2 years, 11 months ago (2016-11-15 08:29:23 UTC) #11
dcheng
On 2016/11/15 08:29:23, dcheng wrote: > On 2016/11/15 08:27:12, esprehn wrote: > > Not lgtm, ...
2 years, 11 months ago (2016-11-15 08:30:01 UTC) #12
haraken
https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp#newcode61 third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:61: if (isolate->InContext()) { In the first place, would you ...
2 years, 11 months ago (2016-11-15 08:44:25 UTC) #14
dcheng
https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp#newcode61 third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:61: if (isolate->InContext()) { On 2016/11/15 08:44:24, haraken wrote: > ...
2 years, 11 months ago (2016-11-15 08:55:07 UTC) #15
haraken
On 2016/11/15 08:55:07, dcheng wrote: > https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp > File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): > > https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp#newcode61 > ...
2 years, 11 months ago (2016-11-15 08:59:58 UTC) #16
Mariusz Mlynski
On 2016/11/15 08:44:25, haraken wrote: > In the first place, would you help me understand ...
2 years, 11 months ago (2016-11-15 09:57:30 UTC) #17
Mariusz Mlynski
Changelog: -Corrected formatting. -Removed the suspect bool. -Replaced canAccess() with canAccessCheckSuborigins(), because this is what ...
2 years, 11 months ago (2016-11-15 13:50:15 UTC) #19
dcheng
https://codereview.chromium.org/2502783004/diff/20001/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/20001/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp#newcode68 third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:68: contentFrame()->securityContext()->getSecurityOrigin())) Is it possible to use BindingSecurity::shouldAllowAccessToFrame() here as ...
2 years, 11 months ago (2016-11-19 06:33:03 UTC) #20
Mariusz Mlynski
https://codereview.chromium.org/2502783004/diff/20001/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/20001/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp#newcode68 third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:68: contentFrame()->securityContext()->getSecurityOrigin())) On 2016/11/19 06:33:02, dcheng wrote: > Is it ...
2 years, 10 months ago (2016-11-20 04:46:16 UTC) #21
Mariusz Mlynski
PTAL.
2 years, 10 months ago (2016-11-20 04:57:27 UTC) #22
haraken
Looks good to me but dcheng@ or jochen@ would be a better reviewer for this. ...
2 years, 10 months ago (2016-11-21 01:26:31 UTC) #23
dcheng
LG once haraken@'s comment is addressed.
2 years, 10 months ago (2016-11-22 06:00:16 UTC) #24
haraken
On 2016/11/22 06:00:16, dcheng wrote: > LG once haraken@'s comment is addressed. LGTM with the ...
2 years, 10 months ago (2016-11-22 06:01:42 UTC) #25
Mariusz Mlynski
https://codereview.chromium.org/2502783004/diff/40001/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/40001/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp#newcode59 third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:59: if (contentFrame() && protocolIsJavaScript(completeURL)) { On 2016/11/21 01:26:31, haraken ...
2 years, 10 months ago (2016-11-22 06:30:14 UTC) #26
dcheng
lgtm as well
2 years, 10 months ago (2016-11-22 07:07:27 UTC) #27
jochen (gone - plz use gerrit)
lgtm
2 years, 10 months ago (2016-11-23 15:08:30 UTC) #28
dcheng
Elliot, are you OK with this version of the patch? It'd be nice to land ...
2 years, 10 months ago (2016-12-02 03:01:02 UTC) #29
esprehn
Lgtm, let's do it!
2 years, 10 months ago (2016-12-02 04:16:40 UTC) #30
dcheng
Marius, can we go ahead and land this?
2 years, 10 months ago (2016-12-05 22:03:04 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2502783004/60001
2 years, 10 months ago (2016-12-05 22:05:11 UTC) #34
esprehn
Can you update the BUG= line to link to at least one example of a ...
2 years, 10 months ago (2016-12-05 22:25:03 UTC) #36
Mariusz Mlynski
On 2016/12/05 22:25:03, esprehn wrote: > Can you update the BUG= line to link to ...
2 years, 10 months ago (2016-12-05 22:28:21 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2502783004/60001
2 years, 10 months ago (2016-12-05 22:29:39 UTC) #40
esprehn
On 2016/12/05 at 22:28:21, marius.mlynski wrote: > On 2016/12/05 22:25:03, esprehn wrote: > > Can ...
2 years, 10 months ago (2016-12-05 22:37:58 UTC) #41
commit-bot: I haz the power
Committed patchset #4 (id:60001)
2 years, 10 months ago (2016-12-05 23:35:19 UTC) #44
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/783e19486cab2b7485b4a19c02a2eb0369f3b350 Cr-Commit-Position: refs/heads/master@{#436449}
2 years, 10 months ago (2016-12-05 23:35:47 UTC) #46
Mariusz Mlynski
2 years, 10 months ago (2016-12-05 23:39:40 UTC) #47
Message was sent while issue was closed.
On 2016/12/05 22:03:04, dcheng wrote:
> Marius, can we go ahead and land this?

Done. Thank you all for the reviews!

Powered by Google App Engine
This is Rietveld 408576698