Crashed for me after an allocation failure (std::bad_alloc exception) while building a gfxTextRun, presumably out-of-memory. Another test crashed with memory allocation failure in nsAString_internal while trying to modify a string with length over 100,000,000 characters(!). This suggests that allowing a script to create such huge strings is a more general problem, not just an issue for drawing the URL. The exact point where the out-of-memory crash will occur may be difficult to predict, but it's clear that we don't always handle allocation failure well. When I tried this in Safari just now, it actually crashed in WebCore::KURL::parse, rather than in the font code (although I can easily believe that is also possible). I tried this on Windows as well (Minefield trunk build); it appeared to hang for some time, and presented an "unresponsive script" message, but allowing it to continue did eventually succeed. It hit several assertions along the way, though: ###!!! ASSERTION: CopyItemSplitOversize() failed: 'NS_SUCCEEDED(nrs)', file c:/mozdev/gfx/thebes/src/gfxWindowsFonts.cpp, line 2204 ###!!! ASSERTION: UniscribeItem is too big, ScriptShape() will fail!: 'mMaxGlyphs < 65535', file c:/mozdev/gfx/thebes/src/gfxWindowsFonts.cpp, line 1638 ###!!! ASSERTION: Failed to shape, twice -- we should never hit this:'SUCCEEDED (rv)', file c:/mozdev/gfx/thebes/src/gfxWindowsFonts.cpp, line 2494 ###!!! ASSERTION: UniscribeItem is too big, ScriptShape() will fail!: 'mMaxGlyphs < 65535', file c:/mozdev/gfx/thebes/src/gfxWindowsFonts.cpp, line 1638
The WebCore bug when I hit it was a NULL deref. The allocation appears to fail and they don't fail gracefully. I guess I should file a webkit bug too - not sure if Apple propagated my report.
Should this bug be split into an Apple branch and a Pango branch? Or should we keep it together and fix the assertions noted in comment 1 (mMaxGlyphs < 65535) to avoid triggering either problem. 64K glyphs in one run ought to be enough for anyone.
I'd say let's just keep them together into this bug, and just clamp down on the number of glyphs -- probably much less than 64k, 16k should be fine for one run. roc, does that sound ok? I don't really see us doing a 16k-glyph run in any real world usage..
We certainly can have more than 16k glyphs in real-world usage. A single very large paragraph of white-space:normal text will do it.
Bug 357637 comment 59 also mentions the integer overflow in Pango, which may be the same issue as bug 430127.
I think we should safely handle textrun creation failures due to OOM. This shouldn't be too hard. We should also cap the maximum length of strings we pass to platform font engines. We already have code that caps strings passed to ATSUI at 500 characters (or less, when necessary, to work around 32K coordinate limits); see gfxAtsuiFontGroup::GuessMaximumStringLength. It doesn't fail, it just breaks up the incoming strings for separate processing and then glues the resulting textruns back together. The incoming text is broken up quite intelligently, generally at space boundaries so we're unlikely to disturb shapers. We should generalize that to all platforms and limit the text to something sensible (say 1K chars) no matter what.
One thing to keep in mind is that a malicious downloaded font might be able to associate a very large number of glyphs with a single character. Someone who knows a lot about Opentype and AAT might be able to figure out how large that number is. So we should probably cap the passed-down string length, say to <1K, but also check the number of glyphs returned and bail if it's too large, say >32K.
(In reply to comment #8) > also check the number of glyphs returned and bail if it's too large, say >32K. The problem here, with Pango, is that we can't check the number of glyphs until after pango_shape has returned, at which point even the number of glyphs may have been overwritten by untrusted data. It seems that this needs to be fixed in the library. If we can't rely on the library being fixed, pango_glyph_string_set_size is a dynamic symbol, so Mozilla can provide its own implementation. PangoGlyphString::space is meant to be private, but I haven't come up with any better ideas.
Created attachment 364264 [details] [diff] [review] patch for pango_glyph_string_set_size
Created attachment 364268 [details] [diff] [review] patch for pango_glyph_string_set_size (with more context)
Created attachment 364394 [details] [diff] [review] patch for safe abort in pango_glyph_string_set_size Callers don't check the new value of num_glyphs, so I think the best thing to do is to abort if allocation will not be possible. This is consistent with what g_realloc will do on failure anyway. I'm doubting this particular overflow is likely to be exploitable with current Pango code (but the patch would make things correct). This overflow would only happen on 32-bit systems, so I'll consider 32-bit systems here. Will, have you seen a SIGSEGV with Pango? On 32-bit or 64-bit systems? sizeof(PangoGlyphInfo) is 0x14, so the smallest new_len passed to pango_glyph_string_set_size that will cause an overflow is 0x08000001. However, the only callers to pango_glyph_string_set_size that subsequently write to the PangoGlyphString and don't increase the new_len gradually are pango_ot_buffer_output and _pango_shape_shape. For callers that increase new_len gradually, a value of 0x04000000 < new_len <= 0x08000000 will try to g_realloc 0x08000000 * 0x14 = 0xa0000000 (3 GB) which seems very unlikely to succeed, and failure results in an abort. pango_ot_buffer_output will set new_len to HB_Buffer::in_length. But it looks like this will only be a large as a successful allocation of n * sizeof(HB_GlyphItemRec). HB_GlyphItemRec seems to need 0x12 bytes, which I assume would align to 0x14, so an allocation of at least 0x08000001 * 0x14 = 0xa0000014 (>3GB) must succeed before new_len will be >= 0x08000001 in pango_glyph_string_set_size. (The size of the HB_GlyphItemRec buffer is increased gradually.) _pango_shape_shape is only called from pango-layout, which Mozilla only uses through GTK (not through our text code), and only when the text has PANGO_ATTR_SHAPE.
I'm committed a similar patch to Pango upstream now. Will be in 1.24.
We should at least put in some barriers like what roc described for 1.9.1 for this.
(In reply to comment #14) > So, in short: > - FF+pango: allocation crash/dos only(*) > - Camino+ripc: sigsegvs with some ability to control; possibly exploitable Just to make sure I'm understanding correctly: Camino would need to cap the length of every single string that might ever come from Core and end up being displayed in the UI? And doing so removes exploitability even if other Core crashes related to ginormous strings remain unfixed on the 1.9.0 branch? (Or alternately, Camino could presumably wait for an OS update... do you know specifically what versions of the OS are vulnerable?)
(In reply to comment #16) > (In reply to comment #14) > > So, in short: > > - FF+pango: allocation crash/dos only(*) > > - Camino+ripc: sigsegvs with some ability to control; possibly exploitable > > Just to make sure I'm understanding correctly: Camino would need to cap the > length of every single string that might ever come from Core and end up being > displayed in the UI? And doing so removes exploitability even if other Core > crashes related to ginormous strings remain unfixed on the 1.9.0 branch? No. There is nothing Camino-specific here other than that the bug reporter has failed to reproduce their particular bug in Firefox for some reason. We can mitigate this in Core.
If part of the bug is that Apple libraries will crash when given incredibly long strings, which seems to be what comment 0 is saying, then how does comment 7 address the entire problem for Camino? There is no direct Core-to-platform-font-engine interface in the case of the URL bar; Core gives Camino the string, and Camino gives it to the OS to render (through native controls).
Ah, that's true. You can't trust Apple's text drawing APIs so whenever you use them directly, you'll have to do something. Core can't help there.
(In reply to comment #14) > Behdad -- Awesome! I'm glad to see the patch heading upstream. When is 1.24 > planned to go out? In a week or two. > thanks! > will
A similar issue to the CG thing here is showing up in bug 482128.
So it sounds like the Core fixes here are: - On Mac, make 500-char CG/Quartz limit apply to all OS X versions - On Linux, no Core fixes, just depend on the fix being in pango 1.24 Right?
(In reply to comment #22) > - On Mac, make 500-char CG/Quartz limit apply to all OS X versions Yep > - On Linux, no Core fixes, just depend on the fix being in pango 1.24 You mean relying on distros to backport the fix to all their supported releases? OK I guess.
> > - On Linux, no Core fixes, just depend on the fix being in pango 1.24 > > You mean relying on distros to backport the fix to all their supported > releases? OK I guess. Since there was no fanfare for the pango 1.24 release and FF/linux won't be making any changes (it seems), I'll drop a line to various vendors soon to let them know that they may want to backport the pango fix. It'd be nice to be able to give them the option to do so before this bug becomes public (even if they choose not to fix it).
Created attachment 371294 [details] [diff] [review] apply OSX limit to all OSX versions Here's the OSX patch; just apply the 500 char limit to all OSX versions.
8 years ago
Comment on attachment 371294 [details] [diff] [review] apply OSX limit to all OSX versions Mac chunk: http://hg.mozilla.org/mozilla-central/rev/4256bc50a5db
Let's call this fixed -- we put in the Mac bandaid, and the linux fixes will come from pango updates. Reopen if anyone disagrees.
8 years ago
8 years ago
Comment on attachment 371294 [details] [diff] [review] apply OSX limit to all OSX versions Any reason not to take this in 1.9.0.x?
Comment on attachment 371294 [details] [diff] [review] apply OSX limit to all OSX versions Approved for 22.214.171.124, a=dveditz for release-drivers
Checking in gfx/thebes/src/gfxAtsuiFonts.cpp; /cvsroot/mozilla/gfx/thebes/src/gfxAtsuiFonts.cpp,v <-- gfxAtsuiFonts.cpp new revision: 1.102; previous revision: 1.101
Going to http://static.dataspill.org/glyph/pango_2.html, causing the latest 1.9.0 build to peg the cpu above 90%, use over 2 GB of RAM, and become non-responsive: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:126.96.36.199pre) Gecko/2009063004 GranParadiso/3.0.12pre. I don't get an actual crash but a complete hang. This does not appear to be fixed.
I see a CPU and memory spike, but after 10-15 seconds the CPU comes back and the memory is released when I close the tab. If you've got less than the amount of memory it's trying to use you might hit swapping/performance problems, but that isn't this bug.
1.8 looks safe.
Based on Dan's comments, marking this as verified for 188.8.131.52.
This is CVE-2009-1194