New issue
Advanced search Search tips

Issue 626182 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security



Sign in to add a comment

Heap-use-after-free in blink::PaintController::commitNewDisplayItems

Project Member Reported by ClusterFuzz, Jul 7 2016

Issue description

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

Fuzzer: bj_broddelwerk
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Heap-use-after-free WRITE 4
Crash Address: 0x611000054510
Crash State:
  blink::PaintController::commitNewDisplayItems
  blink::GraphicsLayer::paint
  blink::FrameView::synchronizedPaintRecursively
  
Recommended Security Severity: High

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=392597:392661

Minimized Testcase (4.54 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94LkTZLa1AcE6eHlvNsBRT394Nz9XSLbP7UR46LIF0X9nvtbDRjjfmI5YhLZg5EKSQz-SRzXOS-kh__R-WfchwK6gGyCKzhapd6YkOS3W8DyYsdp12Jr9DuyC6Myezwh7qNK0XC66R4uoBGKnjB7RLf1RsL5Q?testcase_id=4585524119732224

Filer: ochang

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Cc: wangxianzhu@chromium.org
Components: Blink>Paint
Project Member

Comment 2 by sheriffbot@chromium.org, Jul 7 2016

Labels: M-53
Project Member

Comment 3 by sheriffbot@chromium.org, Jul 7 2016

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 4 by sheriffbot@chromium.org, Jul 7 2016

Labels: Pri-1
Cc: -wangxianzhu@chromium.org
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 9 2016

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

commit a0ccb0df60af245b02a405fef95822b658f10381
Author: wangxianzhu <wangxianzhu@chromium.org>
Date: Sat Jul 09 00:47:41 2016

Fix crash when removing contents from a document having multiple bodies

Removed the early return under condition 'isBody()' from
LayoutObjectChildList::invalidatePaintOnRemoval() to ensure the
painting layer and body object are invalidated.

BTW fixed issues of DisplayItemClient aliveness checking which made
it not actually work for subsequences. (If we didn't have the issues,
we should have caught this bug through aliveness-checking.)

Still not sure if we could have more reduced test. For the test, removing
anything from the test would make the test not reproducing the bug. A
normal removal of <body> couldn't reproduce the bug because we will
invalidate the painting layer in other paths (e.g. layout triggered
invalidation, etc.).

BUG= 626182 

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

[add] https://crrev.com/a0ccb0df60af245b02a405fef95822b658f10381/third_party/WebKit/LayoutTests/paint/invalidation/multiple-body-remove-selection-crash-expected.txt
[add] https://crrev.com/a0ccb0df60af245b02a405fef95822b658f10381/third_party/WebKit/LayoutTests/paint/invalidation/multiple-body-remove-selection-crash.html
[modify] https://crrev.com/a0ccb0df60af245b02a405fef95822b658f10381/third_party/WebKit/Source/core/frame/FrameView.cpp
[modify] https://crrev.com/a0ccb0df60af245b02a405fef95822b658f10381/third_party/WebKit/Source/core/layout/LayoutObjectChildList.cpp
[modify] https://crrev.com/a0ccb0df60af245b02a405fef95822b658f10381/third_party/WebKit/Source/core/layout/LayoutScrollbarPart.cpp
[modify] https://crrev.com/a0ccb0df60af245b02a405fef95822b658f10381/third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.cpp

Project Member

Comment 7 by ClusterFuzz, Jul 9 2016

ClusterFuzz has detected this issue as fixed in range 404506:404552.

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

Fuzzer: bj_broddelwerk
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Heap-use-after-free WRITE 4
Crash Address: 0x611000054510
Crash State:
  blink::PaintController::commitNewDisplayItems
  blink::GraphicsLayer::paint
  blink::FrameView::synchronizedPaintRecursively
  
Recommended Security Severity: High

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=392597:392661
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=404506:404552

Minimized Testcase (4.54 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94LkTZLa1AcE6eHlvNsBRT394Nz9XSLbP7UR46LIF0X9nvtbDRjjfmI5YhLZg5EKSQz-SRzXOS-kh__R-WfchwK6gGyCKzhapd6YkOS3W8DyYsdp12Jr9DuyC6Myezwh7qNK0XC66R4uoBGKnjB7RLf1RsL5Q?testcase_id=4585524119732224

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.
Labels: Merge-Request-53
Project Member

Comment 9 by ClusterFuzz, Jul 9 2016

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase is verified as fixed, closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 10 by sheriffbot@chromium.org, Jul 9 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Status: Available (was: Verified)
Project Member

Comment 12 by ClusterFuzz, Jul 12 2016

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

Fuzzer: mbarbella_js_mutation_layout
Job Type: linux_asan_chrome_v8
Platform Id: linux

Crash Type: Heap-use-after-free WRITE 4
Crash Address: 0x61100002d410
Crash State:
  blink::PaintController::commitNewDisplayItems
  blink::GraphicsLayer::paint
  blink::FrameView::synchronizedPaintRecursively
  
Recommended Security Severity: High

Regressed: V8: r37301:37323

Minimized Testcase (0.55 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv97Sj2Go_MgzK4xDO9EPvPktFyqcg53gOAQUpfm_QRkiX1FQHxBqg5KhCv-dBLvhIVLf7GRHuMYFCUv3EmyDXP4fhBRgweaukdcolvRMrh5pgkS9E_yHcG5HqcpvjtywibuL8ddnyAChZ2BKqV_qT12kK_nIog?testcase_id=5249114501808128
<script>
;
document.addEventListener('selectionchange', function() {
  getSelection().deleteFromDocument();
});
onload = function() {
  var __v_0 = document.body; 
  __v_0.outerHTML = '';
  __v_0.className = 'new-body';
  document.documentElement.insertBefore(__v_0, document.head.nextSibling);
  document.execCommand("SelectAll");
  document.getElementsByTagName('body')[1].outerHTML = '';
  getSelection().getRangeAt(0).extractContents();
};
  </script>
  <style>
   * { -webkit-appearance: radio }
.new-body { backface-visibility: hidden }
  </style>
 </head>



Filer: mmoroz

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
Status: Verified (was: Available)
Project Member

Comment 14 by ClusterFuzz, Jul 12 2016

ClusterFuzz has detected this issue as fixed in range 37616:37628.

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

Fuzzer: mbarbella_js_mutation_layout
Job Type: linux_asan_chrome_v8
Platform Id: linux

Crash Type: Heap-use-after-free WRITE 4
Crash Address: 0x61100002d410
Crash State:
  blink::PaintController::commitNewDisplayItems
  blink::GraphicsLayer::paint
  blink::FrameView::synchronizedPaintRecursively
  
Recommended Security Severity: High

Regressed: V8: r37301:37323
Fixed: V8: r37616:37628

Minimized Testcase (0.55 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv97Sj2Go_MgzK4xDO9EPvPktFyqcg53gOAQUpfm_QRkiX1FQHxBqg5KhCv-dBLvhIVLf7GRHuMYFCUv3EmyDXP4fhBRgweaukdcolvRMrh5pgkS9E_yHcG5HqcpvjtywibuL8ddnyAChZ2BKqV_qT12kK_nIog?testcase_id=5249114501808128
<script>
;
document.addEventListener('selectionchange', function() {
  getSelection().deleteFromDocument();
});
onload = function() {
  var __v_0 = document.body; 
  __v_0.outerHTML = '';
  __v_0.className = 'new-body';
  document.documentElement.insertBefore(__v_0, document.head.nextSibling);
  document.execCommand("SelectAll");
  document.getElementsByTagName('body')[1].outerHTML = '';
  getSelection().getRangeAt(0).extractContents();
};
  </script>
  <style>
   * { -webkit-appearance: radio }
.new-body { backface-visibility: hidden }
  </style>
 </head>



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.
Status: Assigned (was: Verified)
Marking security bug 'Fixed' and 'Verified' before it's merged seems to cause the bug to be overlooked for merging. Why can't we allow marking Fixed after merging? I think we should not mark it Fixed before it's actually fixed in the target milestone.

Comment 16 by aarya@google.com, Jul 12 2016

Security bugs are tracked differently, we track merges by individual merge flags and we need to know if the fix went in which is by the status Fixed. This ensures we can also triage bugs faster that need merges (or where developer forgets to put merge flags).
Status: Verified (was: Assigned)
OK. Thanks for explanation. Then I suppose marking it Fixed won't cause us to overlook merging.
Approving merge to M53 branch 2785 based on comment #14. Please merge ASAP. Thank you.
Labels: -Merge-Request-53 Merge-Approved-53
Project Member

Comment 20 by bugdroid1@chromium.org, Jul 14 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/869a702ec92cfd2550ebb8c7ef10c247c1889516

commit 869a702ec92cfd2550ebb8c7ef10c247c1889516
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Thu Jul 14 16:34:23 2016

Fix crash when removing contents from a document having multiple bodies

Removed the early return under condition 'isBody()' from
LayoutObjectChildList::invalidatePaintOnRemoval() to ensure the
painting layer and body object are invalidated.

BTW fixed issues of DisplayItemClient aliveness checking which made
it not actually work for subsequences. (If we didn't have the issues,
we should have caught this bug through aliveness-checking.)

Still not sure if we could have more reduced test. For the test, removing
anything from the test would make the test not reproducing the bug. A
normal removal of <body> couldn't reproduce the bug because we will
invalidate the painting layer in other paths (e.g. layout triggered
invalidation, etc.).

BUG= 626182 

Review URL: https://codereview.chromium.org/2150933002 .

Review-Url: https://codereview.chromium.org/2133603002
Cr-Original-Commit-Position: refs/heads/master@{#404552}
Cr-Commit-Position: refs/branch-heads/2785@{#112}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[add] https://crrev.com/869a702ec92cfd2550ebb8c7ef10c247c1889516/third_party/WebKit/LayoutTests/paint/invalidation/multiple-body-remove-selection-crash-expected.txt
[add] https://crrev.com/869a702ec92cfd2550ebb8c7ef10c247c1889516/third_party/WebKit/LayoutTests/paint/invalidation/multiple-body-remove-selection-crash.html
[modify] https://crrev.com/869a702ec92cfd2550ebb8c7ef10c247c1889516/third_party/WebKit/Source/core/frame/FrameView.cpp
[modify] https://crrev.com/869a702ec92cfd2550ebb8c7ef10c247c1889516/third_party/WebKit/Source/core/layout/LayoutObjectChildList.cpp
[modify] https://crrev.com/869a702ec92cfd2550ebb8c7ef10c247c1889516/third_party/WebKit/Source/core/layout/LayoutScrollbarPart.cpp
[modify] https://crrev.com/869a702ec92cfd2550ebb8c7ef10c247c1889516/third_party/WebKit/Source/platform/graphics/paint/DisplayItemClient.cpp

Labels: -ReleaseBlock-Beta
Project Member

Comment 22 by sheriffbot@chromium.org, Oct 19 2016

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