HTMLInputElement may dispatch events inside its destructor

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({sec-high})

36 Branch
mozilla49
sec-high
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

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

Details

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

Attachments

(2 attachments)

(Assignee)

Description

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

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

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]
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+
(Assignee)

Comment 3

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

Comment 4

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

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?
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.
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+
(Assignee)

Comment 7

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/87e32446023f9d56c5ca367834b1d9ee7a1701c3
https://hg.mozilla.org/mozilla-central/rev/87e32446023f
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/9942206d3f9c
https://hg.mozilla.org/releases/mozilla-beta/rev/0723a0212f5e
https://hg.mozilla.org/releases/mozilla-esr45/rev/b74f1ab939d2
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+]

Updated

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