Last Comment Bug 793848 - Crash with <svg:use> removed during its own scrollbar attribute mutation event
: Crash with <svg:use> removed during its own scrollbar attribute mutation event
Status: RESOLVED FIXED
[adv-track-main17+][adv-track-esr17+]...
: crash, csectype-uaf, regression, sec-critical, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 17 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks: 325861 344905 778810
  Show dependency treegraph
 
Reported: 2012-09-24 14:38 PDT by Jesse Ruderman
Modified: 2015-10-16 11:50 PDT (History)
11 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
+
fixed
17+
fixed


Attachments
testcase (crashes Firefox when loaded) (735 bytes, text/html)
2012-09-24 14:38 PDT, Jesse Ruderman
no flags Details
stack traces: (1) mutation event listener, (2) crash (19.95 KB, text/html)
2012-09-24 14:39 PDT, Jesse Ruderman
no flags Details
patch (1.06 KB, patch)
2012-09-25 15:01 PDT, Olli Pettay [:smaug]
bzbarsky: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
dveditz: sec‑approval+
Details | Diff | Review

Description Jesse Ruderman 2012-09-24 14:38:26 PDT
Created attachment 664226 [details]
testcase (crashes Firefox when loaded)

bp-0cada683-3e71-482d-8d98-0c2d92120924

I'm guessing this is a regression from bug 778810, which added the function mozilla::ScrollbarActivity::ActivityOccurred.

Why does scrolling set DOM attributes?
Comment 1 Jesse Ruderman 2012-09-24 14:39:25 PDT
Created attachment 664227 [details]
stack traces: (1) mutation event listener, (2) crash
Comment 2 Olli Pettay [:smaug] 2012-09-24 14:43:03 PDT
Why do we end up dispatching mutation event. Scrollbars should be native anonymous.
Comment 3 Olli Pettay [:smaug] 2012-09-25 15:01:25 PDT
Created attachment 664662 [details] [diff] [review]
patch

Shouldn't rely on the element type, but frame type.
In the test we end up getting parent frame which is ScrollFrame.
Comment 4 Olli Pettay [:smaug] 2012-09-26 11:44:09 PDT
As far as I see, this is a really old bug.
Comment 5 Scoobidiver (away) 2012-09-26 12:54:14 PDT
I loaded the testcase on Windows:
* 15.0.1: no crash
* 16.0b4: no crash
* 17.0a2: bp-101eb84b-d2b6-4f41-9dbf-37a5c2120926
* 18.0a1: bp-5894bccb-d4ac-448a-b32b-4c32d2120926
Comment 6 Olli Pettay [:smaug] 2012-09-26 13:54:29 PDT
Sure, using *that* testcase. But I don't see why the problem wouldn't happen with some other
testcase too.
Comment 7 Boris Zbarsky [:bz] 2012-09-27 08:06:26 PDT
Comment on attachment 664662 [details] [diff] [review]
patch

r=me
Comment 8 Olli Pettay [:smaug] 2012-09-27 12:20:43 PDT
Comment on attachment 664662 [details] [diff] [review]
patch

[Security approval request comment]
How easily can the security issue be deduced from the patch?
I'd say quite easily. Just try to make svg:use to use some unusual frame type

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
NA

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 same patch should work in older branches too.
(if the patch doesn't apply cleanly, the changes should be minor
syntactic changes. The code is several years old.)

How likely is this patch to cause regressions; how much testing does it need?
Should be reasonable safe. In normal cases svg:use should behave like svg:use.
Comment 9 Daniel Veditz [:dveditz] 2012-09-27 16:23:03 PDT
Comment on attachment 664662 [details] [diff] [review]
patch

sec-approval+ assuming you can get branch landing approval from drivers
Comment 10 Alex Keybl [:akeybl] 2012-09-28 10:19:47 PDT
(In reply to Olli Pettay [:smaug] from comment #8)
> Which older supported branches are affected by this flaw?
> All

Even though we don't crash in 15/16? Why are those branches marked as unaffected?

We're leaning towards not taking this for FF16 to limit risk, since this was internally reported.
Comment 11 Olli Pettay [:smaug] 2012-09-28 13:58:26 PDT
So I can certainly get access to supposed-to-be-native-anonymous element in FF16. So far no
crash, but I don't see why.
Comment 12 Olli Pettay [:smaug] 2012-09-28 13:59:20 PDT
If we don't take this to FF16, should we land the patch closer to FF17 release?
Comment 13 Alex Keybl [:akeybl] 2012-09-28 14:54:43 PDT
Comment on attachment 664662 [details] [diff] [review]
patch

Longstanding issue, internally reported, and FF16 doesn't crash. I don't think this qualifies for our final beta. Setting the sec-approval flag back to ? to answer the question of when to land on Central/Aurora/(next Beta).
Comment 14 Daniel Veditz [:dveditz] 2012-10-21 00:29:41 PDT
We'll want a test for this that we can check in once we've release Fx17.
Comment 15 Daniel Veditz [:dveditz] 2012-10-21 00:30:30 PDT
Comment on attachment 664662 [details] [diff] [review]
patch

sec-approval+, re-requesting branch approvals.
Comment 16 Olli Pettay [:smaug] 2012-10-21 05:38:53 PDT
https://hg.mozilla.org/mozilla-central/rev/ae051b7f7e4e
Comment 18 Mats Palmgren (:mats) 2013-05-13 07:32:57 PDT
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0da62e1348
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-05-13 13:44:54 PDT
https://hg.mozilla.org/mozilla-central/rev/eb0da62e1348
Comment 20 Tracy Walker [:tracy] 2014-01-10 10:41:15 PST
mass remove verifyme requests greater than 4 months old

Note You need to log in before you can comment on or make changes to this bug.