Bug 723446 (CVE-2012-0459)

Access to a keyframe's cssText after dynamic modification always crashes Gecko

VERIFIED FIXED in Firefox 11

Status

()

defect
P1
critical
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: glazou, Assigned: bzbarsky)

Tracking

({crash, testcase})

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(firefox10 wontfix, firefox11- verified, firefox12- verified, firefox-esr1011+ verified, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical][qa!], crash signature)

Attachments

(2 attachments)

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.
I can reproduce it on Windows:
bp-a273ec4c-38cb-48ee-b455-b85252120202
bp-1f146839-8bb8-460d-94c0-967542120202
bp-dab15220-8dc3-4a77-bd1d-6af492120202
Crash Signature: [@ nsCSSCompressedDataBlock::TryReplaceValue] [@ nsCSSExpandedDataBlock::DoExpand] [@ nsCSSCompressedDataBlock::ValueFor(nsCSSProperty)] [@ mozilla::css::Declaration::Declaration(mozilla::css::Declaration const&)] [@ nsCSSExpandedDataBlock::DoExpand(n…
Keywords: crash, testcase
(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
}
Group: core-security
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...
And fwiw, I'd really appreciate attaching testcases, not linking to them... in this case it would be have been much much better.  ;)
I can add tests, but we probably don't want to land those until we open up the bug....
Attachment #593833 - Flags: review?(dbaron)
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.
Attachment #593833 - Flags: review?(dbaron) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/880df44948d8
Assignee: nobody → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla13
Jesse, can you add this stuff to your fuzzers?  This is the second bug we've had in it...
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.
Attachment #593833 - Flags: approval-mozilla-beta?
Attachment #593833 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/880df44948d8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Attachment #593833 - Flags: approval-mozilla-beta?
Attachment #593833 - Flags: approval-mozilla-beta+
Attachment #593833 - Flags: approval-mozilla-aurora?
Attachment #593833 - Flags: approval-mozilla-aurora+
Whiteboard: [qa+]
Whiteboard: [qa+] → [sg:critical][qa+]
[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.
Checked crashing PoC with 1.9.2.27 on OS X and XP SP3. No crashes. Marking 1.9.2 as unaffected.
verified with latest builds of esr10, fx11, fx12 and fx13 on Mac and Win XP
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Alias: CVE-2012-0459
(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.
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.
Group: core-security
> 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
> Can we fix nsAutoPtr?  This seems like a massive footgun.

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