Closed Bug 1072871 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Graphics, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox34 --- wontfix
firefox35 + fixed
firefox36 + fixed
firefox37 + fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: posidron, Assigned: milan)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main35+][b2g-adv-main2.2+])

Attachments

(2 files, 1 obsolete file)

This happened while opening http://mozilla.github.io/webrtc-landing/pc_test.html in a build with '--enable-ipc-fuzzer' enabled.
Keywords: sec-high
Milan is there somebody who can look at this?  Thanks.
Flags: needinfo?(milan)
: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.
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?
Flags: needinfo?(milan) → needinfo?(nical.bugzilla)
(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.
Flags: needinfo?(nical.bugzilla)
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)
Flags: needinfo?(cdiehl)
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
Flags: needinfo?(cdiehl)
Group: gfx-core-security
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?
Attachment #8535824 - Flags: review?(bgirard)
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.
Attachment #8535824 - Flags: review?(bgirard) → review+
Assignee: nobody → milan
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...
Attachment #8535824 - Attachment is obsolete: true
Attachment #8536659 - Flags: review?(nical.bugzilla)
Attachment #8536659 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8536659 - Flags: sec-approval?
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.
Attachment #8536659 - Flags: sec-approval? → sec-approval+
(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 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
Attachment #8536659 - Flags: approval-mozilla-beta?
Attachment #8536659 - Flags: approval-mozilla-aurora?
(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.
https://hg.mozilla.org/mozilla-central/rev/2bd2d1db4056
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Triage drive-by, will approve this for Beta 8 on Mon Dec 29th.
Group: gfx-core-security
Attachment #8536659 - Flags: approval-mozilla-beta?
Attachment #8536659 - Flags: approval-mozilla-beta+
Attachment #8536659 - Flags: approval-mozilla-aurora?
Attachment #8536659 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main35+]
Whiteboard: [adv-main35+] → [adv-main35+][b2g-adv-main2.2+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.