The default bug view has changed. See this FAQ.
Bug 476547 (CVE-2010-3174)

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

RESOLVED FIXED

Status

()

Core
MathML
--
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: roc)

Tracking

(Blocks: 2 bugs, {assertion, crash, testcase})

Trunk
x86
Mac OS X
assertion, crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta4-fixed, blocking1.9.1 .14+, status1.9.1 .14-fixed)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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]
(Reporter)

Comment 1

8 years ago
Created attachment 360187 [details]
testcase 2 (crashes Firefox when loaded)
(Reporter)

Comment 2

8 years ago
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+

Updated

8 years ago
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.
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.
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
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs landing] → [sg:critical?][needs 192 approval]
Attachment #404767 - Flags: approval1.9.2?
Attachment #404767 - Flags: approval1.9.2? → approval1.9.2+
Whiteboard: [sg:critical?][needs 192 approval] → [sg:critical?][needs 192 landing]
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/44db41f43111
status1.9.2: --- → final-fixed
Whiteboard: [sg:critical?][needs 192 landing] → [sg:critical?]
also crashes 1.9.1
bp-4d08d7b7-3308-4cce-a33d-433672100428

and 1.9.0
bp-885d6b09-9553-4f8c-8d07-ee5d82100428
blocking1.9.1: --- → ?
status1.9.1: --- → wanted
Flags: wanted1.9.0.x+
Need a mozilla-1.9.1 branch patch here, please.
blocking1.9.1: ? → .11+
Created attachment 442814 [details] [diff] [review]
1.9.1 patch

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]

Comment 13

7 years ago
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.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d7bf87af553a
status1.9.1: wanted → .11-fixed
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+ → ?
status1.9.1: .11-fixed → ---
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?

Updated

7 years ago
blocking1.9.1: ? → .12+
status1.9.1: --- → wanted
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.

Comment 22

7 years ago
Moving 1.9.1 blocking flag forward then.
blocking1.9.1: .12+ → .13+
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.
Attachment #442814 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [sg:critical?][needs new 1.9.1 patch] → [sg:critical?]

Updated

7 years ago
Attachment #478068 - Flags: approval1.9.1.14+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f6d450b3aff7
status1.9.1: wanted → .14-fixed

Comment 25

7 years ago
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.
Keywords: checkin-needed
Group: core-security
Crash Signature: [@ nsTextFrameUtils::TransformText]
You need to log in before you can comment on or make changes to this bug.