Bug 489647 (CVE-2009-1313)

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

VERIFIED FIXED

Status

()

defect
--
critical
VERIFIED FIXED
10 years ago
2 years ago

People

(Reporter: dveditz, Assigned: dholbert)

Tracking

(5 keywords)

1.9.0 Branch
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)

Reporter

Description

10 years ago
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
Flags: blocking1.9.0.10?
Assignee

Comment 3

10 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.
Reporter

Updated

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

10 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

10 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.
Assignee

Comment 11

10 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

10 years ago
(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

10 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

10 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

10 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

10 years ago
Depends on: 467150
Assignee

Comment 15

10 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)
Posted file 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

10 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

10 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.
Reporter

Updated

10 years ago
Attachment #374220 - Flags: approval1.9.0.10? → approval1.9.0.10+
Reporter

Comment 20

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

10 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
Closed: 10 years ago
Resolution: --- → FIXED
Assignee

Updated

10 years ago
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
Reporter

Updated

10 years ago
Blocks: 489322, 431260
Assignee

Comment 25

10 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

10 years ago
Alias: CVE-2009-1313
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)
Duplicate of this bug: 490233
Reporter

Updated

10 years ago
Group: core-security

Comment 34

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

Comment 35

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

10 years ago
Attachment #376297 - Flags: review?(dholbert)
Assignee

Comment 37

10 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

10 years ago
Attachment #376301 - Flags: review?(dholbert)
Assignee

Comment 39

10 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

10 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

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

Comment 43

10 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

10 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

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

Updated

2 years ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.