476547 - (CVE-2010-3174) Crash [@ nsTextFrameUtils::TransformText] with MathML, lquote, CSS quotes property

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Attachment: testcase 2 (crashes Firefox when loaded)

Comment 2

[Jesse Ruderman]

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]]

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)]

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)]

Attachment: 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)]

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)]

http://hg.mozilla.org/mozilla-central/rev/47aeba980aeb

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/44db41f43111

Comment 9

[Daniel Veditz [:dveditz]]

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]]

Need a mozilla-1.9.1 branch patch here, please.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 1.9.1 patch

1.9.2 patch applies fine after fixing file names

Comment 12

[Daniel Veditz [:dveditz]]

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]

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)]

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)]

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)]

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

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d7bf87af553a

Comment 18

[(no longer active)]

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

[(no longer active)]

Comment on attachment 442814 [details] [diff] [review]
1.9.1 patch

Nominating for the next release.

Comment 20

[Daniel Veditz [:dveditz]]

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)]

I still don't have time to work on this.

Comment 22

[LegNeato]

Moving 1.9.1 blocking flag forward then.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f6d450b3aff7

Comment 25

[Martin Stránský [:stransky] (ni? me)]

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)]

It's fine, quoteContent can't be null.