47.88 KB, image/png
593 bytes, text/html
5.61 KB, patch
|Details | Diff | Splinter Review|
Created attachment 461339 [details] Testcase (consumes lots of memory; crashes vulnerable versions of Firefox)
Smaug, can you take a look?
crash report from a windows vm using the test case in comment 2. http://crash-stats.mozilla.com/report/index/bp-4f69071c-65f6-4e1f-8ed4-2b6362100803 0 xul.dll TextRunWordCache::FinishTextRun gfx/thebes/gfxTextRunWordCache.cpp:456 1 xul.dll TextRunWordCache::MakeTextRun gfx/thebes/gfxTextRunWordCache.cpp:695 2 xul.dll MakeTextRun layout/generic/nsTextFrameThebes.cpp:446 3 xul.dll BuildTextRunsScanner::BuildTextRunForFrames layout/generic/nsTextFrameThebes.cpp:1807 4 xul.dll BuildTextRunsScanner::FlushFrames layout/generic/nsTextFrameThebes.cpp:1238 5 xul.dll BuildTextRuns layout/generic/nsTextFrameThebes.cpp:1172 6 xul.dll nsTextFrame::EnsureTextRun layout/generic/nsTextFrameThebes.cpp:2040 7 xul.dll nsTextFrame::Reflow layout/generic/nsTextFrameThebes.cpp:6293 8 xul.dll nsLineLayout::ReflowFrame layout/generic/nsLineLayout.cpp:853 9 xul.dll nsBlockFrame::ReflowInlineFrame layout/generic/nsBlockFrame.cpp:3722 10 xul.dll nsBlockFrame::DoReflowInlineFrames layout/generic/nsBlockFrame.cpp:3517 11 xul.dll nsBlockFrame::ReflowInlineFrames layout/generic/nsBlockFrame.cpp:3371 12 xul.dll nsBlockFrame::ReflowLine layout/generic/nsBlockFrame.cpp:2467 13 xul.dll nsBlockFrame::ReflowDirtyLines layout/generic/nsBlockFrame.cpp:1907 14 xul.dll nsBlockFrame::Reflow layout/generic/nsBlockFrame.cpp:1009 15 xul.dll nsBlockReflowContext::ReflowBlock layout/generic/nsBlockReflowContext.cpp:310 16 xul.dll nsBlockFrame::ReflowBlockFrame layout/generic/nsBlockFrame.cpp:3090 17 xul.dll nsBlockFrame::ReflowLine layout/generic/nsBlockFrame.cpp:2412 18 xul.dll nsBlockFrame::ReflowDirtyLines layout/generic/nsBlockFrame.cpp:1907 19 xul.dll nsBlockFrame::Reflow layout/generic/nsBlockFrame.cpp:1009 20 xul.dll nsContainerFrame::ReflowChild layout/generic/nsContainerFrame.cpp:738 21 xul.dll nsCanvasFrame::Reflow layout/generic/nsCanvasFrame.cpp:487 22 @0xad2c3f
need to look deeper to figure out if the crash results in a consistent stack trace, but looks like we see the signature TextRunWordCache::FinishTextRun in the wild between 10-28 times a day over the last month, and with possible increased frequency in 4.0b checking --- TextRunWordCache::FinishTextRun 20100722-crashdata.csv found in: 4.0b1 3.6.7 3.7a5 3.6.6 3.5.11 3.5.10 release total-crashes TextRunWordCache::FinishTextRun crashes pct. all 677113 28 4.1352e-05 4.0b1 21327 15 0.000703334 3.6.7 493844 9 1.82244e-05 3.7a5 583 1 0.00171527 3.6.6 64993 1 1.53863e-05 3.5.11 30481 1 3.28073e-05 3.5.10 6388 1 0.000156544 os breakdown TextRunWordCache::FinishTextRunTotal 28 Win5.1 0.32 Win6.0 0.07 Win6.1 0.54 Mac10.4 0.04 Mac10.5 0.04 Mac10.6 0.00 Lin2.4 0.00 domains of sites 5 // 3 http://www.youtube.com 1 http://www.youtube.com/watch?v=lP9VBAISXT8&feature=related 1 http://www.youtube.com/watch?v=W-WKYIgGBbU&feature=popular 1 http://www.youtube.com/watch?v=KY4Pxg07SIY&feature=related 1 http://www.youjizz.com 1 http://www.shqiperia.com 1 http://www.kaskus.us 1 http://www.google.com.eg 1 http://www.google.com 1 http://www.facebook.com 1 http://www.ensa-agadir.ac.ma 1 http://www.eenadu.net 1 http://www.4shared.com 1 http://vozforums.com 1 http://vnexpress.net 1 http://vkontakte.ru 1 http://uvatoolkit.com 1 http://static.ak.facebook.com 1 http://new.el-ahly.com 1 http://microhardware.org 1 http://mail.canhtoan.com:3000 1 http://consumersupport.lenovo.com 1 http://consumersupport.lenovo.com/vn/en/driversdownloads/drivers_list.aspx?categoryid=358677 1 http://9downsoft.com:2082 1 http://9downsoft.com:2082/frontend/x3/backup/dosqlupload.html
(In reply to comment #3) > Smaug, can you take a look? How strange, I didn't get *any* email about this bug. I wasn't CC'ed to this, but usually I get email about all sg:* bugs.
It would be great if someone at least vaguely familiar with textruns would look at this. My current guess is that TextRunWordCache::LookupWord somehow puts a "word" with invalid textrun to deferredwords, but I could be wrong. Btw, on linux glib seems to abort in this case GLib-ERROR **: gmem.c:176: failed to allocate 1342177280 bytes aborting...
(In reply to comment #7) > It would be great if someone at least vaguely familiar with textruns would look > at this. > My current guess is that TextRunWordCache::LookupWord somehow puts a "word" > with invalid textrun to deferredwords, but I could be wrong. > > Btw, on linux glib seems to abort in this case > GLib-ERROR **: gmem.c:176: failed to allocate 1342177280 bytes > aborting... Well, this was only tested against windows...
Created attachment 469403 [details] [diff] [review] patch, v1 - avoid uniscribe failure on long text runs This is the 1.9.2 equivalent of patch 1 in bug 553963 on trunk. The basic issue is that CopyItemSplitOversize does not handle large items correctly; it calls Uniscribe's ScriptBreak() function with a huge text buffer (42 million chars, in the testcase here), which fails. In a debug build, several assertions get triggered, but in a release build we end up with a textRun in an inconsistent state, leading to invalid array accesses, etc. On trunk, we're able to avoid the use of ScriptBreak here because the font & text restructuring we've done means that we have cluster information already available at this point, but on 1.9.2 that's not the case. So to fix this in a minimally-disruptive way, I have modified CopyItemSplitOversize to use a "sliding window" onto the entire buffer, analyzing a portion at a time so as to avoid the uniscribe limitation. With this patch, I can no longer reproduce the crash with this testcase, although the browser does become unresponsive for an extended time while creating the huge string, building the textRun, reflowing, etc. However, with sufficient patience (10 minutes or so on my fast desktop) it successfully displays the entire test page of 42 million chinese characters. On a memory-constrained system, it's still possible that this kind of test would die with an out-of-memory exception, but that should be a "safe" crash.
Confirmed that the same patch applies and fixes the equivalent crash on 1.9.1 as well.
Comment on attachment 469403 [details] [diff] [review] patch, v1 - avoid uniscribe failure on long text runs Looks OK. The logic in itemizer loop is a tad hairy, I *think* it looks okay but proving that is not a simple exercise.
We've fixed this issue on trunk in bug 553963, but the earlier branches remain vulnerable; requesting blocking1.9.2 and 1.9.1 as it seems possible this might be exploitable (see initial comments).
(In reply to comment #12) > We've fixed this issue on trunk in bug 553963, but the earlier branches remain > vulnerable; requesting blocking1.9.2 and 1.9.1 as it seems possible this might > be exploitable (see initial comments). Are new releases running on the patched trunk? I found evidence of memory corruption on Windows 7 running 3.6.8...
No, "trunk" is Firefox 4 nightlies. For the moment, anyway -- we will create a release branch in the near future and "trunk" will then be ongoing development for whatever comes after "Firefox 4"
Comment on attachment 469403 [details] [diff] [review] patch, v1 - avoid uniscribe failure on long text runs Requesting approval to land on 1.9.2 and 1.9.1. This patch prevents us calling Uniscribe with excessively large buffers, such that it fails and we are left with inconsistent text-runs, leading to invalid memory access and potential memory/stack corruption. The patch resolves the failure and gives correct, safe behavior with the testcase (effectively a stress-test) here. Automated testing to verify this is difficult as the error condition involves huge strings and textruns, and even when handled correctly we're going to experience an apparent "hang" for several minutes, and could experience an out-of-memory exception + crash. (This is the correct result in the event of an allocation failure inside the Uniscribe calls, for example.)
Calling this one "fixed" for trunk (by bug 553963). Branch fixes are tracked by the custom fields.
Comment on attachment 469403 [details] [diff] [review] patch, v1 - avoid uniscribe failure on long text runs Approved for 22.214.171.124 and 126.96.36.199, a=dveditz Please check this into 'default' on both branches (beware: 'tip' often points at relbranches) and update the status1.9.x fields to the appropriate <version>-fixed status.
Pushed to 1.9.1 and 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f282fc256af0 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b2da18a3f256
Is this testable/fuzzable? Throwing large strings at the browser to see what happens seems to be a common attack, ISTR someone filing a bug on this kind of thing nearly monthly. Usually it's just a harmless OOM/DOS. So it seems a bit surprising this was different and not noticed before, but I'd assume people will continue to probe for problems like this.
Yes, fuzzing with large strings is crashing all the time for me.
Martijn, any bug reports of yours that we should focus on fixing? ;)
Is this fixed in the 3.6.9 release?
(In reply to comment #22) > Is this fixed in the 3.6.9 release? "Approved for 188.8.131.52" means it will end up in 3.6.10.
(In reply to comment #23) > (In reply to comment #22) > > Is this fixed in the 3.6.9 release? > > "Approved for 184.108.40.206" means it will end up in 3.6.10. (In reply to comment #23) > (In reply to comment #22) > > Is this fixed in the 3.6.9 release? > > "Approved for 220.127.116.11" means it will end up in 3.6.10. Okay :) I'm 12, I'm relatively new to the development process of large-scale projects.
(In reply to comment #24) > Okay :) > I'm 12, I'm relatively new to the development process of large-scale projects. Hey Alex - I meant to say this earlier when you submitted your bug reports - but it's great to have you helping out. Quit apologizing for your age - you're doing just fine. Has someone introduced you to our IRC network? You might find it interesting/helpful to hang out there, watch the project go by - it's where we coordinate most things. I'm abusing bugzilla a little by using it as a chat forum here - if you have any questions or need help with anything, please feel free to email me: johnath at mozilla dot com
(In reply to comment #5) > need to look deeper to figure out if the crash results in a consistent stack > trace, but looks like we see the signature TextRunWordCache::FinishTextRun in > the wild between 10-28 times a day over the last month, and with possible > increased frequency in 4.0b Is this getting a consistent stack trace? Consistent enough for us to know if we've fixed this running the giant scary attached web page? :-)
This was fixed on trunk in bug 553963
Any reason why we haven't opened this up yet?
Given bug 606714, maybe we shouldn't disclose this yet.
Hate to break bad news. I think Mozilla is on the right track by giving kids 3K (Mozilla should be doing that each month imho) But this "bug" is a duplicate from unicode stack overflow: Bug 504342. Found by Andrew Haynes & Simon Berry-Byrne in 2009. I say duplicate, but actually it's copied pasted since it's exactly the same.
(In reply to comment #32) > Hate to break bad news. I think Mozilla is on the right track by giving kids 3K > (Mozilla should be doing that each month imho) But this "bug" is a duplicate > from unicode stack overflow: Bug 504342. Found by Andrew Haynes & Simon > Berry-Byrne in 2009. I say duplicate, but actually it's copied pasted since > it's exactly the same. What? I actually wrote the testcase myself... Also, doesn't this count as a separate vulnerability because it exploits a different part of Firefox? Also, according to http://blog.mozilla.com/security/2009/07/19/milw0rm-9158-stack-overflow-crash-not-exploitable-cve-2009-2479/ bug 504342 isn't exploitable.
Don't listen to Mike Shaver. Just kidding. No seriously, don't listen to him. Joking. Maybe. Shaver's theorem goes like; We've seen no exploits, so it isn't exploitable. A joke, of course. But he wrote it. Well, lot are exploitable. Not easily, but requires a lot of work. No it's not a different vulnerability, people just weren't paying attention. One simply writes a string beyond an array bound, off into stack space where one can control it. So it basically is an out of bounds error on the looks of it: mItems[aIndex+1]; // out of bounds possibility.