My favorites | Sign in
Project Home Downloads Wiki Issues Code Search
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 167443: Heap-buffer-overflow in WebCore::FontCache::releaseFontData
  Back to list
 
Project Member Reported by infe...@chromium.org, Dec 22, 2012
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
Jan 2, 2013
#1 infe...@chromium.org
Looks like a regression from https://trac.webkit.org/changeset/125925/.

Johnme@, can you please take a look.
Status: Assigned
Owner: johnme@chromium.org
Cc: jchaffraix@chromium.org schen...@chromium.org pdr@chromium.org fmalita@chromium.org
Jan 3, 2013
#2 johnme@chromium.org
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.
Jan 3, 2013
#3 johnme@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
Cc: o...@chromium.org eae@chromium.org
Jan 3, 2013
#4 infe...@chromium.org
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.
Jan 4, 2013
#5 johnme@chromium.org
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.
Labels: WebKit-ID-106014 WebKit-ID-106104
Jan 4, 2013
#6 johnme@chromium.org
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...
Jan 4, 2013
#7 infe...@chromium.org
http://trac.webkit.org/changeset/138812 should fix the security part. We can leave the other upstream bug for extra defense-in-depth.
Status: Fixed
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Jan 4, 2013
#8 pdr@chromium.org
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.
Jan 4, 2013
#9 johnme@chromium.org
inferno@, are you planning to open a new bug for root cause of the animation code producing NaNs (see comment 6)?
Jan 4, 2013
#10 infe...@chromium.org
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.
Jan 4, 2013
#11 johnme@chromium.org
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.
Jan 8, 2013
Project Member #12 clusterfuzz@chromium.org
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.
Jan 10, 2013
#13 scarybea...@gmail.com
http://trac.webkit.org/changeset/138812 merge M25: http://trac.webkit.org/changeset/139397
Labels: -Mstone-23 -Merge-Approved Mstone-25 Merge-Merged Release-0 Release-Private
Mar 9, 2013
#14 bugdro...@chromium.org
(No comment was entered for this change.)
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
Mar 21, 2013
#15 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Security-Impact-Stable Security_Impact-Stable
Mar 21, 2013
#16 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Security-Severity-Medium Security_Severity-Medium
Mar 21, 2013
#17 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Security-Impact-Beta Security_Impact-Beta
Apr 1, 2013
#18 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Performance-Memory-AddressSanitizer Stability-Memory-AddressSanitizer
Apr 5, 2013
#19 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Cr-Content Cr-Blink
Nov 18, 2013
#20 jschuh@chromium.org
Bulk release of old security bug reports.

Labels: -Restrict-View-SecurityNotify
Sign in to add a comment

Powered by Google Project Hosting