crash in mozilla::layers::Layer::SetVisibleRegion

RESOLVED FIXED in Firefox 51

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: esthermonchari, Assigned: sotaro)

Tracking

({crash, csectype-uaf, sec-high})

52 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox-esr45 disabled, firefox50 wontfix, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+], crash signature)

Attachments

(2 attachments, 6 obsolete attachments)

Reporter

Description

3 years ago
This bug was filed from the Socorro interface and its 
report https://crash-stats.mozilla.com/report/index/7cd7f98f-9ac0-40b0-b462-20a7f2160922

This was noted while looking at nightly crash stats.As far as I found out the crash associated multiple users. The crash started on 2016-09-21 23:19:11  with build id 20160920030429 and is still continuing.

The crash is mostly on firefox 52.0a1 version ,windows 10 OS

need info on Carsten since it appears he worked in that area and can maybe help figure out what could be causing the crash.
Reporter

Updated

3 years ago
Flags: needinfo?(mozillamarcia.knous)
Flags: needinfo?(cbook)
Keywords: crash
Adding crash signature field, and switching over to what I believe to be the correct component. There is also one Mac crash so changing Platform info.

adding ni on :mattwoodrow in case he has some ideas.
Status: UNCONFIRMED → NEW
Crash Signature: [@ mozilla::layers::Layer::SetVisibleRegion]
Component: Untriaged → Graphics: Layers
Ever confirmed: true
Flags: needinfo?(mozillamarcia.knous)
Flags: needinfo?(cbook)
OS: Windows 10 → All
Product: Firefox → Core
Hardware: Unspecified → All
Reporter

Updated

3 years ago
Duplicate of this bug: 1307454
Assignee

Updated

3 years ago
Assignee: nobody → sotaro.ikeda.g
Assignee

Comment 3

3 years ago
Almost all crashes happened under LayerTransactionParent::RecvUpdate(). From it, LayerManagerComposite might be destroyed already.
Assignee

Comment 4

3 years ago
When the crash happens, Layer should be still alive since ShadowLayerParent holds a ref count of the Layer.
Assignee

Comment 5

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> Almost all crashes happened under LayerTransactionParent::RecvUpdate(). From
> it, LayerManagerComposite might be destroyed already.

At first LayerManagerComposite of the Layer should be always valid since LayerTransactionParent holds a ref count of the LayerManagerComposite.

The Layer's reference of LayerManagerComposite is updated by CompositorBridgeParent::RecvAdoptChild(). It seems related to the crash.
Assignee

Comment 6

3 years ago
If LayerTransactionParent and Layer holds same ref to LayerManagerComposite, the crash should not happen. Then it seems to mean that they hold different refs of LayerManagerComposite.
Assignee

Comment 7

3 years ago
Assignee

Comment 8

3 years ago
Attachment #8804187 - Attachment is obsolete: true
Assignee

Comment 9

3 years ago
Attachment #8804188 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8804189 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8804189 - Attachment is obsolete: false
Assignee

Updated

3 years ago
Attachment #8804189 - Flags: review?(matt.woodrow)
Layer::mManager is a raw pointer, so if trying to access it is causing a crash, then we have a real problem.

From looking at the reports, it appears that at least in some cases [1] mManager is non-null, so we're trying to access a dangling pointer.

That sounds like a real security bug, so just early returning here probably isn't sufficient.

We really need to make sure LayerManagers outlive all their child layers (and maybe come up with a safer way to ensure this doesn't regress?).



[1] https://crash-stats.mozilla.com/report/index/360860c3-8d06-42c8-867d-f439d2161020
Group: core-security
Assignee

Updated

3 years ago
Attachment #8804189 - Flags: review?(matt.woodrow)
Assignee

Comment 11

3 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> 
> We really need to make sure LayerManagers outlive all their child layers
> (and maybe come up with a safer way to ensure this doesn't regress?).
> 

Thanks for the review. I am going to try to address the comment.
Group: core-security → layout-core-security
Any progress on this, Sotaro?
Flags: needinfo?(sotaro.ikeda.g)
I think the right thing to do here is to make all the raw Layer*/LayerManager* members hold a strong ref using RefPtr<>, and then make sure we always break any cycles explicitly.

A few I can think of right now:

LayerManager::mRoot
Layer::mManager
Layer::mNextSibling
ContainerLayer::mFirstChild.
Assignee

Comment 14

2 years ago
From the source, direct cause of the crash might be related to the following.

CompositorBridgeParent::RecvAdoptChild() updates HostLayerManager of Layers and LayerTransactionParent. but it does not update a pointer of LayerManager of Layers. Then the pointer of LayerManager continue to refer to old HostLayerManager.
Flags: needinfo?(sotaro.ikeda.g)
Assignee

Comment 15

2 years ago
Attachment #8804189 - Attachment is obsolete: true
Assignee

Comment 16

2 years ago
:mattwoodrow, how about attachment 8823984 [details] [diff] [review]? It seems enough to address the crash.
Flags: needinfo?(matt.woodrow)
Can we just update the pointer on the underlying Layer instead?
Flags: needinfo?(matt.woodrow)
Assignee

Comment 18

2 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> Can we just update the pointer on the underlying Layer instead?

I am going to try it.
Assignee

Comment 19

2 years ago
I rechecked the source, Layer::mManager is updated manually like [1] except RefLayerComposite.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/PaintedLayerComposite.cpp#83
Assignee

Comment 20

2 years ago
Attachment #8823984 - Attachment is obsolete: true
Assignee

Comment 21

2 years ago
We also need to ensure LayerManager's lifetime by modifying CompositorBridgeParent::StopAndClearResources().
Assignee

Comment 22

2 years ago
Attachment #8824286 - Attachment is obsolete: true
Assignee

Comment 23

2 years ago
Comment on attachment 8824287 [details] [diff] [review]
patch - Update LayerManager correctly

carry = :mattwoodrow.
Attachment #8824287 - Flags: review+
Assignee

Comment 24

2 years ago
attachment 8824287 [details] [diff] [review] caused tryserver tests failure, I am going to address it.
Assignee

Updated

2 years ago
Attachment #8824286 - Attachment is obsolete: false
Assignee

Updated

2 years ago
Attachment #8824287 - Attachment is obsolete: true
Assignee

Comment 25

2 years ago
There might be a case that the following in CompositorBridgeParent::RecvAdoptChild() could not update LayerManager pointer of all Layers of LayerTransactionParent if there is a Layer that does not exist under the mRoot;

> sIndirectLayerTrees[child].mRoot->AsHostLayer()->SetLayerManager(static_cast<HostLayerManager*>(mLayerManager.get()));

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeParent.cpp#1527
Assignee

Updated

2 years ago
Attachment #8825308 - Flags: review?(matt.woodrow)
Attachment #8825308 - Flags: review?(matt.woodrow) → review+
Assignee

Updated

2 years ago
Attachment #8824286 - Attachment is obsolete: true
Assignee

Comment 28

2 years ago
Comment on attachment 8825308 [details] [diff] [review]
patch - Update LayerManager pointer of all Layers of LayerTransactionParent

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

 It seems difficult to create the exploit based on this patch.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

  No. The patch does not have a comment of saying that the patch fixes a security problem.

> Which older supported branches are affected by this flaw?

It causes the problem when e10s is enabled. e10s seems to be enabled since Firefox 48.

> If not all supported branches, which bug introduced the flaw?

  Bug 918634 introduced the flaw. It caused the problem only when e10s is enabled.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

  No yet. The patch depends on Bug 1323539. A patch becomes a bit different. Creating a patch for the branches is not difficult.

> How likely is this patch to cause regressions; how much testing does it need?

  This fix is low risk of regression.
  I tested locally and tryserver test passed.
Attachment #8825308 - Flags: sec-approval?
Comment on attachment 8825308 [details] [diff] [review]
patch - Update LayerManager pointer of all Layers of LayerTransactionParent

sec-approval+ for trunk.
we'll want patches nominated for Beta and Aurora as soon as possible so we can get it in the final beta that's going to be made real soon now.
Attachment #8825308 - Flags: sec-approval? → sec-approval+
Hi :sotaro,
Is this bug worth uplifting to Beta51 & Aurora52 given it's a sec-high?
Flags: needinfo?(sotaro.ikeda.g)
Assignee

Comment 32

2 years ago
Patch for aurora.
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8826432 - Flags: review+
Assignee

Updated

2 years ago
Attachment #8826432 - Attachment description: patch for aurora - Update LayerManager pointer of all Layers of LayerTransactionParent → patch for aurora and beta - Update LayerManager pointer of all Layers of LayerTransactionParent
Assignee

Comment 33

2 years ago
(In reply to Gerry Chang [:gchang] from comment #31)
> Hi :sotaro,
> Is this bug worth uplifting to Beta51 & Aurora52 given it's a sec-high?

Yes. It worth uplifting.
Assignee

Comment 34

2 years ago
Comment on attachment 8826432 [details] [diff] [review]
patch for aurora and beta - Update LayerManager pointer of all Layers of LayerTransactionParent

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Moving tab between windows could cause crash.
[Is this code covered by automated tests?]: The touched code is functionally tested.
[Has the fix been verified in Nightly?]: Not yet. The patch is in inbound now.
[Needs manual test from QE? If yes, steps to reproduce]: No, we do not know  the explicit STR to reproduce the crash.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]:It actually does not change how LayerTransactionParent works in normal situation.
[String changes made/needed]: none
Attachment #8826432 - Flags: approval-mozilla-beta?
Attachment #8826432 - Flags: approval-mozilla-aurora?
Comment on attachment 8826432 [details] [diff] [review]
patch for aurora and beta - Update LayerManager pointer of all Layers of LayerTransactionParent

Fix for sec-high issue, let's try to get this into Monday's 51RC buld.
Attachment #8826432 - Flags: approval-mozilla-beta?
Attachment #8826432 - Flags: approval-mozilla-beta+
Attachment #8826432 - Flags: approval-mozilla-aurora?
Attachment #8826432 - Flags: approval-mozilla-aurora+
Track 51+ as sec-high.
QA Contact: kjozwiak
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Group: layout-core-security → core-security-release
This is going to be hard to verify as there's no STR nor any attached test cases as mentioned in comment#34. However, looking at the signature under crash stats, it seems like this has been fixed. I'm not seeing any crashes from 51RC1 or 51RC2 which were created after the patch landed.

Liz, do we usually have enough people using RC1/RC2 builds that we can confidently say that this has beed fixed without actually testing it ourselves?
Flags: needinfo?(lhenry)
Most of the beta 51 population is currently on the RC build right now. I see a few crashes for beta 14 but not for the RC builds, so that's a good sign.
Flags: needinfo?(lhenry)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.