Last Comment Bug 723446 - (CVE-2012-0459) Access to a keyframe's cssText after dynamic modification always crashes Gecko
(CVE-2012-0459)
: Access to a keyframe's cssText after dynamic modification always crashes Gecko
Status: VERIFIED FIXED
[sg:critical][qa!]
: crash, testcase
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: Trunk
: All All
P1 critical (vote)
: mozilla13
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-02 03:01 PST by Daniel Glazman (:glazou)
Modified: 2013-01-31 20:16 PST (History)
11 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
verified
-
verified
11+
verified
unaffected


Attachments
Daniel's testcase (1.22 KB, text/html)
2012-02-02 07:21 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Be a little more careful changing declarations on keyframe rules. (1.67 KB, patch)
2012-02-02 07:28 PST, Boris Zbarsky [:bz] (still a bit busy)
dbaron: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Daniel Glazman (:glazou) 2012-02-02 03:01:03 PST
The following test modifies a keyframe through JS and then reads its cssText
twice. Gecko will always crash, sometimes at first read, sometimes at second:

  http://glazman.org/tmp/cssanim_bug.html

Verified on OS X and Linux 32bits with 12.0a1.

Related crash reports:

  bp-7624db3c-492f-42f1-9364-e2f132120202
  bp-8eaecc4f-b4f1-4b5c-870c-b235b2120202
  bp-9c64754f-0056-48eb-936f-0f4a62120202
  bp-4237d936-28e5-4f62-8a00-f5d9c2120202
  bp-9c32ddbf-211f-4d7d-9897-448002120202

This bug blocks the CSS Animations' editor I am currently working on for
BlueGriffon.
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-02 07:01:17 PST
(gdb) frame 3
#3  0x00000001020fbf03 in nsCSSKeyframeRule::GetCssText (this=0x122f867c0, aCssText=@0x118f9dba0) at ../../../mozilla/layout/style/nsCSSRules.cpp:1892
1892      mDeclaration->ToString(tmp);
(gdb) p mDeclaration.mRawPtr
$5 = (class mozilla::css::Declaration *) 0x122fc9160
(gdb) p/x mDeclaration.mRawPtr
$6 = 0x122fc9160
(gdb) frame 3
#3  0x00000001020fbf03 in nsCSSKeyframeRule::GetCssText (this=0x122f867c0, aCssText=@0x118f9dba0) at ../../../mozilla/layout/style/nsCSSRules.cpp:1892
1892      mDeclaration->ToString(tmp);
(gdb) p mDeclaration.mRawPtr
$7 = (class mozilla::css::Declaration *) 0x122fc9160
(gdb) p/x *mDeclaration.mRawPtr
$8 = {
  mOrder = {
    <nsAutoArrayBase<nsTArray<unsigned char, nsTArrayDefaultAllocator>,8u>> = {
      <nsTArray<unsigned char,nsTArrayDefaultAllocator>> = {
        <nsTArray_base<nsTArrayDefaultAllocator>> = {
          mHdr = 0x5a5a5a5a5a5a5a5a
        }, 
        <nsTArray_SafeElementAtHelper<unsigned char,nsTArray<unsigned char, nsTArrayDefaultAllocator> >> = {<No data fields>}, <No data fields>}, 
      members of nsAutoArrayBase<nsTArray<unsigned char, nsTArrayDefaultAllocator>,8u>: 
      {
        mAutoBuf = {0x5a <repeats 16 times>}, 
        mAlign = {
          elem = 0x5a
        }
      }
    }, <No data fields>}, 
  mData = {
    mRawPtr = 0x5a5a5a5a5a5a5a5a
  }, 
  mImportantData = {
    mRawPtr = 0x5a5a5a5a5a5a5a5a
  }, 
  mImmutable = 0x1
}
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-02 07:12:00 PST
So what happens here is that the keyframe rule has:

  nsAutoPtr<mozilla::css::Declaration>       mDeclaration;

When we mutate the DOM declaration, the underlying CSS declaration is mutable, so we just change it in-place and call nsCSSKeyframeStyleDeclaration::SetCSSDeclaration.  This calls nsCSSKeyframeRule::ChangeDeclaration which does:

  mDeclaration = aDeclaration;

which deletes the declaration.  In this case, mDeclaration == aDeclaration before assignment.

We can either check that here, or change nsAutoPtr to handle this case...
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-02 07:20:47 PST
And fwiw, I'd really appreciate attaching testcases, not linking to them... in this case it would be have been much much better.  ;)
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-02 07:21:49 PST
Created attachment 593832 [details]
Daniel's testcase
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-02 07:28:12 PST
Created attachment 593833 [details] [diff] [review]
Be a little more careful changing declarations on keyframe rules.

I can add tests, but we probably don't want to land those until we open up the bug....
Comment 7 User image David Baron :dbaron: ⌚️UTC-8 2012-02-02 08:39:38 PST
Comment on attachment 593833 [details] [diff] [review]
Be a little more careful changing declarations on keyframe rules.

r=dbaron

Not sure what I think about nsAutoPtr now.
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-02 09:00:31 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/880df44948d8
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-02 09:02:39 PST
Jesse, can you add this stuff to your fuzzers?  This is the second bug we've had in it...
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-02 09:05:21 PST
Comment on attachment 593833 [details] [diff] [review]
Be a little more careful changing declarations on keyframe rules.

[Approval Request Comment]
Regression caused by (bug #): Initial landing of CSS Animations
User impact if declined: Probably-exploitable crashes
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky):  Very low risk.  Just avoids deleting an object we still care about.
String changes made by this patch: None.
Comment 11 User image Ed Morley [:emorley] 2012-02-03 11:18:43 PST
https://hg.mozilla.org/mozilla-central/rev/880df44948d8
Comment 12 User image Alex Keybl [:akeybl] 2012-02-05 13:51:17 PST
Comment on attachment 593833 [details] [diff] [review]
Be a little more careful changing declarations on keyframe rules.

[Triage Comment]
Given where we are in the cycle and the low-risk nature of this patch, approving for Aurora 12 and Beta 11.
Comment 14 User image Al Billings [:abillings] 2012-02-23 16:41:55 PST
Crash in 10.0.2 on OS X 10.7.2: bp-db84aa15-8156-4423-ba6c-76c472120224
Comment 16 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-02-23 16:46:38 PST
[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on http://hg.mozilla.org/releases/mozilla-esr10/by Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more information.
Comment 17 User image Boris Zbarsky [:bz] (still a bit busy) 2012-02-23 20:34:10 PST
http://hg.mozilla.org/releases/mozilla-esr10/rev/80653ae3ce0e
Comment 18 User image Al Billings [:abillings] 2012-03-01 17:36:08 PST
Checked crashing PoC with 1.9.2.27 on OS X and XP SP3. No crashes. Marking 1.9.2 as unaffected.
Comment 19 User image Tracy Walker [:tracy] 2012-03-05 13:20:46 PST
verified with latest builds of esr10, fx11, fx12 and fx13 on Mac and Win XP
Comment 21 User image Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-15 17:51:52 PDT
(In reply to Boris Zbarsky (:bz) from comment #3)
> We can either check that here, or change nsAutoPtr to handle this case...

Can we fix nsAutoPtr?  This seems like a massive footgun.
Comment 22 User image Boris Zbarsky [:bz] (still a bit busy) 2012-03-15 17:58:45 PDT
I asked bsmedberg; he was happier with not, but feel free to try to convince him otherwise.  The cost is an extra branch on every assignment to an nsAutoPtr.
Comment 23 User image Jesse Ruderman 2013-01-31 20:16:11 PST
> Jesse, can you add this stuff to your fuzzers?  This is the second bug we've had
> in it...

Better keyframes testing: af88c213f88a, f30ca3bed7d7
Better cssRules  testing: 065587740d67, 04de0dd4d228
Comment 24 User image Jesse Ruderman 2013-01-31 20:16:29 PST
> Can we fix nsAutoPtr?  This seems like a massive footgun.

Bug 827596.

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