Closed Bug 476547 (CVE-2010-3174) Opened 12 years ago Closed 11 years ago

Crash [@ nsTextFrameUtils::TransformText] with MathML, lquote, CSS quotes property

Categories

(Core :: MathML, defect)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta4-fixed
blocking1.9.1 --- .14+
status1.9.1 --- .14-fixed

People

(Reporter: jruderman, Assigned: roc)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical?])

Crash Data

Attachments

(4 files, 1 obsolete file)

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]
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.
Flags: blocking1.9.2?
Assignee: nobody → roc
Flags: blocking1.9.2? → wanted1.9.2+
Depends on: 515771
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.
Group: core-security
Whiteboard: [sg:critical?]
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.
Attached patch fixSplinter Review
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.
Attachment #404767 - Flags: review?(mozbugz)
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
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.
Attachment #404767 - Flags: review?(mozbugz) → review+
Whiteboard: [sg:critical?][needs review] → [sg:critical?][needs landing]
http://hg.mozilla.org/mozilla-central/rev/47aeba980aeb
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs landing] → [sg:critical?][needs 192 approval]
Attachment #404767 - Flags: approval1.9.2? → approval1.9.2+
Whiteboard: [sg:critical?][needs 192 approval] → [sg:critical?][needs 192 landing]
also crashes 1.9.1
bp-4d08d7b7-3308-4cce-a33d-433672100428

and 1.9.0
bp-885d6b09-9553-4f8c-8d07-ee5d82100428
blocking1.9.1: --- → ?
Flags: wanted1.9.0.x+
Need a mozilla-1.9.1 branch patch here, please.
blocking1.9.1: ? → .11+
Attached patch 1.9.1 patch (obsolete) — Splinter Review
1.9.2 patch applies fine after fixing file names
Attachment #442814 - Flags: approval1.9.1.11?
Comment on attachment 442814 [details] [diff] [review]
1.9.1 patch

Approved for 1.9.1.11, a=dveditz for release-drivers
Attachment #442814 - Flags: approval1.9.1.11? → approval1.9.1.11+
Whiteboard: [sg:critical?] → [sg:critical?][needs 191 landing]
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.
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)
I backed it out to fix the test failure.

I don't have time to work on this now.
Whiteboard: [sg:critical?][needs 191 landing] → [sg:critical?]
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.
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.
blocking1.9.1: .11+ → ?
Comment on attachment 442814 [details] [diff] [review]
1.9.1 patch

Nominating for the next release.
Attachment #442814 - Flags: approval1.9.1.11+ → approval1.9.1.12?
blocking1.9.1: ? → .12+
Whiteboard: [sg:critical?] → [sg:critical?][needs new 1.9.1 patch]
Attachment #442814 - Flags: approval1.9.1.12? → approval1.9.1.11-
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.
I still don't have time to work on this.
Moving 1.9.1 blocking flag forward then.
blocking1.9.1: .12+ → .13+
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.
Attachment #442814 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [sg:critical?][needs new 1.9.1 patch] → [sg:critical?]
Attachment #478068 - Flags: approval1.9.1.14+
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?
Alias: CVE-2010-3174
It's fine, quoteContent can't be null.
Group: core-security
Crash Signature: [@ nsTextFrameUtils::TransformText]
You need to log in before you can comment on or make changes to this bug.