Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 167443 Heap-buffer-overflow in WebCore::FontCache::releaseFontData
Starred by 0 users Project Member Reported by infe...@chromium.org, Dec 22 2012 Back to list
Status: Fixed
Owner: joh...@chromium.org
Closed: Jan 2013
Cc: pdr@chromium.org, fmalita@chromium.org, jchaffraix@chromium.org, schenney@chromium.org, ojan@chromium.org, e...@chromium.org
Components:
OS: All
Pri: 1
Type: Bug-Security


Sign in to add a comment
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=152827749

Fuzzer: Inferno_twister_custom_bundle

Crash Type: Heap-buffer-overflow READ 4
Crash Address: 0x7feeb40a3e58
Crash State:
  - crash stack -
  WebCore::FontCache::releaseFontData
  WebCore::FontFallbackList::invalidate
  WebCore::Font::update
  
Regressed: https://cluster-fuzz.appspot.com/revisions?range=152255:152260

Minimized Testcase (1.14 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97pFoAZZ16DMjXvekbA-wNKacZliSutMUm2jYsXP9BriraCxX3NuueVW3YONSdtbdF8ESiDJ2-aZ83Qvo7CAl0ti374dUoNMBm7SV0T5VHmL7aZ6O92rJIg1BECMIbf8ZPX0ICuizNRkL4YK9zXYjixww-Ar4WnIEMnvrQ0n3D6a5-7Mv4
 
Cc: pdr@chromium.org jchaffraix@chromium.org schenney@chromium.org fmalita@chromium.org
Owner: joh...@chromium.org
Status: Assigned
Looks like a regression from https://trac.webkit.org/changeset/125925/.

Johnme@, can you please take a look.
Confirming that this is related to Text Autosizing. If you change ENABLE_TEXT_AUTOSIZING from 1 to 0 in third_party/WebKit/Source/WebKit/chromium/features.gypi the crash no longer occurs.

I'll try and work out what exactly is causing this, starting with that patch.
Cc: e...@chromium.org ojan@chromium.org
Actually that's wrong - it also happens if ENABLE_TEXT_AUTOSIZING is 0; I got confused because the crash only sometimes reproduces (about one in every three times I start chrome with that minimized testcase [on 26.0.1374.0 (174801)]).

This has indeed regressed due to wkrev.com/125925. It allowed the specifiedSize and computedSize of RenderStyle's FontDescription to get set to arbitrary float font sizes (whereas previously they were only set to ints).

I've uploaded a patch that undoes that change, and prevents the crash from occuring on this testcase: wkbug.com/106014. I've cc'd ojan and eae who took at look at the patch already.

However there are some subtleties. For example in the patch above, I used "lroundf", which rounds and casts to long (and these longs are then cast back to floats as specifiedSize and computedSize are stored as floats). If I instead use roundf, the crash still occurs. Now, casting a rounded float to long and back shouldn't normally make much difference - so I suspect what's really going on is that setFontSize is getting passed a NaN or infinite float (perhaps because of the -webkit-animation-iteration-count: 0 ?), and casting to int prevents this from causing problems later.

In fact, I've now confirmed this. If I add ASSERT(isfinite(size)) at the beginning of setFontSize it hits the assert on that page, due to being passed NaN. Stacktrace attached.

So I guess we should fix CSSPropertyAnimation to not divide by zero here! And wkbug.com/106014 can probably be dropped once that's done.

(aside I wonder if SATURATED_LAYOUT_ARITHMETIC (bug 95053) would help this kind of problem? Though I guess font sizes aren't stored as LayoutUnits...)
stacktrace.txt
3.7 KB View Download
johnme@, please make sure that font size are clamped to this number. see http://trac.webkit.org/changeset/123076/trunk/Source/WebCore/css/StyleResolver.cpp. Also, we should add a hard return for release build. We see when this assert ASSERTION FAILED: it != gFontDataCache->end() fails, the next line we try to access stuff out of bounds. Instead we should bail out.
Labels: WebKit-ID-106014 WebKit-ID-106104
Uploaded two patches:
- http://wkbug.com/106014 clamps the font sizes
- http://wkbug.com/106104 prevents out of bounds access

Both patches single-handledly prevent the crash. But seems reasonable to commit both of them, so the fix is less fragile.
By the way, I didn't fix the animation code producing NaNs in the first place. I debugged this, and the root of the problem seems to be in KeyframeAnimation::fetchIntervalEndpointsForProperty. On that test case:

1. |m_animation->iterationCount()| is 0
2. Hence |fractionalTime| gets computed as 1
3. Hence the loop never satisfies the condition |fractionalTime < currKeyFrame.key()|
4. Hence |prevIndex| gets set to the last key frame containing |property|, and |nextIndex| gets set to |m_keyframes.size() - 1|
5. In the test case, these evaluate to the same index (1), hence the line |scale = 1.0 / (nextKeyframe.key() - prevKeyframe.key());| causes scale to become infinite.
6. The infinity gets passed from KeyframeAnimation::animate to CSSPropertyAnimation::blendProperties to PropertyWrapper<float>::blend (page/animation/CSSPropertyAnimation.cpp:379).
7. This function divides by the infinity, and passes a NaN to RenderStyle::setFontSize.

Unfortunately I don't know enough about the animation code to find the cleanest place to fix this. In general all of the logic for the iterationCount == 0 case probably needs checking through.

It does seem that it would be worth getting someone familiar with the animation code to do this, as you can animate things other than font sizes, and it's likely that other crashes can be caused by passing NaNs around...
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Status: Fixed
http://trac.webkit.org/changeset/138812 should fix the security part. We can leave the other upstream bug for extra defense-in-depth.
Comment 8 by pdr@chromium.org, Jan 4 2013
I've fixed a few bugs with NaNs and Infinities as well, and I'm surprised we don't have a cleaner fix for these.
http://trac.webkit.org/changeset/132724 is one such fix recently.
inferno@, are you planning to open a new bug for root cause of the animation code producing NaNs (see comment 6)?
johnme@, i don't know the animations code well enough to see the problem. I dont think it will cause a security problem after r138812.
inferno@, you can animate all sorts of things other than font sizes (opacities, colors, positions, sizes, etc), and the bug in the animation code doesn't seem specific to font sizes, so this could cause similar problems in many other areas of WebKit in the likely event that they aren't NaN-safe.
Project Member Comment 12 by clusterf...@chromium.org, Jan 8 2013
ClusterFuzz has detected this issue as fixed in range 175222:175424.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=152827749

Fuzzer: Inferno_twister_custom_bundle

Crash Type: Heap-buffer-overflow READ 4
Crash Address: 0x7feeb40a3e58
Crash State:
  - crash stack -
  WebCore::FontCache::releaseFontData
  WebCore::FontFallbackList::invalidate
  WebCore::Font::update
  
Regressed: https://cluster-fuzz.appspot.com/revisions?range=152255:152260
Fixed: https://cluster-fuzz.appspot.com/revisions?range=175222:175424

Minimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv97pFoAZZ16DMjXvekbA-wNKacZliSutMUm2jYsXP9BriraCxX3NuueVW3YONSdtbdF8ESiDJ2-aZ83Qvo7CAl0ti374dUoNMBm7SV0T5VHmL7aZ6O92rJIg1BECMIbf8ZPX0ICuizNRkL4YK9zXYjixww-Ar4WnIEMnvrQ0n3D6a5-7Mv4

If you suspect that the result above is incorrect, try re-doing that job on the testcase report page.
Labels: -Mstone-23 -Merge-Approved Mstone-25 Merge-Merged Release-0 Release-Private
http://trac.webkit.org/changeset/138812 merge M25: http://trac.webkit.org/changeset/139397
Project Member Comment 14 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Area-WebKit -Type-Security -SecSeverity-Medium -SecImpacts-Stable -Mstone-25 -SecImpacts-Beta -Stability-AddressSanitizer Cr-Content Performance-Memory-AddressSanitizer Security-Impact-Stable Security-Impact-Beta Security-Severity-Medium Type-Bug-Security M-25
Project Member Comment 15 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member Comment 16 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Severity-Medium Security_Severity-Medium
Project Member Comment 17 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member Comment 18 by bugdroid1@chromium.org, Apr 1 2013
Labels: -Performance-Memory-AddressSanitizer Stability-Memory-AddressSanitizer
Project Member Comment 19 by bugdroid1@chromium.org, Apr 5 2013
Labels: -Cr-Content Cr-Blink
Labels: -Restrict-View-SecurityNotify
Bulk release of old security bug reports.

Project Member Comment 21 by sheriffbot@chromium.org, Jun 14
Labels: -security_impact-beta
Sign in to add a comment