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

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment

Heap-use-after-free in blink::PODIntervalTree<int,blink::FloatingObject

Project Member Reported by ClusterFuzz, Sep 1 2014

Issue description

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

Fuzzer: Inferno_twister
Job Type: Windows_asan_chrome

Crash Type: Heap-use-after-free READ 4
Crash Address: 0x0cb3777c
Crash State:
  blink::PODIntervalTree<int,blink::FloatingObject
  blink::FloatingObjects::logicalLeftOffset
  blink::RenderBlockFlow::logicalLeftSelectionOffset
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=windows_asan_chrome&range=289356:289499

Minimized Testcase (0.65 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97duTvi45Cx8sLUjYHRWXEoPDfrOPxAdlWADmwthIS37CyZRAS2jXK9Q64iW4dHkQXCDs_OIWkDVFLFKb-5Ah_CQ0uIn4DsfeU8sTjGHlEGTm0f7Z1HY0BKhEqWezE69g5X69z3MNBz-rcCfvZyoz-5mQyuIQ

Filer: inferno
 
Labels: Cr-Blink-Rendering
Owner: bjone...@adobe.com
Status: Assigned
Project Member

Comment 2 by ClusterFuzz, Sep 1 2014

Labels: M-38 Pri-1
Project Member

Comment 3 by ClusterFuzz, Sep 1 2014

Labels: ReleaseBlock-Stable

Comment 4 by bjone...@adobe.com, Sep 3 2014

This doesn't repro on Mac. It does repro on Linux.
linux-asan-dump.txt
29.9 KB View Download

Comment 5 by bjone...@adobe.com, Sep 3 2014

Wow. The length of the name of the input file matters. If I use a shorter name, it doesn't crash. But if I use a longer name, it does.

Comment 6 by bjone...@adobe.com, Sep 3 2014

Here's a minimized case. I didn't try to completely minimize the file name length, but this one is long enough to trigger the crash, at least on my system. Perhaps if you have a deeper directory structure on another system, it might work with a shorter filename (or may need a longer file name) I haven't played around with file names on the Mac to see if perhaps there's a different file name length that triggers the crash there.
minimize-float-crash-abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrstuvwxyz.html
323 bytes View Download
Labels: -Security_Impact-Beta -M-38 -ReleaseBlock-Stable Security_Impact-Stable
I think the bug should be old.

Comment 8 by bjone...@adobe.com, Sep 3 2014

I don't understand. What do you mean?
Project Member

Comment 9 by ClusterFuzz, Sep 3 2014

Labels: M-37
Sorry i meant the regression range looks wrong, so i was fixing the impacts label tag.
@inferno: I wonder if the revision range looks wrong because the UAF is actually a symptom of the real problem. It is very weird that the file name length matters in reproducing this issue. The only way I can figure out how that would happen is if something much before the layout code is handling memory incorrectly, since this code never does anything that has anything to do with the URL of the document. That  would explain why even though this code hasn't changed in a long time, the bug just popped up now.
Regression range can be wrong for many reasons, sometimes repros reproduce differently due to the settimeout values in repros. I would say just try debugging the crashing repro on trunk.
But why would the length of the filename matter in if it repros or not? That's the part that doesn't make any sense to me.
Project Member

Comment 14 by ClusterFuzz, Sep 13 2014

Labels: Nag
bjonesbe@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz

Comment 15 by bjone...@adobe.com, Sep 17 2014

@inferno:
I won't have time to look into this for at least 3 weeks.

However, the fact that the filename length matters in reproing this makes me believe that the actual problem is much before the rendering code: the rendering code has nothing to do with filenames. There are also no timeouts in this test case at all, so that doesn't have anything to do with it repro'ing. (And I know almost nothing about the code in Chromium that leads up to the rendering code) I don't know who would be the right person to look into it, though.
Cc: bjone...@adobe.com
Owner: ----
Status: Available
Removing owner so that this gets back into our queue for triage.

bjonesbe: Thanks for the update. If you are able to pick this up before someone else does, feel free to set yourself as the owner again.

Comment 17 by jww@chromium.org, Sep 19 2014

Owner: jchaffraix@chromium.org
jchaffraix@, would you mind taking a look at this and confirming bjonesbe's suspicions?
Cc: le...@chromium.org robho...@gmail.com
Unfortunately I don't have much bandwidth for bugs I didn't cause lately. Not sure when it will be better so adding some rendering folks.
Project Member

Comment 19 by ClusterFuzz, Sep 29 2014

Labels: -M-37 M-38

Comment 20 by robho...@gmail.com, Sep 30 2014

Same thing for me: it crashes on an ASAN build as long as the filename is extra long.


Project Member

Comment 21 by ClusterFuzz, Oct 2 2014

jchaffraix@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Status: Started
Owner: robho...@gmail.com
Cc: hmul...@adobe.com
Ignoring the file-length issue, I believe this crash stems from https://codereview.chromium.org/131133004.

That CL is marking a shape as dirty again and deleting floats in its renderer's descendants while still in the process of making it non-dirty in computedShape().

It tries to prevent that by checking isInPerformLayout - but that doesn't work in this test case.

I think the solution is to check isShapeDirty() instead but there is one flaky test case that clouds the issue for me. I might discuss it on IRC later.

Cc: -hmul...@adobe.com hansmuller@chromium.org
@robhogan Hans is no longer at Adobe. I've cc'd his new address.
Thanks Bem. I will take a look at this tomorrow.

I'm not on IRC. 


@hansmuller: I think@robhogan has it covered, he posted a patch at https://codereview.chromium.org/635533003/ in case you want to take a look. (I haven't had a chance to review it myself yet)
Project Member

Comment 28 by bugdroid1@chromium.org, Oct 9 2014

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=183485

------------------------------------------------------------------
r183485 | robhogan@gmail.com | 2014-10-09T18:54:43.821159Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderBox.cpp?r1=183485&r2=183484&pathrev=183485
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/resources/square.png?r1=183485&r2=183484&pathrev=183485
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/shapes/ShapeOutsideInfo.cpp?r1=183485&r2=183484&pathrev=183485
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/shapes/shape-outside-floats/shape-outside-image-shape-margin-expected.html?r1=183485&r2=183484&pathrev=183485
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/shapes/shape-outside-floats/shape-outside-dynamic-shape-image-threshold-expected.html?r1=183485&r2=183484&pathrev=183485
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/shapes/shape-outside-floats/shape-outside-image-shape-margin.html?r1=183485&r2=183484&pathrev=183485
   D http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/shapes/shape-outside-floats/shape-outside-dynamic-shape-image-threshold.html?r1=183485&r2=183484&pathrev=183485
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/shapes/crash-caused-by-dirtying-a-shape-while-computing-it-requires-a-long-filename-to-crash.html?r1=183485&r2=183484&pathrev=183485
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/shapes/ShapeOutsideInfo.h?r1=183485&r2=183484&pathrev=183485
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/FlakyTests?r1=183485&r2=183484&pathrev=183485
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/shapes/crash-caused-by-dirtying-a-shape-while-computing-it-requires-a-long-filename-to-crash-expected.txt?r1=183485&r2=183484&pathrev=183485
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/shapes?r1=183485&r2=183484&pathrev=183485
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/shapes/shape-outside-svg-image-shape-margin-expected.html?r1=183485&r2=183484&pathrev=183485
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/shapes/shape-outside-svg-image-shape-margin.html?r1=183485&r2=183484&pathrev=183485
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/shapes/shape-outside-image-shape-margin-expected.html?r1=183485&r2=183484&pathrev=183485
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/shapes/shape-outside-dynamic-shape-image-threshold-expected.html?r1=183485&r2=183484&pathrev=183485
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/shapes/shape-outside-image-shape-margin.html?r1=183485&r2=183484&pathrev=183485
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/shapes/shape-outside-dynamic-shape-image-threshold.html?r1=183485&r2=183484&pathrev=183485

Don't delete nodes in the float interval tree while traversing it.

When traversing the floated objects' interval tree in searchForOverlapsFrom() we will crash if 
we delete ourselves from the tree - this can happen if computedShape() is called, 
the shape needs to be computed, and we end up removing ourselves from ancestors' floating 
object lists in markShapeDependentObjectsForLayout() via RenderBox::imageChanged().

What this boils down to is ensuring that we don't mark a shape as dirty while we are in 
the middle of computing it. When we compute a shape RenderBox::imageChanged() will get
called. This function needs to dirty the shape when we are here because
of an asynchronous image; if we're here because we are computing the shape deltas for the float
there is no need to dirty ourselves again.

I explored a few options for determining if we are computing the shape deltas for the
float. Unfortunately we can't just use isShapeDirty() because if a float is added
to the render tree markShapeOutsideDependentsForLayout() will not mark any ancestors
for layout until the first layout has happened. This means we could arrive in
RenderBox::imageChanged()
with the shape marked as dirty but still needing to re-layout its ancestors.

This is a regression from https://codereview.chromium.org/131133004.

Note that I've moved a couple of the shape-outside test to the http/ folder. They use
url() for the image loads; which doesn't work when running on file:// urls. The image
loads always fail with 'Image from origin 'file://' has been blocked from loading 
by Cross-Origin Resource Sharing policy: Received an invalid response. Origin 'null'
is therefore not allowed access.'

BUG= 409508 

Review URL: https://codereview.chromium.org/635533003
-----------------------------------------------------------------
Status: Fixed
Project Member

Comment 30 by ClusterFuzz, Oct 9 2014

Labels: -Restrict-View-SecurityTeam Merge-Triage M-39 Restrict-View-SecurityNotify
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz
Labels: -Nag -M-38 -Merge-Triage Merge-Requested
Labels: -Merge-Requested Merge-Approved
merge approved for m39 branch 2171.
Cc: amineer@chromium.org
Dev/Bug owner, please merge to M-39 branch 2171 asap. We need all these security fixes to go into the first stable.
I can't merge! :/


Project Member

Comment 35 by bugdroid1@chromium.org, Nov 3 2014

Labels: -Merge-Approved merge-merged-2171
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=184800

------------------------------------------------------------------
r184800 | amineer@chromium.org | 2014-11-03T20:43:15.970987Z

Changed paths:
   A http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/fast/shapes/crash-caused-by-dirtying-a-shape-while-computing-it-requires-a-long-filename-to-crash-expected.txt?r1=184800&r2=184799&pathrev=184800
   M http://src.chromium.org/viewvc/blink/branches/chromium/2171/Source/core/rendering/shapes/ShapeOutsideInfo.cpp?r1=184800&r2=184799&pathrev=184800
   D http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/fast/shapes/shape-outside-floats/shape-outside-image-shape-margin-expected.html?r1=184800&r2=184799&pathrev=184800
   D http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/fast/shapes/shape-outside-floats/shape-outside-dynamic-shape-image-threshold-expected.html?r1=184800&r2=184799&pathrev=184800
   D http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/fast/shapes/shape-outside-floats/shape-outside-image-shape-margin.html?r1=184800&r2=184799&pathrev=184800
   A http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/fast/shapes/crash-caused-by-dirtying-a-shape-while-computing-it-requires-a-long-filename-to-crash.html?r1=184800&r2=184799&pathrev=184800
   D http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/fast/shapes/shape-outside-floats/shape-outside-dynamic-shape-image-threshold.html?r1=184800&r2=184799&pathrev=184800
   M http://src.chromium.org/viewvc/blink/branches/chromium/2171/Source/core/rendering/shapes/ShapeOutsideInfo.h?r1=184800&r2=184799&pathrev=184800
   M http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/FlakyTests?r1=184800&r2=184799&pathrev=184800
   A http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/http/tests/resources/square.png?r1=184800&r2=184799&pathrev=184800
   M http://src.chromium.org/viewvc/blink/branches/chromium/2171/Source/core/rendering/RenderBox.cpp?r1=184800&r2=184799&pathrev=184800

Merge 183485 "Don't delete nodes in the float interval tree whil..."

Merging to branch 2171
BUG= 409508 

> Don't delete nodes in the float interval tree while traversing it.
> 
> When traversing the floated objects' interval tree in searchForOverlapsFrom() we will crash if 
> we delete ourselves from the tree - this can happen if computedShape() is called, 
> the shape needs to be computed, and we end up removing ourselves from ancestors' floating 
> object lists in markShapeDependentObjectsForLayout() via RenderBox::imageChanged().
> 
> What this boils down to is ensuring that we don't mark a shape as dirty while we are in 
> the middle of computing it. When we compute a shape RenderBox::imageChanged() will get
> called. This function needs to dirty the shape when we are here because
> of an asynchronous image; if we're here because we are computing the shape deltas for the float
> there is no need to dirty ourselves again.
> 
> I explored a few options for determining if we are computing the shape deltas for the
> float. Unfortunately we can't just use isShapeDirty() because if a float is added
> to the render tree markShapeOutsideDependentsForLayout() will not mark any ancestors
> for layout until the first layout has happened. This means we could arrive in
> RenderBox::imageChanged()
> with the shape marked as dirty but still needing to re-layout its ancestors.
> 
> This is a regression from https://codereview.chromium.org/131133004.
> 
> Note that I've moved a couple of the shape-outside test to the http/ folder. They use
> url() for the image loads; which doesn't work when running on file:// urls. The image
> loads always fail with 'Image from origin 'file://' has been blocked from loading 
> by Cross-Origin Resource Sharing policy: Received an invalid response. Origin 'null'
> is therefore not allowed access.'
> 
> BUG= 409508 
> 
> Review URL: https://codereview.chromium.org/635533003

TBR=robhogan@gmail.com

Review URL: https://codereview.chromium.org/698093002
-----------------------------------------------------------------
Labels: Release-0-M39
Thanks guys - after a bit of reading up I reckon I can give this a shot on  bug 423891  too - as it's related. Could you approve that for merge too?
Labels: -Cr-Blink-Rendering Cr-Blink-Layout
Migrate from Cr-Blink-Rendering to Cr-Blink-Layout
Project Member

Comment 39 by ClusterFuzz, Jan 15 2015

Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member

Comment 40 by sheriffbot@chromium.org, Oct 1 2016

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

Comment 41 by sheriffbot@chromium.org, Oct 2 2016

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
Labels: allpublic

Sign in to add a comment