723446 - (CVE-2012-0459) Access to a keyframe's cssText after dynamic modification always crashes Gecko

Status: VERIFIED FIXED

(Core :: DOM: CSS Object Model, defect, P1)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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 1

[Scoobidiver (away)]

I can reproduce it on Windows:
bp-a273ec4c-38cb-48ee-b455-b85252120202
bp-1f146839-8bb8-460d-94c0-967542120202
bp-dab15220-8dc3-4a77-bd1d-6af492120202

Comment 2

[Boris Zbarsky [:bzbarsky]]

(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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Daniel's testcase

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: 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-4, no longer working on Mozilla)]

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

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/880df44948d8

Comment 9

[Boris Zbarsky [:bzbarsky]]

Jesse, can you add this stuff to your fuzzers?  This is the second bug we've had in it...

Comment 10

[Boris Zbarsky [:bzbarsky]]

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]]

https://hg.mozilla.org/mozilla-central/rev/880df44948d8

Comment 12

[Alex Keybl [:akeybl]]

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 13

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/68fb495e3183
http://hg.mozilla.org/releases/mozilla-beta/rev/edef91ca0282

Comment 14

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

Crash in 10.0.2 on OS X 10.7.2: bp-db84aa15-8156-4423-ba6c-76c472120224

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

[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

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-esr10/rev/80653ae3ce0e

Comment 18

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

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]]

verified with latest builds of esr10, fx11, fx12 and fx13 on Mac and Win XP

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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 [:bzbarsky]]

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]

> 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]

> Can we fix nsAutoPtr?  This seems like a massive footgun.

Bug 827596.