xorl %eax, %eax

CVE-2009-1313: Mozilla Firefox Memory Corruption

leave a comment »

It was reported by Marc Gueury and Daniel Veditz and disclosed in 27 April 2009. It affects Mozilla Firefox prior to 3.0.10 (specifically, the 1.9.0 branch of its Layout component). The following code is taken from 3.0.9 release of the popular web browser.

50 /* rendering object for textual content of elements */
     ...
3494 void
3495 nsTextFrame::ClearTextRun()
3496 {
3497   // save textrun because ClearAllTextRunReferences will clear ours
3498   gfxTextRun* textRun = mTextRun;
3499
3500   if (!textRun)
3501     return;
3502
3503   UnhookTextRunFromFrames(textRun);
3504   // see comments in BuildTextRunForFrames...
3505 //  if (textRun->GetFlags() & gfxFontGroup::TEXT_IS_PERSISTENT) {
3506 //    NS_ERROR("Shouldn't reach here for now...");
3507 //    // the textrun's text may be referencing a DOM node that has changed,
3508 //    // so we'd better kill this textrun now.
3509 //    if (textRun->GetExpirationState()->IsTracked()) {
3510 //      gTextRuns->RemoveFromCache(textRun);
3511 //    }
3512 //    delete textRun;
3513 //    return;
3514 //  }
3515
3516   if (!(textRun->GetFlags() & gfxTextRunWordCache::TEXT_IN_CACHE)) {
3517     // Remove it now because it's not doing anything useful
3518     gTextRuns->RemoveFromCache(textRun);
3519     delete textRun;
3520   }
3521 }

The above function is taken from layout/generic/nsTextFrameThebes.cpp. As you can see, it’s a simple function, its only operations are:
1) Setting textRun pointer to point to mTextRun
2) Check that textRun is not NULL
3) Unhook it from its frame
4) Check if textRun is in cache
5) If this is the case, remove it from the cache and delete this object
The commented lines contain some previous code which was buggy too. The bug with the above code is that mTextRun could contain invalid flags from previous operations and thus leading to an incorrect free() (delete at line 3519) which could lead to an exploitable condition. To fix this, the Mozilla Firefox developers added a new constant:

+ // Set when this text frame is mentioned in the userdata for a textrun
+ #define TEXT_IN_TEXTRUN_USER_DATA  0x40000000
+

And updated various routines to include this behavior when handling the frames. Specifically, ClearAllTextRunReferences() was changed to include this:

 static void
 ClearAllTextRunReferences(nsTextFrame* aFrame, gfxTextRun* aTextRun)
 {
+  aFrame->RemoveStateBits(TEXT_IN_TEXTRUN_USER_DATA);
   while (aFrame) {
+    NS_ASSERTION(aFrame->GetType() == nsGkAtoms::textFrame,
+                 "Bad frame");
     if (aFrame->GetTextRun() != aTextRun)

Which removes the new flag from the frame and performs an assertion for debugging purposes. This function is used to remove TextRun from the frame continuation chain starting at aFrame, which should be marked as a TextRun owner (as it says on the comments). The next function being patched was BuildTextRunsScanner::BuildTextRunForFrames(). This was patched like this:

     newFlow->mStartFrame = mappedFlow->mStartFrame;
+    if (!mSkipIncompleteTextRuns) {
+      // If mSkipIncompleteTextRuns is set, then we're just going to
+      // throw away the userData.
+      newFlow->mStartFrame->AddStateBits(TEXT_IN_TEXTRUN_USER_DATA);
+    }
     newFlow->mDOMOffsetToBeforeTransformOffset = builder.GetCharCount() -
       mappedFlow->mStartFrame->GetContentOffset();

Again, it marks the TextRun with the new flag if it is set to skip the incomplete TextRun objects. Finally, nsContinuingTextFrame::Destroy() which is name is pretty self explanatory was patched like this:

   // frame chain from the start to the end.
-  if (!mPrevContinuation ||
+  // If this frame is mentioned in the userData for a textrun (say
+  // because there's a direction change at the start of this frame), then
+  // we have to clear the textrun because we're going away and the
+  // textrun had better not keep a dangling reference to us.
+  if ((GetStateBits() & TEXT_IN_TEXTRUN_USER_DATA) ||
+      !mPrevContinuation ||
       mPrevContinuation->GetStyleContext() != GetStyleContext()) {
     ClearTextRun();

Which once again uses the new flag as an additional check before calling ClearTextRun() which was the initial buggy routine. By doing this it eliminates the possibility of dangling reference as it explicitly states at the comments. More information that could assist in exploiting this remote memory corruption vulnerability can be found in the initial bug-track comments. Martin Wargers also provided a PoC HTML page for that bug which can be found here and according to the developers it reproduces the bug. It’s this:

<html><head><title> Bug 489647 -  New 1.9.0.9 topcrash [@nsTextFrame::ClearTextRun()]</title></head>
<body>
<div id="a" style="white-space: pre;">
m</div>
<script>
function doe() {
  document.getElementById('a').childNodes[0].splitText(1);
}
setTimeout(doe, 100);
</script>
</body>
</html>

Written by xorl

May 8, 2009 at 12:29

Posted in bugs

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s