Closed
Bug 476547
(CVE-2010-3174)
Opened 16 years ago
Closed 16 years ago
Crash [@ nsTextFrameUtils::TransformText] with MathML, lquote, CSS quotes property
Categories
(Core :: MathML, defect)
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
Details
(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical?])
Crash Data
Attachments
(4 files, 1 obsolete file)
|
274 bytes,
application/xhtml+xml
|
Details | |
|
273 bytes,
application/xhtml+xml
|
Details | |
|
7.62 KB,
patch
|
karlt
:
review+
roc
:
approval1.9.2+
|
Details | Diff | Splinter Review |
|
8.01 KB,
patch
|
christian
:
approval1.9.1.14+
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
| Reporter | ||
Comment 2•16 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 | ||
Updated•16 years ago
|
Assignee: nobody → roc
| Assignee | ||
Updated•16 years ago
|
Flags: blocking1.9.2? → wanted1.9.2+
Comment 3•16 years ago
|
||
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
Updated•16 years ago
|
Whiteboard: [sg:critical?]
| Assignee | ||
Comment 4•16 years ago
|
||
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.
| Assignee | ||
Comment 5•16 years ago
|
||
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)
| Assignee | ||
Updated•16 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
| Assignee | ||
Comment 6•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #404767 -
Flags: review?(mozbugz) → review+
| Assignee | ||
Updated•16 years ago
|
Whiteboard: [sg:critical?][needs review] → [sg:critical?][needs landing]
| Assignee | ||
Comment 7•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs landing] → [sg:critical?][needs 192 approval]
| Assignee | ||
Updated•16 years ago
|
Attachment #404767 -
Flags: approval1.9.2?
| Assignee | ||
Updated•15 years ago
|
Attachment #404767 -
Flags: approval1.9.2? → approval1.9.2+
| Assignee | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?][needs 192 approval] → [sg:critical?][needs 192 landing]
| Assignee | ||
Comment 8•15 years ago
|
||
status1.9.2:
--- → final-fixed
Whiteboard: [sg:critical?][needs 192 landing] → [sg:critical?]
Comment 9•15 years ago
|
||
also crashes 1.9.1
bp-4d08d7b7-3308-4cce-a33d-433672100428
and 1.9.0
bp-885d6b09-9553-4f8c-8d07-ee5d82100428
| Assignee | ||
Comment 11•15 years ago
|
||
1.9.2 patch applies fine after fixing file names
Attachment #442814 -
Flags: approval1.9.1.11?
Comment 12•15 years ago
|
||
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+
| Assignee | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][needs 191 landing]
Comment 13•15 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.
Comment 14•15 years ago
|
||
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)
| Assignee | ||
Comment 15•15 years ago
|
||
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?]
| Assignee | ||
Comment 16•15 years ago
|
||
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•15 years ago
|
||
Comment 18•15 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•15 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?
blocking1.9.1: ? → .12+
status1.9.1:
--- → wanted
Updated•15 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][needs new 1.9.1 patch]
Updated•15 years ago
|
Attachment #442814 -
Flags: approval1.9.1.12? → approval1.9.1.11-
Comment 20•15 years ago
|
||
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.
| Assignee | ||
Comment 21•15 years ago
|
||
I still don't have time to work on this.
| Assignee | ||
Comment 23•15 years ago
|
||
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
| Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [sg:critical?][needs new 1.9.1 patch] → [sg:critical?]
Attachment #478068 -
Flags: approval1.9.1.14+
Comment 24•15 years ago
|
||
Comment 25•15 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?
Updated•15 years ago
|
Alias: CVE-2010-3174
| Assignee | ||
Comment 26•15 years ago
|
||
It's fine, quoteContent can't be null.
Updated•14 years ago
|
Keywords: checkin-needed
Updated•14 years ago
|
Group: core-security
Updated•14 years ago
|
Crash Signature: [@ nsTextFrameUtils::TransformText]
You need to log in
before you can comment on or make changes to this bug.
Description
•