Last Comment Bug 476547 - (CVE-2010-3174) Crash [@ nsTextFrameUtils::TransformText] with MathML, lquote, CSS quotes property
(CVE-2010-3174)
: Crash [@ nsTextFrameUtils::TransformText] with MathML, lquote, CSS quotes pro...
Status: RESOLVED FIXED
: [sg:critical?]
: assertion, crash, testcase
Product: Core
Classification: Components
Component: MathML
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Mozilla Corporation)
: mathml
:
: 515771
: 306939 347580
  Show dependency treegraph
 
Reported: 2009-02-02 16:15 PST by Jesse Ruderman
Modified: 2010-10-30 11:16 PDT (History)
14 users (show)
roc: wanted1.9.2+
dveditz: wanted1.9.0.x+
roc: in‑testsuite+
See Also:
Crash Signature:
[@ nsTextFrameUtils::TransformText]
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta4-fixed
  .14+
  .14-fixed


Attachments
testcase 1 (assertions only) (274 bytes, application/xhtml+xml)
2009-02-02 16:15 PST, Jesse Ruderman
no flags Details
testcase 2 (crashes Firefox when loaded) (273 bytes, application/xhtml+xml)
2009-02-02 16:15 PST, Jesse Ruderman
no flags Details
fix (7.62 KB, patch)
2009-10-05 21:07 PDT, Robert O'Callahan (:roc) (Mozilla Corporation)
karlt: review+
roc: approval1.9.2+
Details | Diff | Splinter Review
1.9.1 patch (8.04 KB, patch)
2010-04-30 14:46 PDT, Robert O'Callahan (:roc) (Mozilla Corporation)
dveditz: approval1.9.1.11-
Details | Diff | Splinter Review
1.9.1 patch that works (8.01 KB, patch)
2010-09-23 14:34 PDT, Robert O'Callahan (:roc) (Mozilla Corporation)
clegnitto: approval1.9.1.14+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2009-02-02 16:15:05 PST
Created attachment 360186 [details]
testcase 1 (assertions only)

Testcase 1:

###!!! ASSERTION: negative length: 'GetContentEnd() - mContentOffset >= 0', file nsTextFrame.h, line 307

###!!! ASSERTION: bad index: 'PRUint32(aIndex) < mState.mLength', file nsTextFragment.h, line 184

###!!! ASSERTION: integer overflow: 'mMaxTextLength <= mMaxTextLength + aFrame->GetContentLength()', file layout/generic/nsTextFrameThebes.cpp, line 1222

Testcase 2:

Null deref crash [@ nsTextFrameUtils::TransformText]
Comment 1 Jesse Ruderman 2009-02-02 16:15:36 PST
Created attachment 360187 [details]
testcase 2 (crashes Firefox when loaded)
Comment 2 Jesse Ruderman 2009-07-09 11:12:31 PDT
It would be nice if this were fixed, so I could look for textframe assertions again.  I have to ignore a bunch of them because of this known bug.
Comment 3 Daniel Veditz 2009-09-11 11:02:13 PDT
The null deref in testcase 2 is now covered by bug 515771, this bug can be about the assertions.

Unless this bug is saying the assertions are incorrect, "integer overflow" sounds like a security bug that needs hiding.
Comment 4 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-10-05 19:43:17 PDT
The bug here is that MathML's quote-setting is rather naive:

///////////////////////////////////////////////////////////////////////////
// For <ms>, it is assumed that the mathml.css file contains two rules:
// ms:before { content: open-quote; }
// ms:after { content: close-quote; }
// With these two rules, the frame construction code will
// create inline frames that contain text frames which themselves
// contain the text content of the quotes.
// So the main idea in this code is to see if there are lquote and 
// rquote attributes. If these are there, we ovewrite the default
// quotes in the text frames.

However, due to RTL and line-breaking the anonymous content for :before and :after can actually comprise multiple frames each, which is what's happening here.
Comment 5 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-10-05 21:07:52 PDT
Created attachment 404767 [details] [review]
fix

This fixes it. It turns out that because of changes to the frame tree since this was first implemented, rquote doesn't work at all on trunk. So I'm adding a test that lquote and rquote actually work.
Comment 6 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-10-05 21:09:56 PDT
BTW lquote and rquote should really be implemented by mapping into a style rule somehow, but I don't want to invest in that right now.
Comment 7 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-10-15 21:45:30 PDT
http://hg.mozilla.org/mozilla-central/rev/47aeba980aeb
Comment 8 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-11-18 23:58:58 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/44db41f43111
Comment 9 Daniel Veditz 2010-04-28 17:42:18 PDT
also crashes 1.9.1
bp-4d08d7b7-3308-4cce-a33d-433672100428

and 1.9.0
bp-885d6b09-9553-4f8c-8d07-ee5d82100428
Comment 10 Mike Beltzner [:beltzner] 2010-04-30 13:22:35 PDT
Need a mozilla-1.9.1 branch patch here, please.
Comment 11 Robert O'Callahan (:roc) (Mozilla Corporation) 2010-04-30 14:46:17 PDT
Created attachment 442814 [details] [review]
1.9.1 patch

1.9.2 patch applies fine after fixing file names
Comment 12 Daniel Veditz 2010-06-14 10:31:36 PDT
Comment on attachment 442814 [details] [review]
1.9.1 patch

Approved for 1.9.1.11, a=dveditz for release-drivers
Comment 13 Robert Kaiser (:kairo@mozilla.com) 2010-06-21 05:46:35 PDT
This caused reftest failures on 1.9.1 on all platforms. See:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1277096000.1277096773.19239.gz (Mac)
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1277096175.1277097521.21730.gz (Linux)
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1277095999.1277096721.19141.gz (Windows)

SeaMonkey2.0 shows the exact same reftest failures, so it's surely not a one-off.
Comment 14 Karl Tomlinson, offline (:karlt) 2010-06-21 13:50:49 PDT
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-1.9.1-macosx-unittest-reftest/build/reftest/tests/layout/reftests/mathml/quotes-1.xhtml | timed out waiting for reftest-wait to be removed (after onload fired)
Comment 15 Robert O'Callahan (:roc) (Mozilla Corporation) 2010-06-21 21:56:11 PDT
I backed it out to fix the test failure.

I don't have time to work on this now.
Comment 16 Robert O'Callahan (:roc) (Mozilla Corporation) 2010-06-23 22:29:26 PDT
Can we bump this to the next release? I don't have time to work out why the test failed on branch. It's probably something simple though.
Comment 17 Ehsan Akhgari [:ehsan] 2010-06-25 14:41:28 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d7bf87af553a
Comment 18 Ehsan Akhgari [:ehsan] 2010-06-25 17:04:10 PDT
So, I think I shouldn't have landed this at all, since it has caused the same orange as comment 13.

I backed it out: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/801b22550e2e

I think roc meant to switch the blocking flag to ? in comment 16.
Comment 19 Ehsan Akhgari [:ehsan] 2010-06-25 17:04:39 PDT
Comment on attachment 442814 [details] [review]
1.9.1 patch

Nominating for the next release.
Comment 20 Daniel Veditz 2010-06-25 17:10:19 PDT
Comment on attachment 442814 [details] [review]
1.9.1 patch

I'm going to minus this patch for 1.9.1.11 because we'll need some sort of test fix. If you create a merged patch we can approve that, if it's just an additive test patch we can approve both. We'll wait for the explicit .12 requests to know which way to go.
Comment 21 Robert O'Callahan (:roc) (Mozilla Corporation) 2010-08-11 18:25:44 PDT
I still don't have time to work on this.
Comment 22 Christian Legnitto [:LegNeato] 2010-08-13 13:34:46 PDT
Moving 1.9.1 blocking flag forward then.
Comment 23 Robert O'Callahan (:roc) (Mozilla Corporation) 2010-09-23 14:34:11 PDT
Created attachment 478068 [details] [review]
1.9.1 patch that works

OK, the problem was simply that reftests on 1.9.1 don't support the MozReftestInvalidate event, so the test doesn't run properly.

Fixed that by doing a setTimeout after onload instead. It won't necessarily test dynamic updates reliably, but that doesn't matter.

This is ready to land.
Comment 25 Martin Stránský 2010-09-30 10:02:54 PDT
There's missing NULL check for quoteContent which was in the original code:

-  if (quoteContent && quoteContent->IsNodeOfType(nsINode::eTEXT)) {
[...]
+  if (!quoteContent->IsNodeOfType(nsINode::eTEXT))

Is it an intended change?
Comment 26 Robert O'Callahan (:roc) (Mozilla Corporation) 2010-10-07 22:13:06 PDT
It's fine, quoteContent can't be null.

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