1248794 - crash in `anonymous namespace''::WindowsGamepadService::ScanForDevices

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jukka Jylänki]

This bug was filed from the Socorro interface and is 
report bp-fc3e7119-b8f8-4b21-afe0-2d6f32160216.
=============================================================

Occurred when running 32-bit Firefox Nightly on emunittest 0.5 suite.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

As far as I see, Observer::mSvc is effectively unsafe Foo*, and nothing guarantees 
Observer::mSvc outlives the Observer object.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

WindowsGamepadService::Shutdown() should probably have 
if (mObserver) {
  mObserver->Stop();
  mObserver = nullptr;
}
or some such.

Do we have similar error on OSX and Linux backends?

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Olli Pettay [:smaug] (high review load) from comment #2)
> WindowsGamepadService::Shutdown() should probably have 
> if (mObserver) {
>   mObserver->Stop();
>   mObserver = nullptr;
> }
> or some such.
> 
> Do we have similar error on OSX and Linux backends?

No, this only exists in windows because of the way device hotplugging works. I think just putting guards around the mObserver access and making sure it's cleared on shutdown should fix it.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Clean up observer on WindowsGamepadService shutdown

Cleans up observer on shutdown, and adds checks for validity on use.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8720069 [details] [diff] [review]
Patch 1 (v1) - Clean up observer on WindowsGamepadService shutdown

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

I don't think it'd be very easy to exploit this. It's require racing a device connection and shutdown of the service, which would be difficult for an attacker to simulate through the browser alone.

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

Not really.

Which older supported branches are affected by this flaw?

This came in with the original Gamepad patches, so it's been around a few years.

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

Bug 604039

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

I think this patch should backport cleanly, but if not, cleanup shouldn't be too hard.

How likely is this patch to cause regressions; how much testing does it need?

Shouldn't cause any regressions, the usual test runs should be fine.

Comment 6

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

sec-approval+ for trunk. I'm not sure if this will be backported though.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/458e59c0a6d9

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Kyle, should we uplift this fix to Beta45 and Aurora46?

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8720069 [details] [diff] [review]
Patch 1 (v1) - Clean up observer on WindowsGamepadService shutdown

Sure, I think it'd be a low impact uplift.

Approval Request Comment
[Feature/regressing bug #]: Bug 604039
[User impact if declined]: Possible use-after-free in windows gamepad. Difficult to hit, but it's there
[Describe test coverage new/current, TreeHerder]: Merged and passed m-c tests
[Risks and why]: None known
[String/UUID change made/needed]: None

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8720069 [details] [diff] [review]
Patch 1 (v1) - Clean up observer on WindowsGamepadService shutdown

Fix a crash, taking it.

Should be in 45 beta 9.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f726135384f7

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e9ddb2825ac

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Kyle, based on comment 5 and 6, should I wontfix this for esr38? Or do you think we should uplift to esr38 as well? Thanks!

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I think it'd be a clean uplift to esr38 if we want to try, but I don't think it's mandatory.

Comment 15

[Daniel Veditz [:dveditz]]

Lowering the security severity based on comment 5.