Closed Bug 1082986 Opened 5 years ago Closed 5 years ago

Exploitable crash in mozilla::layers::ImageBridgeParent::SendFenceHandleIfPresent

Categories

(Core :: Graphics: Layers, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox33 --- unaffected
firefox34 --- unaffected
firefox35 + verified
firefox36 + fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.2 --- fixed

People

(Reporter: bc, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

(4 keywords)

Crash Data

Attachments

(5 files, 1 obsolete file)

Bughunter has detected what appears to be an exploitable crash on Windows at least. It also crashes OSX 10.8 at least. The breakpad exploitable tool rates this as high. Microsoft's exploitable rates this as exploitable user mode dep. I have a semi saved version of the original NSFW page that reproduces but neither it nor the original url reproduce 100% of the time. I'll try to reduce. If not, I'll post the url and a zip of the saved version later. I'm filing now in case looking at the stacks helps identify the issue sooner.

See also bug 1081706
As you can see, this one is much worse:
Crash reason:  EXCEPTION_ACCESS_VIOLATION_EXEC
Crash address: 0xffffffffcccccccc
Component: IPC → Graphics: Layers
Assignee: nobody → sotaro.ikeda.g
This code path is actually used only on b2g, the code could be disabled on platforms except b2g. But it does not fix the problem on b2g.
In current code, CompositableHost/TextureHost does not own Compositor's reference pointer. It could cause stale pointer's dereference.

nial, do you know the reason of it?
Flags: needinfo?(nical.bugzilla)
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> In current code, CompositableHost/TextureHost does not own Compositor's
> reference pointer. It could cause stale pointer's dereference.

It just owns Composotor's raw pointer.
Let's just make it a RefPtr
Flags: needinfo?(nical.bugzilla)
Sotaro, what bug is this a regression from?
Bug 1067455 is a direct cause of the regression. Potential problem was added since Bug 825928 fix.
Thanks!

[Tracking Requested - why for this release]: sec-high.
Attachment #8505692 - Flags: review?(nical.bugzilla)
Comment on attachment 8505692 [details] [diff] [review]
patch - Use RefPtr to hold Compositor

Review of attachment 8505692 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/CompositingRenderTargetOGL.h
@@ +156,5 @@
>     */
>    void InitializeImpl();
>  
>    InitParams mInitParams;
> +  RefPtr<CompositorOGL> mCompositor;

nit: There is temporary a cycle between the compositor and the render target, each having a strong ref to the other. It's okay since the compositor's reference to the target is always cleared at the end of a frame, but it would be nice to add a comment, maybe in Compositor.h around SetRenderTarget to say that the reference must only be kept during the duration of the frame. Otherwise I am worried that we will eventually break this equilibrium by keeping the target after the frames or run into this every time we run some cycle detection tools like bjacob did a little while ago.
Attachment #8505692 - Flags: review?(nical.bugzilla) → review+
(In reply to Sotaro Ikeda [:sotaro] from comment #13)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3c90324964f7

I hope it fix the crash.
The crash was not very reproducible. I could not reliably detect it with the saved version of the page and the live version was only reproducible maybe once in a hundred attempts. Once this lands on mozilla-central, I will run a retest over all the workers and will do repeated attempts on windows 7 for at least 200 attempts. I'll let you know the results when it completes.
Thanks!
https://hg.mozilla.org/mozilla-central/rev/3c90324964f7

This should have had sec-approval before landing, BTW.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
We'll need to get the questions answered from the sec-approval? template before we can determine whether to take this on Aurora.
I could not reproduce this crash on Nightly on Win7 32bit after this fix landed.
Cool! Thanks :-)
Comment on attachment 8505692 [details] [diff] [review]
patch - Use RefPtr to hold Compositor

[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 tests included.
   One comment is added. It is not related to the security. 

Which older supported branches are affected by this flaw?
 - firefox35 is affected.

If not all supported branches, which bug introduced the flaw?
 - Bug 1067455 is a direct cause of the regression.
 - Potential problem was added since Bug 825928 fix.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
- The same patch seems to work for firefox35.
  create a patch for firefox35 is easy and low risk.

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.
  And comment 19 did confirmation.
Attachment #8505692 - Flags: sec-approval?
Comment on attachment 8505692 [details] [diff] [review]
patch - Use RefPtr to hold Compositor

Sounds good. Can we get an Aurora patch (or a nomination on the existing patch if it applies) so we can check this in there as well?
Attachment #8505692 - Flags: sec-approval? → sec-approval+
A patch for aurora is necessary. I am preparing it.
Comment on attachment 8505692 [details] [diff] [review]
patch - Use RefPtr to hold Compositor

Approval Request Comment
[Feature/regressing bug #]:Bug 1067455
[User impact if declined]:security risk. And chrome process crash.
[Describe test coverage new/current, TBPL]:locally tested.
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8505692 - Flags: approval-mozilla-aurora?
Attachment #8505692 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Sorry, patch for aurora attachment 8507265 [details] [diff] [review] was different from Comment 25. I uploaded just the master's patch.
Correct patch for aurora. Carry "r=nical".
Attachment #8507265 - Attachment is obsolete: true
Attachment #8508978 - Flags: review+
Hi Bob, can you verify that this has been fixed in Fx35? Thank you.
Flags: needinfo?(bob)
Matt, will do. I'm doing a quick test on Windows and OSX with one attempt per beta, aurora, nightly and a longer attempt on winxp, win7 32/64 on beta only where I'm loading the url 200 times each. I'll let you know later this evening how it goes.
Flags: needinfo?(bob)
I could not reproduce the crash with a Nightly built from today.
Marking verified based on comment 36.
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.