Note: There are a few cases of duplicates in user autocompletion which are being worked on.
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
(Assignee)

Comment 2

6 years ago
(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
(Assignee)

Comment 3

6 years ago
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...
(Assignee)

Comment 4

6 years ago
And fwiw, I'd really appreciate attaching testcases, not linking to them... in this case it would be have been much much better.  ;)
(Assignee)

Comment 5

6 years ago
Created attachment 593832 [details]
Daniel's testcase
(Assignee)

Comment 6

6 years ago
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+
(Assignee)

Comment 8

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/880df44948d8
Assignee: nobody → bzbarsky
tracking-firefox11: --- → ?
tracking-firefox12: --- → ?
Priority: -- → P1
Target Milestone: --- → mozilla13
(Assignee)

Comment 9

6 years ago
Jesse, can you add this stuff to your fuzzers?  This is the second bug we've had in it...
(Assignee)

Comment 10

6 years ago
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+
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 17

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

5 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.
(Assignee)

Comment 22

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