|
|
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. |
DescriptionDon'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. #
Messages
Total messages: 47 (17 generated)
marius.mlynski@gmail.com changed reviewers: + dcheng@chromium.org
This mitigates 117226, 117550, 143439, 456518, 464552, 516377, 519558, 541206, 560011, 605766, 621362, 628942, 630870, 645211, 655904, 658535, 663476, and possibly quite a few more known and unknown cases. With this patch, iframe elements with a content frame won't be able to bypass SOP checks if attached from the parser. The security check, apart from being used to do what it's intended for, is also called to determine whether a layout object is needed for a frame element. Ideally, layout code should use some other function, but I'm not going to complicate this too much and have introduced a way to keep the current behavior to avoid potential regressions with layoutObjectIsNeeded returning false where it used to return true.
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dcheng@chromium.org changed reviewers: + jochen@chromium.org
This patch LGTM. I think there are still some underlying bugs (elsewhere) that we must address in followups, but this is a good general mitigation. +jochen for OWNERS review for bindings changes Some minor nits about the CL description: usually it's formatted as a shorter one-line description followed by a longer description wrapped at ~72 chars: this is mostly so the description doesn't get mangled too horribly if it gets merged / reverted. In this particular case, I'd suggest shortening the title to: Don't skip security checks for javascript: URLs when the JS stack is empty. And then expanding more on the approach in the details: Instead of just skipping the check and always assuming it's safe when there's no current context, use the security context of the containing Document to check. [more details here about why this is the correct (or incorrect) context, and if incorrect, why it's better than just skipping the check] I think the last sentence is important: if we've ended up in the situation where we're about to check if a javascript: URL is allowed with no current context, but the javascript: URL was originally set as the result of executing some script, we're already in a bad place. But it's still better to check against something rather than blindly assuming that it's safe to execute the javascript: URL. (Also, don't forget to git cl format; it looks like the presubmit is complaining) https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:67: if (!fromLayout && !document().getSecurityOrigin()->canAccess( How come we need to skip this block if we're in layout? If we didn't have this, we could (in theory) combine this into: LocalDOMWindow* accessingWindow = isolate->InContext() ? currentDOMWindow(isolate) : document()->window(); if (!BindingSecurity::shouldAllowAccessToFrame(...)) (I would be OK investigating this in a followup though, there is some risk here: if there's a contentFrame(), the containing document() should be active, but there's no asserts that document that, and who knows what strange corner cases are out there)
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Not lgtm, that bool is named wrong and also looks very suspect. https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:53: bool HTMLFrameElementBase::isURLAllowed(bool fromLayout) const { This check is named wrong, you set that bool inside the layout object creation check which is inside style recalc. All returning false there does is make the iframe display none, it doesn't control if the frame is loaded or not, and it isn't from layout. I guess what you're trying to do is keep the tests passing that display that frame but it's not at all clear to me why we make iframes for urls that are not allowed display none. Why shouldn't the iframe display a border at least? What do other browsers do here? If we want to keep this behavior we should split this function and have some shared checks and one method for if the layout object is needed and another for is we should load the URL.
On 2016/11/15 08:27:12, esprehn wrote: > Not lgtm, that bool is named wrong and also looks very suspect. > > https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): > > https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:53: bool > HTMLFrameElementBase::isURLAllowed(bool fromLayout) const { > This check is named wrong, you set that bool inside the layout object creation > check which is inside style recalc. All returning false there does is make the > iframe display none, it doesn't control if the frame is loaded or not, and it > isn't from layout. > > I guess what you're trying to do is keep the tests passing that display that > frame but it's not at all clear to me why we make iframes for urls that are not > allowed display none. Why shouldn't the iframe display a border at least? What > do other browsers do here? > > If we want to keep this behavior we should split this function and have some > shared checks and one method for if the layout object is needed and another for > is we should load the URL. esprehn: I agree that calling isURLAllowed() from layout is pretty bogus. I actually have an alternate CL that might fix this: https://codereview.chromium.org/2504573002
On 2016/11/15 08:29:23, dcheng wrote: > On 2016/11/15 08:27:12, esprehn wrote: > > Not lgtm, that bool is named wrong and also looks very suspect. > > > > > https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): > > > > > https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:53: bool > > HTMLFrameElementBase::isURLAllowed(bool fromLayout) const { > > This check is named wrong, you set that bool inside the layout object creation > > check which is inside style recalc. All returning false there does is make the > > iframe display none, it doesn't control if the frame is loaded or not, and it > > isn't from layout. > > > > I guess what you're trying to do is keep the tests passing that display that > > frame but it's not at all clear to me why we make iframes for urls that are > not > > allowed display none. Why shouldn't the iframe display a border at least? What > > do other browsers do here? > > > > If we want to keep this behavior we should split this function and have some > > shared checks and one method for if the layout object is needed and another > for > > is we should load the URL. > > esprehn: I agree that calling isURLAllowed() from layout is pretty bogus. I > actually have an alternate CL that might fix this: > https://codereview.chromium.org/2504573002 (style recalc, even)
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:61: if (isolate->InContext()) { In the first place, would you help me understand why we need to check a different thing depending on whether we have JavaScript on stack or not?
https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:61: if (isolate->InContext()) { On 2016/11/15 08:44:24, haraken wrote: > > In the first place, would you help me understand why we need to check a > different thing depending on whether we have JavaScript on stack or not? Here is my understanding: If there is no current context, the javascript: URL must come from the parser. If it is coming from the parser, it must have been specified like this: <iframe src="javascript:..."></iframe> Since the iframe must be same-origin, it is always safe to execute the javascript: URL, so we can skip the security checks. If there is a current context, then the javascript: URL could have been set by script like this: window[0].location = 'javascript:...'; In this case, we must make sure that the current context is same-origin with the target frame; otherwise, the javascript: URL will be executing in a cross-origin context. However, the problem is with the "if there is no context, always allow" branch. In theory, this should be true. However, if there is a Blink bug that causes a frame that already has a cross-origin document to be re-inserted from the parser, the security check gets skipped since there is no current context. (I don't quite understand the last part: the calls that move nodes around in the DOM are coming from JS, so I'm not sure why there's no current context. See https://crbug.com/456518, which hints this to be the case)
On 2016/11/15 08:55:07, dcheng wrote: > https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): > > https://codereview.chromium.org/2502783004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:61: if > (isolate->InContext()) { > On 2016/11/15 08:44:24, haraken wrote: > > > > In the first place, would you help me understand why we need to check a > > different thing depending on whether we have JavaScript on stack or not? > > Here is my understanding: > > If there is no current context, the javascript: URL must come from the parser. > If it is coming from the parser, it must have been specified like this: > > <iframe src="javascript:..."></iframe> > > Since the iframe must be same-origin, it is always safe to execute the > javascript: URL, so we can skip the security checks. > > If there is a current context, then the javascript: URL could have been set by > script like this: > > window[0].location = 'javascript:...'; > > In this case, we must make sure that the current context is same-origin with the > target frame; otherwise, the javascript: URL will be executing in a cross-origin > context. > > However, the problem is with the "if there is no context, always allow" branch. > In theory, this should be true. However, if there is a Blink bug that causes a > frame that already has a cross-origin document to be re-inserted from the > parser, the security check gets skipped since there is no current context. > > (I don't quite understand the last part: the calls that move nodes around in the > DOM are coming from JS, so I'm not sure why there's no current context. See > https://crbug.com/456518, which hints this to be the case) Thanks, makes sense. Would it be crazy to make the parser instantiate a context before evaluating the javascript URL? That will enable us to remove the risky branch...
On 2016/11/15 08:44:25, haraken wrote: > In the first place, would you help me understand why we need to check a > different thing depending on whether we have JavaScript on stack or not? I feel it's correct to always use the current Javascript context, and only fall back to other options if there's no other choice. With JavaScript on the stack, there might be corner cases (involving document.domain changes, etc.) where simply using the frame's containing document could potentially give wrong results. Of course, setting the src attribute of a frame element requires that JS has a reference to the frame element, and if JavaScript code has a reference to a frame element then it means it'll be able to execute script in the context of the frame's containing document one way or another, but still... HTMLFrameElementBase::isURLAllowed has been using currentDOMWindow for years, and I'd feel uncomfortable changing anything about it in this patch, so it just adds the safe fallback case. On 2016/11/15 08:55:07, dcheng wrote: > window[0].location = 'javascript:...'; For the record, window[0].location = 'javascript:...'; doesn't use HTMLFrameElementBase::isURLAllowed at all, HTMLFrameElementBase::isURLAllowed (excluding the layoutObjectIsNeeded calls) is called only from HTMLFrameElementBase::openURL, which is called from HTMLFrameElementBase::setNameAndOpenURL (subtree insertion notifications) and HTMLFrameElementBase::setLocation (frame element's src attribute handling). > (I don't quite understand the last part: the calls that move nodes around in the > DOM are coming from JS, so I'm not sure why there's no current context. Not necessarily from JS, there are cases when nodes are moved around without any JS on the stack, for example the adoption agency algorithm or the error message block insertion in XML documents. On 2016/11/15 08:59:58, haraken wrote: > Would it be crazy to make the parser instantiate a context before evaluating the > javascript URL? That will enable us to remove the risky branch... There are no risky branches after this patch, this is the exact point of it. When the parser performs actions without any JS on the stack, you can safely assume that it's the frame's containing document's context that you want to use for security checks. I think that trying to rely on manual context instantiation could be error prone and might lead to problems with obscure code paths bypassing the instantiation step. I'll address the patch comments soon.
Description was changed from ========== Don't skip the security check of a frame's URL if the JavaScript stack is empty, instead use the security context of the frame's containing document. Currently, the security check assumes that it's always safe to execute script in the context of the attached iframe if there are no JavaScript frames on the stack. This is wrong and leads to UXSS in numerous unexpected circumstances. BUG=too many to mention ========== to ========== 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=too many to mention ==========
Changelog: -Corrected formatting. -Removed the suspect bool. -Replaced canAccess() with canAccessCheckSuborigins(), because this is what BindingSecurity::shouldAllowAccessToFrame does internally.
https://codereview.chromium.org/2502783004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:68: contentFrame()->securityContext()->getSecurityOrigin())) Is it possible to use BindingSecurity::shouldAllowAccessToFrame() here as well? We can pass document()->window(), right? Or can we get here for a detached frame element? (I would find that very surprising, but line 73 seems to hint this might be possible?)
https://codereview.chromium.org/2502783004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:68: contentFrame()->securityContext()->getSecurityOrigin())) On 2016/11/19 06:33:02, dcheng wrote: > Is it possible to use BindingSecurity::shouldAllowAccessToFrame() here as well? > We can pass document()->window(), right? Or can we get here for a detached frame > element? > > (I would find that very surprising, but line 73 seems to hint this might be > possible?) Not really for a detached frame element, it's the case of a frame element attached to a document with no frame, like: document.implementation.createHTMLDocument().documentElement.appendChild(document.createElement('iframe')).src = 'javascript:alert(1)'; It still won't call BindingSecurity::shouldAllowAccessToFrame, because contentFrame() should be null. Even if that wasn't the case, it's fine to call BindingSecurity::shouldAllowAccessToFrame with the accessingWindow being null -- it's passed to canAccessFrame, which passes it to canAccessFrameInternal, which simply returns false for this case, and the javascript: URL is rejected. I'll update the patch in a moment.
PTAL.
Looks good to me but dcheng@ or jochen@ would be a better reviewer for this. https://codereview.chromium.org/2502783004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:59: if (contentFrame() && protocolIsJavaScript(completeURL)) { Add a comment and explain what these security checks are doing.
LG once haraken@'s comment is addressed.
On 2016/11/22 06:00:16, dcheng wrote: > LG once haraken@'s comment is addressed. LGTM with the comment addressed
https://codereview.chromium.org/2502783004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right): https://codereview.chromium.org/2502783004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:59: if (contentFrame() && protocolIsJavaScript(completeURL)) { On 2016/11/21 01:26:31, haraken wrote: > > Add a comment and explain what these security checks are doing. Done.
lgtm as well
lgtm
Elliot, are you OK with this version of the patch? It'd be nice to land a general mitigation.
Lgtm, let's do it!
Marius, can we go ahead and land this?
The CQ bit was checked by marius.mlynski@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2502783004/#ps60001 (title: "Added comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by esprehn@chromium.org
Can you update the BUG= line to link to at least one example of a bug? "too many to mention" doesn't help anyone doing archaeology in the future.
Description was changed from ========== 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=too many to mention ========== to ========== 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 ==========
On 2016/12/05 22:25:03, esprehn wrote: > Can you update the BUG= line to link to at least one example of a bug? "too many > to mention" doesn't help anyone doing archaeology in the future. Sure, let's use the very first one.
The CQ bit was checked by marius.mlynski@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/05 at 22:28:21, marius.mlynski wrote: > On 2016/12/05 22:25:03, esprehn wrote: > > Can you update the BUG= line to link to at least one example of a bug? "too many > > to mention" doesn't help anyone doing archaeology in the future. > > Sure, let's use the very first one. Awesome, thanks!
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480976907479730, "parent_rev": "b0b8ab82a1cfa686c65dd506a8a2019db0bb80ef", "commit_rev": "5da20e136e5f174ba0d7c526ea428f39e0f9bfd9"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/783e19486cab2b7485b4a19c02a2eb0369f3b350 Cr-Commit-Position: refs/heads/master@{#436449}
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! |