Issue metadata
Sign in to add a comment
|
Bad-cast to blink::LayoutBox from blink::LayoutBR;blink::PaintLayer::setNeedsCompositingInputsUpdate;blink::RootScrollerController::recomputeEffectiveRootScroller |
||||||||||||||||||||||
Issue descriptionDetailed report: https://cluster-fuzz.appspot.com/testcase?key=5432883591512064 Fuzzer: bj_broddelwerk Job Type: linux_cfi_chrome Platform Id: linux Crash Type: Bad-cast Crash Address: 0x7f493b747508 Crash State: Bad-cast to blink::LayoutBox from blink::LayoutBR blink::PaintLayer::setNeedsCompositingInputsUpdate blink::RootScrollerController::recomputeEffectiveRootScroller Recommended Security Severity: High Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_cfi_chrome&range=434312:434364 Minimized Testcase (3.09 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97OYWNjZQW1bfSNGSQdzPWYqByZ-8_sjVsEUXJAPTMTPtCSvHLbjFjFB8c2INIhQYTVJ1K0THR2kySUzwmwKCOeT4neHqdMsXbCYW1NPVpSRlE675C80sLzy0FPotlf6nNo5l2sMxB9kfCAJD3MHsSfSkwFuQ?testcase_id=5432883591512064 Issue filed automatically. See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
,
Nov 25 2016
(need to steal this bug to gain access to the report)
,
Nov 25 2016
Document::documentElement() returns an HTMLBRElement, and the root scroller uses this as its effective root scroller (and assumes that it's LayoutBox or better, which sounds reasonable - I think the CSS spec requires that it be a block). Now, how can document.documentElement become a BR?
,
Nov 25 2016
Editing seems relevant.
,
Nov 25 2016
Editing does indeed trigger the crash (but it's possible to cause the same problem without it). At some point, the document is left with a BR element and nothing else (no HTML root, no BODY). The BR element becomes document.documentRoot and it also becomes the root scroller. The BR element doesn't have a layout object at this point, because it wouldn't be a LayoutBox (but rather LayoutBR), and LayoutView refuses to accept the child when reattaching the tree. Then Editor::tidyUpHTMLStructure() repairs the document, by wrapping the BR inside <html><body>. Then the BR creates a LayoutBR. We get another layout pass. The root scroller is still the BR element, though (missing update here?). If we get a call to RootScrollerController::rootScrollerPaintLayer() before the root scroller is updated to the HTML root, we'll crash. It's possible to do this without editing, though. Attaching test case. If we really should allow a BR to be a document root, this needs to be fixed in the scrolling code.
,
Nov 25 2016
,
Nov 25 2016
This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 25 2016
,
Nov 25 2016
Thanks for the detailed analysis. Looks to me like a series of bugs: 1) rootScroller should never accept an element without a LayoutBox 2) rootScroller isn't updated after the layout pass with the repaired document 3) My gut feeling is that we shouldn't be returning <br> as the documentElement, that seems wrong and I wouldn't be surprised if it violates assumptions elsewhere. I'll look to fix #1 so we can fix the security regression in M56. 2 and 3 I'll fix separately once the immediate issue is fixed.
,
Nov 28 2016
I have a fix up at https://codereview.chromium.org/2531343002 for #1 #2 is actually working fine, the issue was *while* the root scroller was updating, it was assuming the *old* root scroller was a Box. Fixing #1 means the root scroller is appropriately updated. I think #3 is actually seems intentional and changing it could have wide ranging implications so I'm going to leave it alone.
,
Nov 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1a413b53d66a15908a7960fe1d7a3dc3c70a8a5e commit 1a413b53d66a15908a7960fe1d7a3dc3c70a8a5e Author: bokan <bokan@chromium.org> Date: Mon Nov 28 23:27:34 2016 Don't assume current root scroller is a LayoutBox. There was an edge case here where we might get an arbitrary element as the document.documentElement. In these cases, the element won't get a LayoutObject as LayoutView will refuse to attach to it, but when we replace it a layout will create a non-LayoutBox for it and when RootScroller tries to replace it as the effective root scroller it needs to do some book keeping using the old root scroller. It incorrectly assumed it must be a LayoutBox. This patch now guards against this edge case. BUG= 668553 Review-Url: https://codereview.chromium.org/2531343002 Cr-Commit-Position: refs/heads/master@{#434771} [modify] https://crrev.com/1a413b53d66a15908a7960fe1d7a3dc3c70a8a5e/third_party/WebKit/Source/core/page/scrolling/RootScrollerUtil.cpp [modify] https://crrev.com/1a413b53d66a15908a7960fe1d7a3dc3c70a8a5e/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp
,
Nov 28 2016
,
Nov 29 2016
ClusterFuzz has detected this issue as fixed in range 434723:434847. Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5432883591512064 Fuzzer: bj_broddelwerk Job Type: linux_cfi_chrome Platform Id: linux Crash Type: Bad-cast Crash Address: 0x7f493b747508 Crash State: Bad-cast to blink::LayoutBox from blink::LayoutBR blink::PaintLayer::setNeedsCompositingInputsUpdate blink::RootScrollerController::recomputeEffectiveRootScroller Recommended Security Severity: High Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_cfi_chrome&range=434312:434364 Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_cfi_chrome&range=434723:434847 Minimized Testcase (3.09 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97OYWNjZQW1bfSNGSQdzPWYqByZ-8_sjVsEUXJAPTMTPtCSvHLbjFjFB8c2INIhQYTVJ1K0THR2kySUzwmwKCOeT4neHqdMsXbCYW1NPVpSRlE675C80sLzy0FPotlf6nNo5l2sMxB9kfCAJD3MHsSfSkwFuQ?testcase_id=5432883591512064 See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
Nov 29 2016
ClusterFuzz testcase is verified as fixed, closing issue. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Nov 29 2016
,
Nov 29 2016
Reopening as this still needs a merge into 56.
,
Nov 29 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
Nov 30 2016
Sorry, this is actually fine in M56 as the bug was introduced by https://codereview.chromium.org/2499853002 which was after branch.
,
Dec 15 2016
,
Mar 8 2017
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by dominickn@chromium.org
, Nov 24 2016Components: Blink>Layout
Owner: dgro...@chromium.org
Status: Assigned (was: Untriaged)