Bug 489647 (CVE-2009-1313)

New 1.9.0.9 topcrash [@nsTextFrame::ClearTextRun()]

VERIFIED FIXED

Status

()

Core
Layout
--
critical
VERIFIED FIXED
8 years ago
4 months ago

People

(Reporter: dveditz, Assigned: dholbert)

Tracking

(5 keywords)

1.9.0 Branch
crash, regression, topcrash, verified1.9.0.10, verified1.9.0.11
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.11 +
wanted1.9.0.x +
wanted1.8.1.x -
wanted1.8.0.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments)

Firefox 3.0.9 has exposed a new topcrash @nsTextFrame::ClearTextRun(). One instance is bug 489322, involving the HTML Validator addon. We'll see soon if that's the only case because the author of that addon is about to release an update with a workaround (disables the crashing feature).

Filing separate to track as a security bug.

Crashes:
http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.0.9&query_search=signature&query_type=exact&query=&date=&range_value=1&range_unit=weeks&do_query=1&signature=nsTextFrame%3A%3AClearTextRun()

This appears to be exploitable given random addresses at the top of the stack
bp-e5e76111-98f2-4785-9fe6-ba0582090421
bp-49a91d2b-b49c-4316-957e-d2c9b2090421
bp-87a98e87-4982-488f-8c11-6a2c72090421
bp-043a79b6-250a-4d52-8862-ef1d72090421
etc.

Here's one with a comment that does NOT mention view source -- this one says they did a ctrl-f (find) and clicked the "Highlight All" button.
bp-d31d7a90-09b4-4cf5-9baf-7b1952090422
Regression range is between the 2009-02-26 and 2009-02-27 builds on Windows only (looks like the extension has specific versions per-OS).

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2009-02-26&maxdate=2009-02-27&cvsroot=%2Fcvsroot

Pretty sure this is either bug 431260 or bug 444027.
Flags: blocking1.9.0.10?
Given the build hour being 06, you probably want a range more like http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2009-02-26%2004:00&maxdate=2009-02-27%2007:00&cvsroot=%2Fcvsroot although you could reduce it by looking at tinderbox.

In any case, I'd bet on bug 431260.
(Assignee)

Comment 3

8 years ago
Changing OS field from Mac to Windows, since the crash report table shows 0 mac crashes, 1 linux crash, and 1113 windows crashes.  (We can change the field to "All" after confirming a crash on another platform)

Note: Bugzilla's auto-linking doesn't catch the "()" on the end of the first URL in comment 0 (the table of all matching crash reports) -- you have to copy and paste the full link (or click it and then manually add "()" at the end) in order for the link to work.
OS: Mac OS X → Windows XP
So we fixed 431260, which wasn't really a security problem, and we introduced this bug which probably is. Perhaps we need to be more picky about what we land on branch.
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.10?
Flags: blocking1.9.0.10+
Whiteboard: [sg:critical?]
Daniel: Giving this over to you since you fixed bug 431260.
Assignee: nobody → dholbert
Since this isn't a problem on trunk or 1.9.1 (I presume), it's almost certain that a change in 1.9.1 fixed or prevented this bug. We just need to figure out which one.

Bug 472776 is a strong candidate, Daniel, can you see if that patch fixes your Linux 1.9.0 opt build?
(Assignee)

Comment 7

8 years ago
(In reply to comment #6)
> Since this isn't a problem on trunk or 1.9.1 (I presume)

That's correct -- I don't crash in my mozilla-central nightly, whereas I do in a 1.9.0.x nightly (per bug 489322 comment 24).

> Bug 472776 is a strong candidate, Daniel, can you see if that patch fixes your
> Linux 1.9.0 opt build?

Yeah, I'll give that a shot.
(Assignee)

Updated

8 years ago
OS: Windows XP → All
I guess a surefire way to track this would be to take the patch in bug 431260 and bisect along the 1.9.1 branch between the 1.9.0 branch point and where the patch actually landed to identify where adding that patch to the 1.9.1 branch no longer causes a crash.

Thanks Daniel!
It might not be bug 472776 that fixed it, since that landed on 1.9.1 on Feb 26, months after the patches in bug 431260 landed on 1.9.1.
Bug 467150 is another possibility.
(Assignee)

Comment 11

8 years ago
So while waiting for my opt build to finish, I figured I'd test mozilla-central nightlies from right after the potential regressing patches landed, in the hopes that we crashed there too (and then we'd be able to do a binary-search for the fix range).

I tried these mozilla-central nightly builds, to catch the just-landed fixes for bug 431341, bug 431260 / bug 455826, bug 444027 respectively:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1a2pre) Gecko/2008073002 Minefield/3.1a2pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20081128 Minefield/3.1b3pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090211 Minefield/3.2a1pre

However, none of them crashed.  So unless I'm making a mistake, it looks like we never actually hit this crash on mozilla-central. :-/
(Assignee)

Comment 12

8 years ago
Created attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

(In reply to comment #10)
> Bug 467150 is another possibility.

That bug's patch indeed fixes this (at least, it fixes bug 489322), in my 190x opt build!  I'm attaching a backported version of it here.

It looks like 190x already includes the *first* patch on that bug (labeled "patch that landed on trunk"), whereas mozilla-central has the *second* patch (labeled "revised patch").  The two patches conflict, so this backport simply undoes the first patch and applies the second patch, making 1.9.0.x match mozilla-central.
Attachment #374220 - Flags: approval1.9.0.10?
(Assignee)

Updated

8 years ago
Attachment #374220 - Attachment description: patch for bug 467150, backported to CVS trunk → 'revised patch' from bug 467150, backported to CVS trunk
(Assignee)

Comment 13

8 years ago
(In reply to comment #12)
> It looks like 190x already includes the *first* patch on that bug (labeled
> "patch that landed on trunk")

So, it turns out that patch was already applied on trunk because it was actually originally *part of* bug 455826's patch, attachment 348661 [details] [diff] [review], as posted on bugzilla (which I landed in 1.9.0.9 to fix bug 431260).

However, when bug 455826 originally landed on mozilla-central, it was missing that exact chunk due to a minor oversight, as explained in bug 455826 comment 16. That comment explained that this chunk would be handled & land in a separate bug -- bug 467150.  And *that* bug is where this chunk was landed, backed out, and then *improved* to form the patch that ended up actually fixing **this** bug here.

Hope that makes sense.  My bad for not seeing that bug 455826's patch got split up and improved elsewhere -- sorry about that!  I think this all makes sense now, though -- we just need to "complete" the patch for bug 455826 by updating the chunk that was improved in bug 467150.
(Assignee)

Comment 14

8 years ago
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

(In reply to comment #13)
> I think this all makes sense
> now, though -- we just need to "complete" the patch for bug 455826 by updating
> the chunk that was improved in bug 467150.

(That is to say -- we just need to land "patch for bug 467150, backported to CVS trunk", a.k.a.
attachment 374220 [details] [diff] [review])
(Assignee)

Updated

8 years ago
Depends on: 467150
(Assignee)

Comment 15

8 years ago
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

roc, just to confirm -- as the author of the patch on bug 467150, do you think it's suitable/safe for 1.9.0.x branch?

FWIW, it doesn't seem to have caused any known regressions -- the only dependency on it (besides this bug here and the wikipedia line-wrap bug, both of which I've just added as dependencies) is bug 430332, and that relationship is only there because bug 467150 was part of the fix for bug 430332 (per bug 430332 comment 39).
Attachment #374220 - Flags: review?(roc)
Created attachment 374249 [details]
testcase

This is a testcase that seems to reproduce this crash in current 1.9.0.x build:
http://crash-stats.mozilla.com/report/index/6ba4cbb4-9c50-454a-a54a-747f32090423?p=1
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

It's my fault for creating such a messy bug structure in the first place.
Attachment #374220 - Flags: review?(roc) → review+
(Assignee)

Comment 18

8 years ago
I've confirmed that the attached patch (attachment 374220 [details] [diff] [review]) fixes the
crash on mw22's testcase (attachment 374249 [details]), in my optimized build.

Testcase crashes immediately before patching & doesn't crash after patching.
(Assignee)

Comment 19

8 years ago
I can also confirm the crash with "Hightlight All" mentioned in comment 0 -- the crash report says to load http://msdn.microsoft.com/en-us/library/system.collections.hashtable(VS.71).aspx , hit Ctrl-F & type "using", and click "highlight all" --> crash)

The patch fixes that crash as well.
Attachment #374220 - Flags: approval1.9.0.10? → approval1.9.0.10+
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

Approved for 1.9.0.10, a=dveditz for release-drivers
(Assignee)

Comment 21

8 years ago
Patch landed:
Checking in layout/generic/nsTextFrameThebes.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.189; previous revision: 3.188
done
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Keywords: testcase-wanted → fixed1.9.0.10
Keywords: fixed1.9.0.11 → fixed1.9.0.10
Keywords: fixed1.9.0.10 → fixed1.9.0.11
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

Approved for 1.9.0.10 (the real one!). a=ss
Attachment #374220 - Flags: approval1.9.0.10+
To be clear, my approval is for the GECKO190_20090406_RELBRANCH.
Comment on attachment 374220 [details] [diff] [review]
'revised patch' from bug 467150, backported to CVS trunk

Landed on the release branch (GECKO190_20090406_RELBRANCH)
Checking in generic/nsTextFrameThebes.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.188.2.1; previous revision: 3.188
done
Keywords: fixed1.9.0.10
Blocks: 489322, 431260
(Assignee)

Comment 25

8 years ago
Setting in-testsuite? flag, as a reminder to check in mw22's testcase once the fix has made it out to users.
Flags: in-testsuite?
Hardware: x86 → All
(In reply to comment #16)
> Created an attachment (id=374249) [details]
> testcase
> 
> This is a testcase that seems to reproduce this crash in current 1.9.0.x build:
> http://crash-stats.mozilla.com/report/index/6ba4cbb4-9c50-454a-a54a-747f32090423?p=1

Verified the crash in 1.9.0.9 and that it no longer occurs in 1.9.0.10 on Linux with  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.10) Gecko/2009042315 Firefox/3.0.10. 

I'll mark this verified1.9.0.10 after I can check on Windows.

Updated

8 years ago
Alias: CVE-2009-1313

Updated

8 years ago
Duplicate of this bug: 489676
Verified on Windows XP: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10 (.NET CLR 3.5.30729)
Keywords: fixed1.9.0.10 → verified1.9.0.10

Updated

8 years ago
Duplicate of this bug: 490233
Group: core-security

Comment 34

8 years ago
the code does not exist on 1.8 and the testcase does not trigger a crash there either.
Flags: wanted1.8.1.x-
Flags: wanted1.8.0.x-
Depends on: 490410

Comment 35

8 years ago
Created attachment 376294 [details] [diff] [review]
Patch to add crashtest to trunk

I just took Martijn's test and made it into a crashtest.  I'll upload patches for 1.9.1 and 1.9.0 as well so that we'll know if we ever regress this.
Attachment #376294 - Flags: review?(dholbert)

Comment 36

8 years ago
Created attachment 376297 [details] [diff] [review]
Patch for 1.9.1
Attachment #376297 - Flags: review?(dholbert)
(Assignee)

Comment 37

8 years ago
Comment on attachment 376294 [details] [diff] [review]
Patch to add crashtest to trunk

Two things:
* Since this test uses setTimeout, could you add "reftest-wait" magic, for correctness?  (Otherwise, if we're super-speedy, I think we could successfully load the test and move on to the next testcase before the setTimeout fires.  Or something like that)

* Also, we should reduce the setTimeout wait-time to 0 (or a small positive value), as long as it still crashes 1.9.0.9 builds, which I think it should.  (We should we can avoid introducing biggish wait-times into our test suite, if at all possible, because they add up.)

Otherwise, looks good!

Comment 38

8 years ago
Created attachment 376301 [details] [diff] [review]
Patch for 1.9.0
Attachment #376301 - Flags: review?(dholbert)
(Assignee)

Comment 39

8 years ago
Comment on attachment 376294 [details] [diff] [review]
Patch to add crashtest to trunk

One more thing -- looks like the testcase has no newline at end of file (as noted in the raw patch), which is messy -- can you fix that, too?

r=dholbert, after addressing that & comment 37
Attachment #376294 - Flags: review?(dholbert) → review+
(Assignee)

Comment 40

8 years ago
oh, sorry, I just noticed you have reftest-wait magic already... I don't know how I missed that.  Disregard that part of my comments :)

Comment 41

8 years ago
(In reply to comment #40)
> oh, sorry, I just noticed you have reftest-wait magic already... I don't know
> how I missed that.  Disregard that part of my comments :)

I was about to tell you I already did :) I'll run some tests and see what I can get away with decreasing the setTimeout call to and fix that newline. Thanks for the quick review on this.  I'll repost the final test and carry your review forward.
Verified for 1.9.0.11 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11pre) Gecko/2009051404 GranParadiso/3.0.11pre.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.11 → verified1.9.0.11
(Assignee)

Comment 43

8 years ago
> (In reply to comment #40)
> I'll run some tests and see what I can
> get away with decreasing the setTimeout call to and fix that newline. Thanks
> for the quick review on this.  I'll repost the final test and carry your review
> forward.

Hi Clint! I think you might've forgotten about this -- looks like this crashtest never got reposted with those fixes (reduced timeout if possible, & add missing newline).
(Assignee)

Comment 44

8 years ago
Comment on attachment 376297 [details] [diff] [review]
Patch for 1.9.1

r=dholbert on branch crashtests, with the same fixes from comment 43 that are still needed for the trunk crashtest.
Attachment #376297 - Flags: review?(dholbert) → review+
(Assignee)

Updated

8 years ago
Attachment #376301 - Flags: review?(dholbert) → review+
Crash Signature: [@nsTextFrame::ClearTextRun()]

Comment 45

4 months ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8eec4f84d97
Crashtest.
Flags: in-testsuite? → in-testsuite+

Comment 46

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e8eec4f84d97
You need to log in before you can comment on or make changes to this bug.