Bug 723446 (CVE-2012-0459)

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

VERIFIED FIXED in Firefox 11

Status

()

Core
DOM: CSS Object Model
P1
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: glazou, Assigned: bz)

Tracking

({crash, testcase})

Trunk
mozilla13
crash, testcase
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)

(Reporter)

Description

6 years ago
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

6 years ago
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::DoE…
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.  ;)
Created attachment 593832 [details]
Daniel's testcase
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....
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
tracking-firefox11: --- → ?
tracking-firefox12: --- → ?
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
Last Resolved: 6 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+
http://hg.mozilla.org/releases/mozilla-aurora/rev/68fb495e3183
http://hg.mozilla.org/releases/mozilla-beta/rev/edef91ca0282
status-firefox11: --- → fixed
status-firefox12: --- → fixed
Whiteboard: [qa+]

Updated

6 years ago
tracking-firefox11: ? → -
tracking-firefox12: ? → -
status-firefox-esr10: --- → affected
status-firefox10: --- → wontfix
tracking-firefox-esr10: --- → 11+
Whiteboard: [qa+] → [sg:critical][qa+]
Crash in 10.0.2 on OS X 10.7.2: bp-db84aa15-8156-4423-ba6c-76c472120224
status-firefox-esr10: affected → ---
status-firefox10: wontfix → ---
tracking-firefox-esr10: 11+ → ---
status-firefox-esr10: --- → affected
status-firefox10: --- → wontfix
tracking-firefox-esr10: --- → 11+
[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.
http://hg.mozilla.org/releases/mozilla-esr10/rev/80653ae3ce0e
status-firefox-esr10: affected → fixed
Checked crashing PoC with 1.9.2.27 on OS X and XP SP3. No crashes. Marking 1.9.2 as unaffected.
status1.9.2: --- → unaffected
verified with latest builds of esr10, fx11, fx12 and fx13 on Mac and Win XP
Status: RESOLVED → VERIFIED
status-firefox-esr10: fixed → verified
status-firefox11: fixed → verified
status-firefox12: fixed → verified

Updated

6 years ago
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

Comment 23

5 years ago
> 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

5 years ago
> 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.