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 (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
:
Mentors:
Depends on: 515771
Blocks: randomstyles 347580
  Show dependency treegraph
 
Reported: 2009-02-02 16:15 PST by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
14 users (show)
roc: wanted1.9.2+
dveditz: wanted1.9.0.x+
roc: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
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) (email my personal email if necessary)
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) (email my personal email if necessary)
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) (email my personal email if necessary)
christian: approval1.9.1.14+
Details | Diff | Splinter Review

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 [:dveditz] 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) (email my personal email if necessary) 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) (email my personal email if necessary) 2009-10-05 21:07:52 PDT
Created attachment 404767 [details] [diff] [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) (email my personal email if necessary) 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) (email my personal email if necessary) 2009-10-15 21:45:30 PDT
http://hg.mozilla.org/mozilla-central/rev/47aeba980aeb
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-18 23:58:58 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/44db41f43111
Comment 9 Daniel Veditz [:dveditz] 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, not reading bugmail] 2010-04-30 13:22:35 PDT
Need a mozilla-1.9.1 branch patch here, please.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-04-30 14:46:17 PDT
Created attachment 442814 [details] [diff] [review]
1.9.1 patch

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

Approved for 1.9.1.11, a=dveditz for release-drivers
Comment 13 Robert Kaiser 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 (: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) (email my personal email if necessary) 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) (email my personal email if necessary) 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 18 :Ehsan Akhgari 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 2010-06-25 17:04:39 PDT
Comment on attachment 442814 [details] [diff] [review]
1.9.1 patch

Nominating for the next release.
Comment 20 Daniel Veditz [:dveditz] 2010-06-25 17:10:19 PDT
Comment on attachment 442814 [details] [diff] [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) (email my personal email if necessary) 2010-08-11 18:25:44 PDT
I still don't have time to work on this.
Comment 22 christian 2010-08-13 13:34:46 PDT
Moving 1.9.1 blocking flag forward then.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-23 14:34:11 PDT
Created attachment 478068 [details] [diff] [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 24 Daniel Veditz [:dveditz] 2010-09-28 23:01:12 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f6d450b3aff7
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) (email my personal email if necessary) 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.