Closed Bug 1273202 Opened 3 years ago Closed 3 years ago

HTMLInputElement may dispatch events inside its destructor

Categories

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

36 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- wontfix
firefox47 + fixed
firefox48 + fixed
firefox49 + fixed
firefox-esr45 47+ fixed

People

(Reporter: smaug, Assigned: smaug)

Details

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

Attachments

(2 files)

Attached patch patchSplinter Review
This should fix bug 1210595.
Attachment #8752942 - Flags: review?(jwatt)
This is sec-high or -critical, since we end up touching deleted objects.
Keywords: sec-high
Comment on attachment 8752942 [details] [diff] [review]
patch

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+
makes probably a bit harder to merge this to all the branches, but sure.
Attached patch with enumSplinter Review
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?
all

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.
Attachment #8752959 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/87e32446023f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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+
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+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.