|
|
Created:
4 years, 1 month ago by dcheng Modified:
4 years, 1 month ago Reviewers:
esprehn CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, jochen (gone - plz use gerrit), dominicc (has gone to gerrit) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSECURITY_CHECK that inserted HTMLFrameElementBases aren't already attached.
The consequences of getting this wrong are quite bad. Corrupting Blink's
state about the DOM tree and causing an already-attached frame to be
reattached is a common UXSS primitive: if the frame is reattached with
a javascript: URL, the normal security checks are bypassed, allowing
cross-origin script execution.
Taken from https://codereview.chromium.org/2169453002.
BUG=663476
Patch Set 1 #
Messages
Total messages: 15 (7 generated)
Description was changed from ========== Add a SECURITY_CHECK that HTMLFrameElementBase is a consistent state. The consequences of getting this wrong are quite bad. Corrupting Blink's state about the DOM tree and causing an already-attached frame to be reattached is a common UXSS primitive: if the frame is reattached with a javascript: URL, the normal security checks are bypassed, allowing cross-origin script execution. BUG=663476 ========== to ========== Add a SECURITY_CHECK to enforce HTMLFrameElemneBase insertion preconditions. The consequences of getting this wrong are quite bad. Corrupting Blink's state about the DOM tree and causing an already-attached frame to be reattached is a common UXSS primitive: if the frame is reattached with a javascript: URL, the normal security checks are bypassed, allowing cross-origin script execution. Taken from https://codereview.chromium.org/2169453002. BUG=663476 ==========
dcheng@chromium.org changed reviewers: + esprehn@chromium.org
Description was changed from ========== Add a SECURITY_CHECK to enforce HTMLFrameElemneBase insertion preconditions. The consequences of getting this wrong are quite bad. Corrupting Blink's state about the DOM tree and causing an already-attached frame to be reattached is a common UXSS primitive: if the frame is reattached with a javascript: URL, the normal security checks are bypassed, allowing cross-origin script execution. Taken from https://codereview.chromium.org/2169453002. BUG=663476 ========== to ========== SECURITY_CHECK that inserted HTMLFrameElementBases aren't already attached. The consequences of getting this wrong are quite bad. Corrupting Blink's state about the DOM tree and causing an already-attached frame to be reattached is a common UXSS primitive: if the frame is reattached with a javascript: URL, the normal security checks are bypassed, allowing cross-origin script execution. Taken from https://codereview.chromium.org/2169453002. BUG=663476 ==========
This will cause UXSSes to crash the renderer instead, but I think that's preferable. We should stop the bleeding. As a bonus, I think this will help clusterfuzz find more DOM corruption issues (it already found one from the previous time we tried to commit this patch).
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: This issue passed the CQ dry run.
It can't be guaranteed that there's no content frame, here's a simple example that will terminate the renderer: <script> var d = document.createElement('div'); var i0 = d.appendChild(document.createElement('iframe')); var i1 = d.appendChild(document.createElement('iframe')); i0.src = "javascript:parent.i1.src='https://www.google.com'"; document.documentElement.appendChild(d); </script>
On 2016/11/12 22:24:54, Mariusz Mlynski wrote: > It can't be guaranteed that there's no content frame, here's a simple example > that will terminate the renderer: > > <script> > var d = document.createElement('div'); > var i0 = d.appendChild(document.createElement('iframe')); > var i1 = d.appendChild(document.createElement('iframe')); > i0.src = "javascript:parent.i1.src='https://www.google.com'"; > document.documentElement.appendChild(d); > </script> I agree that that's not true in Blink today, but is there a reason that Blink shouldn't begin enforcing this invariant? The fuzzer has found similar crashing fragments, but I think we should be detaching the frame before reinserting it...
ping =)
On 2016/11/12 at 23:56:38, dcheng wrote: > On 2016/11/12 22:24:54, Mariusz Mlynski wrote: > > It can't be guaranteed that there's no content frame, here's a simple example > > that will terminate the renderer: > > > > <script> > > var d = document.createElement('div'); > > var i0 = d.appendChild(document.createElement('iframe')); > > var i1 = d.appendChild(document.createElement('iframe')); > > i0.src = "javascript:parent.i1.src='https://www.google.com'"; > > document.documentElement.appendChild(d); > > </script> > > I agree that that's not true in Blink today, but is there a reason that Blink shouldn't begin enforcing this invariant? The fuzzer has found similar crashing fragments, but I think we should be detaching the frame before reinserting it... We can't land anything that'll make a simple test case like that crash...
On 2016/11/12 23:56:38, dcheng wrote: > I agree that that's not true in Blink today, but is there a reason that Blink > shouldn't begin enforcing this invariant? The fuzzer has found similar crashing > fragments, but I think we should be detaching the frame before reinserting it... No idea -- seeing that it'd be observable by web content, I'm not too sure what might break. For example, if a scheduled frame navigation was same-origin and there were properties on the initial empty document, they'd be blown away. I think the real problem lies somewhere else, and it's orthogonal to the frame state -- I can try to prepare a mitigation if you'd like to review.
On 2016/11/15 05:00:53, Mariusz Mlynski wrote: > On 2016/11/12 23:56:38, dcheng wrote: > > I agree that that's not true in Blink today, but is there a reason that Blink > > shouldn't begin enforcing this invariant? The fuzzer has found similar > crashing > > fragments, but I think we should be detaching the frame before reinserting > it... > > No idea -- seeing that it'd be observable by web content, I'm not too sure what > might break. For example, if a scheduled frame navigation was same-origin and > there were properties on the initial empty document, they'd be blown away. > > I think the real problem lies somewhere else, and it's orthogonal to the frame > state -- I can try to prepare a mitigation if you'd like to review. Right, I don't think this is the fix for the bug in question, but this is far too commonly abused to turn any DOM corruption into a UXSS. I'll investigate what's going on in the fuzzer cases + the snippet you've linked (I believe it's related).
On 2016/11/15 05:11:16, dcheng wrote: > Right, I don't think this is the fix for the bug in question, but this is far > too commonly abused to turn any DOM corruption into a UXSS. No, I mean DOM corruption UXSS'es in general, not 663476 specifically. I think the root problem is that the security check is skipped in the first place. Please see https://codereview.chromium.org/2502783004 |