1072871 - IPC: heap-use-after-free crash [@mozilla::gfx::DrawTargetCG::CopySurface]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: faulty-CGBlt_copyBytes.txt

This happened while opening http://mozilla.github.io/webrtc-landing/pc_test.html in a build with '--enable-ipc-fuzzer' enabled.

Comment 1

[Andrew McCreight [:mccr8]]

Milan is there somebody who can look at this?  Thanks.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

:cdiehl, to reproduce this, go to the page from comment 0 and hit start test?  How long did you wait before it crashed?  Does it crash all the time?  Just trying to reproduce locally.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Nical, looks like we try to use the shared/tile memory on the client side, after the texture host has gone away, releasing that memory?

Comment 4

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] from comment #3)
> Nical, looks like we try to use the shared/tile memory on the client side,
> after the texture host has gone away, releasing that memory?

Most likely, yes. Is the fuzzer only randomly changing the value of message arguments, or does it also add messages?
If it can cause a PTexture actor to artificially send the __delete__ or RemoveTexture messages there is nothing we can do about the client process trying to access shared memory that it just told the host to destroy.
If it can't do this then I am not sure how we can get into this situation.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

I don't think we add messages, I believe we just change the integer parameters +1/-1 from what I've seen here and in the past. I imagine we'd get more insight if we were paying attention to the results of EndTransaction() instead of ignoring it in places...

Christoph, any magic to reproducing this? (see comment 2)

Comment 6

[Christoph Diehl [:posidron]]

Sorry, for getting back so late this bug I was working on a project.

That's correct Milan, we are not adding messages, we only intercept them and mutate their values inside.

Please follow those steps to reproduce the issue in the best way currently possible:
https://bugzilla.mozilla.org/show_bug.cgi?id=1078105#c1

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Can the references surfaces disappear?

This is really more for feedback, since I can't reproduce locally, but can it be that the surfaces disappear under us unless we hold on to them?

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 8535824 [details] [diff] [review]
Can the references surfaces disappear?

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

r+ without the last 2 lines.

::: gfx/layers/client/TextureClient.cpp
@@ +523,5 @@
>      return false;
>    }
>  
> +  RefPtr<DrawTarget> destinationTarget = aTarget->BorrowDrawTarget();
> +  RefPtr<DrawTarget> sourceTarget = BorrowDrawTarget();

This is good! We shouldn't have a naked pointer to a ref counted object unless we can prove that something else is holding the reference on our behalf. It's not the case here.

@@ +530,5 @@
>                                   aRect ? *aRect : gfx::IntRect(gfx::IntPoint(0, 0), GetSize()),
>                                   aPoint ? *aPoint : gfx::IntPoint(0, 0));
>    source = nullptr;
> +  sourceTarget = nullptr;
> +  destinationTarget = nullptr;

We don't need these two lines, RefPtr will auto Release() here since we're going out of scope part of the return statement.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Stop the surfaces from disappearing. Carry r=benwa

Asking for a second review, because this change was a part of bug 1023350.  :nical, there are comments with that patch about "don't hold a reference once it's unlocked", and this patch may be going exactly against that, except probably not, but...

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8536659 [details] [diff] [review]
Stop the surfaces from disappearing. Carry r=benwa

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

It is UAF, but I don't know how easy it would be to guess the previous draw target location.

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

Points out reference counting issue, which could lead to a conclusion that there is a use after free, but not how to get to that scenario.

Which older supported branches are affected by this flaw?
This was introduced in 33, so 34 and higher at this point.

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

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

How likely is this patch to cause regressions; how much testing does it need?
Low level of risk.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4fa34dd09d0a

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8536659 [details] [diff] [review]
Stop the surfaces from disappearing. Carry r=benwa

sec-approval+. Please create and nominate Aurora and Beta patches as well.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Al Billings [:abillings] from comment #12)
> Comment on attachment 8536659 [details] [diff] [review]
> Stop the surfaces from disappearing. Carry r=benwa
> 
> sec-approval+. Please create and nominate Aurora and Beta patches as well.

The same patch applies to Aurora and Beta, but I'd like for this to land and spend a few days on the trunk before we uplift it.  I'll nominate it now and you can approve it once that happens?

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8536659 [details] [diff] [review]
Stop the surfaces from disappearing. Carry r=benwa

Approval Request Comment
[Feature/regressing bug #]: 1023350
[User impact if declined]: Use after free crash
[Describe test coverage new/current, TBPL]:
[Risks and why]: Low
[String/UUID change made/needed]: None

Comment 15

[Al Billings [:abillings - ex-MoCo]]

(In reply to Milan Sreckovic [:milan] from comment #13)
> The same patch applies to Aurora and Beta, but I'd like for this to land and
> spend a few days on the trunk before we uplift it.  I'll nominate it now and
> you can approve it once that happens?

Sure. Please needinfo? when the time comes.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd2d1db4056

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bd2d1db4056

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive-by, will approve this for Beta 8 on Mon Dec 29th.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d887c4a25f8
https://hg.mozilla.org/releases/mozilla-beta/rev/fe0b12352606

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/d322008da1aa