Bug 476547 (CVE-2010-3174)

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

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: jruderman, Assigned: roc)

Tracking

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

Trunk
x86
macOS
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

11 years ago
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 2

10 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+
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.
Posted 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: 10 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+
Posted 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]
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.

Comment 18

9 years ago
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 19

9 years ago
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

9 years ago
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.

Comment 22

9 years ago
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?]

Updated

9 years ago
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.