Bob Clary [:bc] (inactive) Reporter
	Description • 7 years ago

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.

	Stephanie Ouillon [:arroway] (needinfo me)
	Comment 1 • 7 years ago

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)

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 2 • 7 years ago

I have a hunch, but I imagine we can't test it if we can't reproduce it.  I'll attach a patch.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 3 • 7 years ago

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 :)

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 4 • 7 years ago

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?

	Jeff Muizelaar [:jrmuizel]
	Comment 5 • 7 years ago

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.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 7 • 7 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 8 • 7 years ago

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.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 9 • 7 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8199078921c6b4c0ee4c984100d33750dfe8e144

This doesn't graft cleanly to ESR52 (Beta is fine). Please provide a rebased patch.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 11 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/8199078921c6

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 12 • 7 years ago

(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.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 14 • 7 years ago

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

Sec-high, Beta57+, ESR52+

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 7 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/598d726067c4

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 7 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/3d6d558ae6a6