1273202 - HTMLInputElement may dispatch events inside its destructor

Status: RESOLVED FIXED

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

Description

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

Attachment: patch

This should fix bug 1210595.

Comment 1

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

This is sec-high or -critical, since we end up touching deleted objects.

Comment 2

[Jonathan Watt [:jwatt]]

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.

Comment 3

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

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

Comment 4

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

Attachment: with enum

Comment 5

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

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.

Comment 6

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

sec-approval+ for trunk. I'd like to see this in Aurora, Beta, and ESR45 as well as a sec-high JS issue.

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/87e32446023f9d56c5ca367834b1d9ee7a1701c3

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/87e32446023f

Comment 9

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

Comment on attachment 8752959 [details] [diff] [review]
with enum

Sec-high, Aurora48+, Beta47+, ESR45+

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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