Bug 476547 (CVE-2010-3174)

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




10 years ago
8 years ago


(Reporter: jruderman, Assigned: roc)


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

assertion, crash, testcase
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)


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


(4 attachments, 1 obsolete attachment)



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

Comment 1

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

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.
Created attachment 404767 [details] [diff] [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]
Last Resolved: 9 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]
status1.9.2: --- → final-fixed
Whiteboard: [sg:critical?][needs 192 landing] → [sg:critical?]
also crashes 1.9.1

and 1.9.0
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, 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

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

Comment 17

9 years ago
status1.9.1: wanted → .11-fixed

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+ → ?
status1.9.1: .11-fixed → ---

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?


9 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 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+
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?]


8 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.
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.