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]
:
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]
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]
dbaron: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description 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 Boris Zbarsky [:bz] 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 Boris Zbarsky [:bz] 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 Boris Zbarsky [:bz] 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 Boris Zbarsky [:bz] 2012-02-02 07:21:49 PST
Created attachment 593832 [details]
Daniel's testcase
Comment 6 Boris Zbarsky [:bz] 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 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 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 9 Boris Zbarsky [:bz] 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 Boris Zbarsky [:bz] 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 Ed Morley [:emorley] 2012-02-03 11:18:43 PST
https://hg.mozilla.org/mozilla-central/rev/880df44948d8
Comment 12 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 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 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 18 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 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 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 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 Boris Zbarsky [:bz] 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 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 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.