Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(296)

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

Created:
4 years, 1 month ago by Mariusz Mlynski
Modified:
4 years 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, ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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, ...
4 years, 1 month 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 ...
4 years, 1 month 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: > ...
4 years, 1 month 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 > ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month ago (2016-11-20 04:46:16 UTC) #21
Mariusz Mlynski
PTAL.
4 years, 1 month 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. ...
4 years, 1 month ago (2016-11-21 01:26:31 UTC) #23
dcheng
LG once haraken@'s comment is addressed.
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month ago (2016-11-22 06:30:14 UTC) #26
dcheng
lgtm as well
4 years, 1 month ago (2016-11-22 07:07:27 UTC) #27
jochen (gone - plz use gerrit)
lgtm
4 years 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 ...
4 years ago (2016-12-02 03:01:02 UTC) #29
esprehn
Lgtm, let's do it!
4 years ago (2016-12-02 04:16:40 UTC) #30
dcheng
Marius, can we go ahead and land this?
4 years 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
4 years 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 ...
4 years 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 ...
4 years 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
4 years 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 ...
4 years ago (2016-12-05 22:37:58 UTC) #41
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years 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}
4 years ago (2016-12-05 23:35:47 UTC) #46
Mariusz Mlynski
4 years 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