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

RESOLVED FIXED in Firefox 45

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: jujjyl, Assigned: qdot)

Tracking

(4 keywords)

22 Branch
mozilla47
x86
Windows NT
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox44 wontfix, firefox45+ fixed, firefox46+ fixed, firefox47+ fixed, firefox-esr38- wontfix, firefox-esr45 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main45+], crash signature)

Attachments

(1 attachment)

Reporter

Description

3 years ago
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.
Flags: needinfo?(ted)
Flags: needinfo?(kyle)
Group: core-security
As far as I see, Observer::mSvc is effectively unsafe Foo*, and nothing guarantees 
Observer::mSvc outlives the Observer object.
WindowsGamepadService::Shutdown() should probably have 
if (mObserver) {
  mObserver->Stop();
  mObserver = nullptr;
}
or some such.

Do we have similar error on OSX and Linux backends?
(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.
Flags: needinfo?(kyle)
Cleans up observer on shutdown, and adds checks for validity on use.
Assignee: nobody → kyle
Attachment #8720069 - Flags: review?(ted)
Attachment #8720069 - Flags: review?(bugs)
Attachment #8720069 - Flags: review?(bugs) → review+
Group: core-security → dom-core-security
Attachment #8720069 - Flags: review?(ted) → review+
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.
Attachment #8720069 - Flags: sec-approval?
sec-approval+ for trunk. I'm not sure if this will be backported though.
Attachment #8720069 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/458e59c0a6d9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Kyle, should we uplift this fix to Beta45 and Aurora46?
Flags: needinfo?(kyle)
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
Flags: needinfo?(ted)
Flags: needinfo?(kyle)
Attachment #8720069 - Flags: approval-mozilla-beta?
Attachment #8720069 - Flags: approval-mozilla-aurora?
Group: dom-core-security → core-security-release
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.
Attachment #8720069 - Flags: approval-mozilla-beta?
Attachment #8720069 - Flags: approval-mozilla-beta+
Attachment #8720069 - Flags: approval-mozilla-aurora?
Attachment #8720069 - Flags: approval-mozilla-aurora+
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!
Flags: needinfo?(kyle)
I think it'd be a clean uplift to esr38 if we want to try, but I don't think it's mandatory.
Flags: needinfo?(kyle)
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main45+]
Flags: in-testsuite?
Version: unspecified → 22 Branch
Group: core-security-release
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.