HTMLInputElement may dispatch events inside its destructor

RESOLVED FIXED in Firefox 47



DOM: Core & HTML
a year ago
11 months ago


(Reporter: smaug, Assigned: smaug)



36 Branch
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox46 wontfix, firefox47+ fixed, firefox48+ fixed, firefox49+ fixed, firefox-esr4547+ fixed)


(Whiteboard: [post-critsmash-triage][adv-main47+][adv-esr45.2+])


(2 attachments)



a year ago
Created attachment 8752942 [details] [diff] [review]

This should fix bug 1210595.
Attachment #8752942 - Flags: review?(jwatt)

Comment 1

a year ago
This is sec-high or -critical, since we end up touching deleted objects.
Keywords: sec-high
Comment on attachment 8752942 [details] [diff] [review]

Review of attachment 8752942 [details] [diff] [review]:

r+ with the enum added.

::: dom/html/HTMLInputElement.cpp
@@ +1006,5 @@
>  HTMLInputElement::~HTMLInputElement()
>  {
>    if (mNumberControlSpinnerIsSpinning) {
> +    StopNumberControlSpinnerSpin(false);

Ugh @ bool arg. This looks like the call is saying "don't stop spinner spins". Please add an enum value for this.
Attachment #8752942 - Flags: review?(jwatt) → review+

Comment 3

a year ago
makes probably a bit harder to merge this to all the branches, but sure.

Comment 4

a year ago
Created attachment 8752959 [details] [diff] [review]
with enum

Comment 5

a year ago
Comment on attachment 8752959 [details] [diff] [review]
with enum

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Hmm, maybe some JS could access the dispatched event and then afterwards 
do something with the .target field which internally would point to deleted object...

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
eDisallowDispatchingEvents in dtor does hint pretty strongly where the issue is, but
how to actually utilize that...not clear.

Which older supported branches are affected by this flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch seems to apply to beta tree with some fuzz.

How likely is this patch to cause regressions; how much testing does it need?
Should be very safe. Effectively adding an if-check to prevent doing the bad stuff during dtor call.
Attachment #8752959 - Flags: sec-approval?
Attachment #8752959 - Flags: approval-mozilla-esr45?
Attachment #8752959 - Flags: approval-mozilla-beta?
Attachment #8752959 - Flags: approval-mozilla-aurora?
sec-approval+ for trunk. I'd like to see this in Aurora, Beta, and ESR45 as well as a sec-high JS issue.
status-firefox46: --- → wontfix
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox47: --- → ?
tracking-firefox48: --- → +
tracking-firefox49: --- → +
tracking-firefox-esr45: --- → 47+
Attachment #8752959 - Flags: sec-approval? → sec-approval+

Comment 7

a year ago
Last Resolved: a year ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49


a year ago
tracking-firefox47: ? → +
Comment on attachment 8752959 [details] [diff] [review]
with enum

Sec-high, Aurora48+, Beta47+, ESR45+
Attachment #8752959 - Flags: approval-mozilla-esr45?
Attachment #8752959 - Flags: approval-mozilla-esr45+
Attachment #8752959 - Flags: approval-mozilla-beta?
Attachment #8752959 - Flags: approval-mozilla-beta+
Attachment #8752959 - Flags: approval-mozilla-aurora?
Attachment #8752959 - Flags: approval-mozilla-aurora+
status-firefox47: affected → fixed
status-firefox48: affected → fixed
status-firefox-esr45: affected → fixed
Group: core-security-release
Group: dom-core-security
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main47+][adv-esr45.2+]


11 months ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.