New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 680224 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
NOT IN USE
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security



Sign in to add a comment

Heap-use-after-free in blink::LayoutBox::getPaginationBreakability

Project Member Reported by ClusterFuzz, Jan 11 2017

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5658905435963392

Fuzzer: ifratric-browserfuzzer-v3
Job Type: linux_asan_chrome_media
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x612000008480
Crash State:
  blink::LayoutBox::getPaginationBreakability
  blink::LayoutTableRow::getPaginationBreakability
  blink::InitialColumnHeightFinder::examineBoxAfterEntering
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_media&range=434255:434304

Minimized Testcase (0.69 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv95xZcQy-vOfSFMIWKC2yztMt-OF2jpb0WjgzxuxprmtoBaicJLHVRNOPt_9KOshVwfoHmcCLqFPlDRfSrbIEuug6Tr6ImOYS946WwZmNyUZzBkWc9rLCyucRFkc9NpXfn6khap7K9476sgIZwLss2j_t-Ems3Eix5BPodv23JDsvk3an35l1ZsBw--QqyS4CjBeVJDk4c45qdhaQir5zue05CX7OYPcD0gijIXO6ikcMb1nrQKIvIT-CUFknMNDmx5fotUgrnas5oP53Tagyq6aYGoF_ZDltgeoleEOQTNikMNjOQ6ApcyeFarE0LqNTDh0MtRavouXRWfINNEu2oDnfyyS92ZypFSZIR8S0e2r8gcSL1vMtcP4lwazKZV7A-U-laAQbAyvqU77Sc2qZ2c53aeNBA?testcase_id=5658905435963392
<style>
.class7, .class1 { isolation: isolate; contain: strict; font-weight: normal }
.class2 { mso-style-next: Normal; overflow: -webkit-paged-x; writing-mode: vertical-lr;</style>
<script>
function eventhandler4() {
 /* HTMLTableSectionElement*/ var var00053 = htmlvar00023.createTHead(); 
 var00053.setAttribute("hidden", "hidden"); 
 if(htmlvar00009) htmlvar00009.appendChild(htmlvar00046); 
}
</script>
<dl id="htmlvar00009" class="class7">
<dd class="class2">
<ol>
<li class="class7" tabindex="-1">
<table id="htmlvar00023">
<tr>
<audio onloadstart="eventhandler4()">
<source id="htmlvar00035" srcset="Bl]`YR0sjKF"9]@sc8Du&amp;</source>
<thead>
</table>
</ol>
<map id="htmlvar00046")9H2seb3~%-A">


Issue filed automatically.

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Project Member

Comment 1 by sheriffbot@chromium.org, Jan 12 2017

Labels: M-57
Project Member

Comment 2 by sheriffbot@chromium.org, Jan 12 2017

Labels: ReleaseBlock-Beta
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
Project Member

Comment 3 by sheriffbot@chromium.org, Jan 12 2017

Labels: Pri-1
Project Member

Comment 4 by sheriffbot@chromium.org, Jan 13 2017

Labels: ReleaseBlock-Beta
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
Components: Blink>Layout
Owner: msten...@opera.com
Status: Assigned (was: Untriaged)
mstensho: can you please take a look at this?

Comment 6 by msten...@opera.com, Jan 17 2017

Attaching a cleaned up test case and trying to explain what goes wrong:

...
<body>
  <div id="outerStrict" style="contain:strict;">
    <div id="multicol" style="columns:1; writing-mode:vertical-lr;">
      <div id="multicolChild"></div>
      <div id="regularBlock">
        <div id="innerStrict" style="contain:strict;">
          <table>
            <thead id="thead"></thead>
            <tbody>
              <tr>

If we set display:none on both #multicolChild and #thead here, we're going to mark for re-layout. #multicolChild will mark #multicol for layout, and #outerStrict too, but it will stop there, since #outerStrict is a re-layout boundary element, due to contain:strict. #thead will mark its table parent for layout, and also make sure that the table is marked with needsSectionRecalc. We'll also mark #innerStrict for layout, but not continue further up the container chain, since #innerStrict also is a re-layout boundary.

In other words, #regularBlock will not be marked for layout (while both its child and its parent will be marked). This is how containment is supposed to work, so fine.

There's a tree depth sorted list of re-layout roots, which ensures that #innerStrict will be laid out before #outerStrict. It would be kind of bad if #outerStrict just skipped over a subtree of objects needing layout, so doing it depth-first is essential.

Unfortunately, that's exactly what goes wrong in our case, because of an additional mechanism: Objects that establish an orthogonal flow - i.e. #multicol in our case - will be laid out separately before laying out the rest. So we end up laying out #multicol first. We'll skip #regularBlock, since it hasn't been marked for layout, although it does have descendants at this point that need layout. At the end of #multicol layout, the column balancer kicks in, and examines the subtree, which still consists of objects needing layout. When examining the TR, we consult the table and get a dead layout object (it went display:none on us, remember?) for the THEAD returned.
tc.html
605 bytes View Download

Comment 7 by msten...@opera.com, Jan 17 2017

Cc: dgro...@chromium.org e...@chromium.org
Blockedon: 682307

Comment 9 by msten...@opera.com, Jan 19 2017

Blockedon: -682307
Cc: kojii@chromium.org
Cc: wangxianzhu@chromium.org

A friendly reminder that M57 Beta launch is coming soon on February 2nd! Your bug is labelled as Beta ReleaseBlock, pls make sure to land the fix and get it merged into the release branch (2987) ASAP so it gets enough baking time in Dev (before Beta promotion). Thank you!

Comment 12 by msten...@opera.com, Jan 23 2017

Fix already in code review. I'll ping.
Project Member

Comment 13 by bugdroid1@chromium.org, Jan 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/538d2655968dd8e6a8c3b8baec1b496dd4f74882

commit 538d2655968dd8e6a8c3b8baec1b496dd4f74882
Author: mstensho <mstensho@opera.com>
Date: Mon Jan 23 21:45:04 2017

Merge list of orthogonal writing mode roots into depth-ordered layout list.

If we're going to perform a series of subtree layouts, rather than one layout
from LayoutView, and we at the same time have a list of orthogonal writing mode
roots that need to be laid out before their ancestors, we need to make sure
that subtrees are laid out in an overall tree depth ordered manner, or we risk
skipping layout of a subtree needing layout. That would cause trouble for the
column balancer (which examines the tree after layout and expects everything to
be laid out), and quite possibly other kinds of trouble elsewhere too.

BUG= 680224 

Review-Url: https://codereview.chromium.org/2635143003
Cr-Commit-Position: refs/heads/master@{#445497}

[add] https://crrev.com/538d2655968dd8e6a8c3b8baec1b496dd4f74882/third_party/WebKit/LayoutTests/fast/multicol/contain-strict-orthogonal-writing-mode-root-crash.html
[modify] https://crrev.com/538d2655968dd8e6a8c3b8baec1b496dd4f74882/third_party/WebKit/Source/core/frame/FrameView.cpp
[modify] https://crrev.com/538d2655968dd8e6a8c3b8baec1b496dd4f74882/third_party/WebKit/Source/core/frame/FrameView.h

Comment 14 by msten...@opera.com, Jan 24 2017

Status: Fixed (was: Assigned)
Project Member

Comment 15 by ClusterFuzz, Jan 24 2017

ClusterFuzz has detected this issue as fixed in range 445491:445525.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5658905435963392

Fuzzer: ifratric-browserfuzzer-v3
Job Type: linux_asan_chrome_media
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x612000008480
Crash State:
  blink::LayoutBox::getPaginationBreakability
  blink::LayoutTableRow::getPaginationBreakability
  blink::InitialColumnHeightFinder::examineBoxAfterEntering
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_media&range=434255:434304
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_media&range=445491:445525

Minimized Testcase (0.69 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv95xZcQy-vOfSFMIWKC2yztMt-OF2jpb0WjgzxuxprmtoBaicJLHVRNOPt_9KOshVwfoHmcCLqFPlDRfSrbIEuug6Tr6ImOYS946WwZmNyUZzBkWc9rLCyucRFkc9NpXfn6khap7K9476sgIZwLss2j_t-Ems3Eix5BPodv23JDsvk3an35l1ZsBw--QqyS4CjBeVJDk4c45qdhaQir5zue05CX7OYPcD0gijIXO6ikcMb1nrQKIvIT-CUFknMNDmx5fotUgrnas5oP53Tagyq6aYGoF_ZDltgeoleEOQTNikMNjOQ6ApcyeFarE0LqNTDh0MtRavouXRWfINNEu2oDnfyyS92ZypFSZIR8S0e2r8gcSL1vMtcP4lwazKZV7A-U-laAQbAyvqU77Sc2qZ2c53aeNBA?testcase_id=5658905435963392
<style>
.class7, .class1 { isolation: isolate; contain: strict; font-weight: normal }
.class2 { mso-style-next: Normal; overflow: -webkit-paged-x; writing-mode: vertical-lr;</style>
<script>
function eventhandler4() {
 /* HTMLTableSectionElement*/ var var00053 = htmlvar00023.createTHead(); 
 var00053.setAttribute("hidden", "hidden"); 
 if(htmlvar00009) htmlvar00009.appendChild(htmlvar00046); 
}
</script>
<dl id="htmlvar00009" class="class7">
<dd class="class2">
<ol>
<li class="class7" tabindex="-1">
<table id="htmlvar00023">
<tr>
<audio onloadstart="eventhandler4()">
<source id="htmlvar00035" srcset="Bl]`YR0sjKF"9]@sc8Du&amp;</source>
<thead>
</table>
</ol>
<map id="htmlvar00046")9H2seb3~%-A">


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.
Project Member

Comment 16 by sheriffbot@chromium.org, Jan 24 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -ReleaseBlock-Beta ReleaseBlock-Stable
Labels: Merge-Request-57
Project Member

Comment 19 by sheriffbot@chromium.org, Feb 13 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 20 by bugdroid1@chromium.org, Feb 13 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eb6e98df03a497ed99cc06e8741c18c12ae67572

commit eb6e98df03a497ed99cc06e8741c18c12ae67572
Author: Morten Stenshorne <mstensho@opera.com>
Date: Mon Feb 13 20:30:02 2017

Merge list of orthogonal writing mode roots into depth-ordered layout list.

If we're going to perform a series of subtree layouts, rather than one layout
from LayoutView, and we at the same time have a list of orthogonal writing mode
roots that need to be laid out before their ancestors, we need to make sure
that subtrees are laid out in an overall tree depth ordered manner, or we risk
skipping layout of a subtree needing layout. That would cause trouble for the
column balancer (which examines the tree after layout and expects everything to
be laid out), and quite possibly other kinds of trouble elsewhere too.

BUG= 680224 

Review-Url: https://codereview.chromium.org/2635143003
Cr-Commit-Position: refs/heads/master@{#445497}
(cherry picked from commit 538d2655968dd8e6a8c3b8baec1b496dd4f74882)

Review-Url: https://codereview.chromium.org/2696713002 .
Cr-Commit-Position: refs/branch-heads/2987@{#486}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[add] https://crrev.com/eb6e98df03a497ed99cc06e8741c18c12ae67572/third_party/WebKit/LayoutTests/fast/multicol/contain-strict-orthogonal-writing-mode-root-crash.html
[modify] https://crrev.com/eb6e98df03a497ed99cc06e8741c18c12ae67572/third_party/WebKit/Source/core/frame/FrameView.cpp
[modify] https://crrev.com/eb6e98df03a497ed99cc06e8741c18c12ae67572/third_party/WebKit/Source/core/frame/FrameView.h

Labels: -Hotlist-Merge-Approved -ReleaseBlock-Stable
Project Member

Comment 22 by bugdroid1@chromium.org, Feb 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25993aa7c80d68de1a53bff157d02c8bc8ec3458

commit 25993aa7c80d68de1a53bff157d02c8bc8ec3458
Author: mstensho <mstensho@opera.com>
Date: Fri Feb 17 07:10:28 2017

Don't keep pointers to table sections when told to recalculate sections.

They may have been deleted.

We should ideally assert in header(), footer() and firstBody() in LayoutTable
that we're not waiting for a section recalc, but that's currently failing in one
trybot; see crbug.com/693212

This would have fixed the original use-after-free issue with  bug 680224 , but
before this CL landed, I found another fix that attacked the root cause instead.
Still no point in keeping potentially dead pointers around, though.

BUG= 680224 

Review-Url: https://codereview.chromium.org/2636153002
Cr-Commit-Position: refs/heads/master@{#451257}

[modify] https://crrev.com/25993aa7c80d68de1a53bff157d02c8bc8ec3458/third_party/WebKit/Source/core/layout/LayoutTable.h

Project Member

Comment 23 by sheriffbot@chromium.org, May 2 2017

Labels: -Restrict-View-SecurityNotify allpublic
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