Changeset 206244 in webkit

Timestamp:

Sep 21, 2016 6:23:26 PM (8 years ago)

Author:

Brent Fulgham

Message:

Correct uses of 'safeCast'
​https://bugs.webkit.org/show_bug.cgi?id=162301
<rdar://problem/28343658>

Reviewed by Antti Koivisto.

Source/WebCore:

A number of integer calculations in BitmapImage and PDFDocumentImage
are not properly checked for overflow. Correct this.

Tested by fast/images/large-size-image-crash.html

• loader/cache/MemoryCache.cpp:

(WebCore::MemoryCache::adjustSize): RELEASE_ASSERT on overflow.

• platform/graphics/BitmapImage.cpp:

(WebCore::BitmapImage::destroyMetadataAndNotify):
(WebCore::BitmapImage::cacheFrame):
(WebCore::BitmapImage::didDecodeProperties):
(WebCore::BitmapImage::dataChanged):
(WebCore::BitmapImage::ensureFrameAtIndexIsCached):
(WebCore::BitmapImage::frameImageAtIndex):

• platform/graphics/BitmapImage.h:
• platform/graphics/cg/PDFDocumentImage.cpp:

(WebCore::PDFDocumentImage::decodedSizeChanged):
(WebCore::PDFDocumentImage::updateCachedImageIfNeeded):

Source/WTF:

• wtf/StdLibExtras.h:

(WTF::safeCast): RELEASE_ASSERT on overflow.

Location:

trunk/Source

Files:

• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/StdLibExtras.h (modified) (1 diff)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/loader/cache/MemoryCache.cpp (modified) (1 diff)
• WebCore/platform/graphics/BitmapImage.cpp (modified) (10 diffs)
• WebCore/platform/graphics/cg/PDFDocumentImage.cpp (modified) (5 diffs)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2016-09-20  Brent Fulgham  <bfulgham@apple.com>
+
+        Correct uses of 'safeCast'
+        https://bugs.webkit.org/show_bug.cgi?id=162301
+        <rdar://problem/28343658>
+
+        Reviewed by Antti Koivisto.
+
+        * wtf/StdLibExtras.h:
+        (WTF::safeCast): RELEASE_ASSERT on overflow.
+
 2016-09-21  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/WTF/wtf/StdLibExtras.h

@@ -160 +160 @@
 inline ToType safeCast(FromType value)
 {
-    ASSERT(isInBounds<ToType>(value));
+    RELEASE_ASSERT(isInBounds<ToType>(value));
     return static_cast<ToType>(value);
 }

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-09-20  Brent Fulgham  <bfulgham@apple.com>
+
+        Correct uses of 'safeCast'
+        https://bugs.webkit.org/show_bug.cgi?id=162301
+        <rdar://problem/28343658>
+
+        Reviewed by Antti Koivisto.
+
+        A number of integer calculations in BitmapImage and PDFDocumentImage
+        are not properly checked for overflow. Correct this.
+
+        Tested by fast/images/large-size-image-crash.html
+
+        * loader/cache/MemoryCache.cpp:
+        (WebCore::MemoryCache::adjustSize): RELEASE_ASSERT on overflow.
+        * platform/graphics/BitmapImage.cpp:
+        (WebCore::BitmapImage::destroyMetadataAndNotify):
+        (WebCore::BitmapImage::cacheFrame):
+        (WebCore::BitmapImage::didDecodeProperties):
+        (WebCore::BitmapImage::dataChanged):
+        (WebCore::BitmapImage::ensureFrameAtIndexIsCached):
+        (WebCore::BitmapImage::frameImageAtIndex):
+        * platform/graphics/BitmapImage.h:
+        * platform/graphics/cg/PDFDocumentImage.cpp:
+        (WebCore::PDFDocumentImage::decodedSizeChanged):
+        (WebCore::PDFDocumentImage::updateCachedImageIfNeeded):
+
 2016-09-21  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebCore/loader/cache/MemoryCache.cpp

@@ -645 +645 @@
 {
     if (live) {
-        ASSERT(delta >= 0 || ((int)m_liveSize + delta >= 0));
+        RELEASE_ASSERT(delta >= 0 || ((int)m_liveSize + delta >= 0));
         m_liveSize += delta;
     } else {
-        ASSERT(delta >= 0 || ((int)m_deadSize + delta >= 0));
+        RELEASE_ASSERT(delta >= 0 || ((int)m_deadSize + delta >= 0));
         m_deadSize += delta;
     }

trunk/Source/WebCore/platform/graphics/BitmapImage.cpp

@@ -1 +1 @@
 /*
  * Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
- * Copyright (C) 2004, 2005, 2006, 2008, 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2006, 2008, 2015-2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -37 +37 @@
 #include "TextStream.h"
 #include "Timer.h"
+#include <wtf/CheckedArithmetic.h>
 #include <wtf/CurrentTime.h>
 #include <wtf/Vector.h>
@@ -143 +144 @@
     invalidatePlatformData();
 
-    ASSERT(m_decodedSize >= frameBytesCleared);
-    m_decodedSize -= frameBytesCleared;
+    if (!WTF::safeSub(m_decodedSize, frameBytesCleared, m_decodedSize))
+        CRASH_WITH_SECURITY_IMPLICATION();
 
     // Clearing the ImageSource destroys the extra decoded data used for determining image properties.
+    long long adjustedFrameBytesCleared = frameBytesCleared;
     if (clearedSource == ClearedSource::Yes) {
-        frameBytesCleared += m_decodedPropertiesSize;
+        adjustedFrameBytesCleared += m_decodedPropertiesSize;
         m_decodedPropertiesSize = 0;
     }
 
-    if (frameBytesCleared && imageObserver())
-        imageObserver()->decodedSizeChanged(this, -safeCast<int>(frameBytesCleared));
+    if (adjustedFrameBytesCleared && imageObserver()) {
+        Checked<int> checkedDelta = adjustedFrameBytesCleared;
+        imageObserver()->decodedSizeChanged(this, -checkedDelta.unsafeGet());
+    }
 }
 
@@ -173 +177 @@
 
     if (m_frames[index].hasNativeImage()) {
-        int deltaBytes = safeCast<int>(m_frames[index].frameBytes());
-        m_decodedSize += deltaBytes;
+        if (!WTF::safeAdd(m_decodedSize, m_frames[index].frameBytes(), m_decodedSize)) {
+            LOG(Images, "BitmapImage %p cacheFrame m_decodedSize overflowed unsigned.", this);
+            destroyDecodedData(false);
+            return;
+        }
+
         // The fully-decoded frame will subsume the partially decoded data used
         // to determine image properties.
-        deltaBytes -= m_decodedPropertiesSize;
+        long long deltaBytes = m_frames[index].frameBytes() - m_decodedPropertiesSize;
         m_decodedPropertiesSize = 0;
+
+        Checked<int, RecordOverflow> checkedDeltaBytes = deltaBytes;
+        if (checkedDeltaBytes.hasOverflowed()) {
+            LOG(Images, "BitmapImage %p cacheFrame deltaBytes=%lld overflowed integer.", this, deltaBytes);
+            destroyDecodedData(false);
+            return;
+        }
+
         if (imageObserver())
-            imageObserver()->decodedSizeChanged(this, deltaBytes);
+            imageObserver()->decodedSizeChanged(this, checkedDeltaBytes.unsafeGet());
     }
 }
@@ -193 +209 @@
         return;
 
-    int deltaBytes = updatedSize - m_decodedPropertiesSize;
+    long long deltaBytes = updatedSize - m_decodedPropertiesSize;
 #if !ASSERT_DISABLED
     bool overflow = updatedSize > m_decodedPropertiesSize && deltaBytes < 0;
@@ -200 +216 @@
 #endif
     m_decodedPropertiesSize = updatedSize;
-    if (imageObserver())
-        imageObserver()->decodedSizeChanged(this, deltaBytes);
+    if (imageObserver()) {
+        Checked<int> checkedDeltaBytes = deltaBytes;
+        imageObserver()->decodedSizeChanged(this, checkedDeltaBytes.unsafeGet());
+    }
 }
 
@@ -257 +275 @@
     // frame affected by appending new data here. Thus we have to clear all the
     // incomplete frames to be safe.
-    unsigned frameBytesCleared = 0;
+    Checked<unsigned> frameBytesCleared = 0;
     for (auto& frame : m_frames) {
         // NOTE: Don't call frameIsCompleteAtIndex() here, that will try to
@@ -265 +283 @@
             frameBytesCleared += frame.clear();
     }
-    destroyMetadataAndNotify(frameBytesCleared, ClearedSource::No);
+    destroyMetadataAndNotify(frameBytesCleared.unsafeGet(), ClearedSource::No);
 #else
     // FIXME: why is this different for iOS?
-    int deltaBytes = 0;
+    Checked<int> deltaBytes = 0;
     if (!m_frames.isEmpty()) {
         if (int bytes = m_frames[m_frames.size() - 1].clear()) {
@@ -276 +294 @@
         }
     }
-    destroyMetadataAndNotify(deltaBytes, ClearedSource::No);
+    destroyMetadataAndNotify(deltaBytes.unsafeGet(), ClearedSource::No);
 #endif
     
@@ -357 +375 @@
 
         // If the image is already cached, but at too small a size, re-decode a larger version.
-        int sizeChange = -m_frames[index].clear();
+        unsigned sizeChange = m_frames[index].clear();
         invalidatePlatformData();
-        m_decodedSize += sizeChange;
+
+        if (WTF::safeSub(m_decodedSize, sizeChange, m_decodedSize)) {
+            LOG(Images, "BitmapImage %p frameImageAtIndex m_decodedSize overflowed unsigned.", this);
+            destroyDecodedData(false);
+            return nullptr;
+        }
+
+        Checked<int, RecordOverflow> checkedSizeChange = -sizeChange;
+        if (checkedSizeChange.hasOverflowed()) {
+            LOG(Images, "BitmapImage %p frameImageAtIndex sizeChange=%u overflowed integer.", this, -sizeChange);
+            destroyDecodedData(false);
+            return nullptr;
+        }
+
         if (imageObserver())
-            imageObserver()->decodedSizeChanged(this, sizeChange);
+            imageObserver()->decodedSizeChanged(this, checkedSizeChange.unsafeGet());
     }
 

trunk/Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2004, 2005, 2006, 2013 Apple Inc.  All rights reserved.
+ * Copyright (C) 2004-2016 Apple Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -39 +39 @@
 #include "IntRect.h"
 #include "Length.h"
+#include "Logging.h"
 #include "SharedBuffer.h"
 #include "TextStream.h"
 #include <CoreGraphics/CGContext.h>
 #include <CoreGraphics/CGPDFDocument.h>
+#include <wtf/CheckedArithmetic.h>
 #include <wtf/MathExtras.h>
 #include <wtf/RAMSize.h>
@@ -182 +184 @@
         return;
 
+    long long deltaBytes = m_cachedBytes - newCachedBytes;
+
+    Checked<int> checkedDeltaBytes = deltaBytes;
     if (imageObserver())
-        imageObserver()->decodedSizeChanged(this, -safeCast<int>(m_cachedBytes) + newCachedBytes);
+        imageObserver()->decodedSizeChanged(this, -checkedDeltaBytes.unsafeGet());
 
     ASSERT(s_allDecodedDataSize >= m_cachedBytes);
     // Update with the difference in two steps to avoid unsigned underflow subtraction.
-    s_allDecodedDataSize -= m_cachedBytes;
-    s_allDecodedDataSize += newCachedBytes;
+    if (!WTF::safeSub(s_allDecodedDataSize, m_cachedBytes, s_allDecodedDataSize))
+        CRASH_WITH_SECURITY_IMPLICATION();
+
+    if (!WTF::safeAdd(s_allDecodedDataSize, newCachedBytes, s_allDecodedDataSize))
+        CRASH_WITH_SECURITY_IMPLICATION();
 
     m_cachedBytes = newCachedBytes;
@@ -236 +244 @@
     if (m_pdfImageCachingPolicy == PDFImageCachingBelowMemoryLimit) {
         IntSize scaledSize = ImageBuffer::compatibleBufferSize(cachedImageSize, context);
-        if (s_allDecodedDataSize + safeCast<size_t>(scaledSize.width()) * scaledSize.height() * 4 - m_cachedBytes > s_maxDecodedDataSize) {
+        Checked<size_t, RecordOverflow> scaledBytes = scaledSize.area() * 4;
+
+        if (scaledBytes.hasOverflowed()) {
+            LOG(Images, "PDFDocumentImage %p updateCachedImageIfNeeded scaledBytes overflowed size_t.", this);
+            destroyDecodedData();
+            return;
+        }
+
+        Checked<size_t, RecordOverflow> potentialDecodedDataSize = s_allDecodedDataSize + scaledBytes - m_cachedBytes;
+        if (potentialDecodedDataSize.hasOverflowed() || potentialDecodedDataSize.unsafeGet() > s_maxDecodedDataSize) {
+            LOG(Images, "PDFDocumentImage %p updateCachedImageIfNeeded potentialDecodedDataSize overflowed size_t.", this);
             destroyDecodedData();
             return;
@@ -260 +278 @@
 
     IntSize internalSize = m_cachedImageBuffer->internalSize();
-    decodedSizeChanged(safeCast<size_t>(internalSize.width()) * internalSize.height() * 4);
+    Checked<size_t, RecordOverflow> scaledBytes = internalSize.area() * 4;
+    if (scaledBytes.hasOverflowed()) {
+        LOG(Images, "PDFDocumentImage %p updateCachedImageIfNeeded scaledBytes overflowed size_t.", this);
+        destroyDecodedData();
+        return;
+    }
+
+    decodedSizeChanged(scaledBytes.unsafeGet());
 }