Closed Bug 1387799 Opened 2 years ago Closed 2 years ago

AddressSanitizer: heap-use-after-free @ nsTArray_base<...>::Length() | mozilla::layers::CompositorBridgeChild::RecvDidComposite

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 57+ fixed
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 + fixed
firefox58 + fixed

People

(Reporter: bc, Assigned: milan)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [gfx-noted][adv-main57+][adv-esr52.5+][post-critsmash-triage])

Attachments

(3 files, 1 obsolete file)

Caution: This site does not appear to contain malware but on one occasion I was hit by an infinite app launcher via urls such as mailto etc. Be cautious.

This requires a Linux x86_64 ASAN *DEBUG* build.

1. Install Spider
   <https://bclary.com/projects/spider/spider-0.1.0.5-an+fn+fx+sm+tb.xpi>

2. firefox -spider -start -quit -url https://krgamedev.itch.io/word-whip

3. ==9560==ERROR: AddressSanitizer: heap-use-after-free on address 0x6170002a3a88 at pc 0x7f352a23f442 bp 0x7fff1b358aa0 sp 0x7fff1b358a98
READ of size 8 at 0x6170002a3a88 thread T0
    #0 0x7f352a23f441 in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::Length() const /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:398:37
    #1 0x7f352c10826a in mozilla::layers::CompositorBridgeChild::RecvDidComposite(unsigned long const&, unsigned long const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /home/worker/workspace/build/src/gfx/layers/ipc/CompositorBridgeChild.cpp:533:40

I do have a saved version captured via wget but I don't know how useful it will be to determine if there was mal-intent.
Group: core-security → gfx-core-security
Jim, could take a look at this bug to investigate what's going on? (I'm making a guess that you are the right person for this part of the code, please reassign if somebody else would be more suitable, thx)
Flags: needinfo?(jmathies)
Priority: -- → P3
Whiteboard: [gfx-noted]
I have a hunch, but I imagine we can't test it if we can't reproduce it.  I'll attach a patch.
Assignee: nobody → milan
Flags: needinfo?(jmathies)
This was probably introduced in v50.  The CompositorBridgeChild can go away during ::RecvDidComposite(), as first dealt with in bug 1242668.  Bug 1176011 introduced more members, which are accessed after it potentially went away.

Maybe :)
Attached patch Hold on to the array items (obsolete) — Splinter Review
Not sure we can have the callers grip the object, it's coming from IPC; can't extra ref the array itself, so this just holds on to the array elements harder.

Another idea?
Attachment #8913779 - Flags: review?(jmuizelaar)
Comment on attachment 8913779 [details] [diff] [review]
Hold on to the array items

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

::: gfx/layers/ipc/CompositorBridgeChild.cpp
@@ +526,5 @@
>  CompositorBridgeChild::RecvDidComposite(const uint64_t& aId, const uint64_t& aTransactionId,
>                                          const TimeStamp& aCompositeStart,
>                                          const TimeStamp& aCompositeEnd)
>  {
> +  AutoTArray<RefPtr<TextureClientPool>,2> texturePools;

I think texturePools = mTexturePools should be sufficient instead of the manual loop.

Also maybe add a comment about why this needs to be done.
Attachment #8913779 - Flags: review?(jmuizelaar) → review+
Duplicate of this bug: 1408685
Patch with review comments addressed.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easy - we don't know what's trigger it.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? How it manifests, but not how to get there.

Which older supported branches are affected by this flaw? All relevant.

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

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

How likely is this patch to cause regressions; how much testing does it need? Not likely.  Some objects now live longer.
Attachment #8913779 - Attachment is obsolete: true
Attachment #8919484 - Flags: sec-approval?
Attachment #8919484 - Flags: review+
Attachment #8919484 - Attachment description: Hold on to the array items → Hold on to the array items. Carry r+
Comment on attachment 8919484 [details] [diff] [review]
Hold on to the array items.  Carry r+

sec-approval+ for trunk.
Let's get Beta and ESR52 patches made and nominated as well.
Attachment #8919484 - Flags: sec-approval? → sec-approval+
Comment on attachment 8919484 [details] [diff] [review]
Hold on to the array items.  Carry r+

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Low volume UAF.
Fix Landed on Version: 58
Attachment #8919484 - Flags: approval-mozilla-esr52?
Attachment #8919484 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/8199078921c6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 8199078921c6b4c0ee4c984100d33750dfe8e144
> 
> This doesn't graft cleanly to ESR52 (Beta is fine). Please provide a rebased
> patch.

Coming up.
Comment on attachment 8919484 [details] [diff] [review]
Hold on to the array items.  Carry r+

Sec-high, Beta57+, ESR52+
Attachment #8919484 - Flags: approval-mozilla-esr52?
Attachment #8919484 - Flags: approval-mozilla-esr52+
Attachment #8919484 - Flags: approval-mozilla-beta?
Attachment #8919484 - Flags: approval-mozilla-beta+
Group: gfx-core-security → core-security-release
Whiteboard: [gfx-noted] → [gfx-noted][adv-main57+][adv-esr52.5+]
Flags: qe-verify-
Whiteboard: [gfx-noted][adv-main57+][adv-esr52.5+] → [gfx-noted][adv-main57+][adv-esr52.5+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.