Changeset 259788 in webkit

Timestamp:

Apr 9, 2020, 3:52:52 AM (6 years ago)

Author:

ddkilzer@apple.com

Message:

Follow-up: WTF::Persistence::VectorCoder and IPC::VectorArgumentCoder should do bounds checking without crashing
<​https://webkit.org/b/210227>
<rdar://problem/60832243>

Reviewed by Alex Christensen.

Source/WebKit:

• Platform/IPC/ArgumentCoders.h:
• Add missing call to decoder.markInvalid() if decoding of decodedSize fails.
• Replace safeCast<size_t> with isInBounds<size_t> so that we don't crash if decodedSize is too big. Instead we fail decoding by marking the decoder invalid and returning early.
• Revert checked arithemtic for multiplication since bufferIsLargeEnoughToContain<T(size) already did this check for us.

Source/WTF:

• wtf/persistence/PersistentCoders.h:

(WTF::Persistence::VectorCoder::decode):

• Replace safeCast<size_t> with isInBounds<size_t> so that we don't crash if decodedSize is too big. Instead we fail decoding by returning early.
• Revert checked arithemtic for multiplication since bufferIsLargeEnoughToContain<T(size) already did this check for us.

Location:

trunk/Source

Files:

• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/persistence/PersistentCoders.h (modified) (2 diffs)
• WebKit/ChangeLog (modified) (1 diff)
• WebKit/Platform/IPC/ArgumentCoders.h (modified) (4 diffs)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2020-04-09  David Kilzer  <ddkilzer@apple.com>
+
+        Follow-up: WTF::Persistence::VectorCoder and IPC::VectorArgumentCoder should do bounds checking without crashing
+        <https://webkit.org/b/210227>
+        <rdar://problem/60832243>
+
+        Reviewed by Alex Christensen.
+
+        * wtf/persistence/PersistentCoders.h:
+        (WTF::Persistence::VectorCoder::decode):
+        - Replace safeCast<size_t> with isInBounds<size_t> so that we
+          don't crash if `decodedSize` is too big.  Instead we fail
+          decoding by returning early.
+        - Revert checked arithemtic for multiplication since
+          bufferIsLargeEnoughToContain<T(size) already did this check
+          for us.
+
 2020-04-09  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/WTF/wtf/persistence/PersistentCoders.h

@@ -161 +161 @@
             return false;
 
-        auto size = safeCast<size_t>(decodedSize);
+        if (!isInBounds<size_t>(decodedSize))
+            return false;
+
+        auto size = static_cast<size_t>(decodedSize);
 
         // Since we know the total size of the elements, we can allocate the vector in
@@ -172 +175 @@
         temp.grow(size);
 
-        Checked<size_t> checkedSize(size);
-        decoder.decodeFixedLengthData(reinterpret_cast<uint8_t*>(temp.data()), (checkedSize * sizeof(T)).unsafeGet());
+        decoder.decodeFixedLengthData(reinterpret_cast<uint8_t*>(temp.data()), size * sizeof(T));
 
         vector.swap(temp);

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2020-04-09  David Kilzer  <ddkilzer@apple.com>
+
+        Follow-up: WTF::Persistence::VectorCoder and IPC::VectorArgumentCoder should do bounds checking without crashing
+        <https://webkit.org/b/210227>
+        <rdar://problem/60832243>
+
+        Reviewed by Alex Christensen.
+
+        * Platform/IPC/ArgumentCoders.h:
+        - Add missing call to decoder.markInvalid() if decoding of
+          `decodedSize` fails.
+        - Replace safeCast<size_t> with isInBounds<size_t> so that we
+          don't crash if `decodedSize` is too big.  Instead we fail
+          decoding by marking the decoder invalid and returning early.
+        - Revert checked arithemtic for multiplication since
+          bufferIsLargeEnoughToContain<T(size) already did this check
+          for us.
+
 2020-04-08  David Kilzer  <ddkilzer@apple.com>
 

trunk/Source/WebKit/Platform/IPC/ArgumentCoders.h

@@ -368 +368 @@
     {
         uint64_t decodedSize;
-        if (!decoder.decode(decodedSize))
-            return false;
-
-        auto size = safeCast<size_t>(decodedSize);
+        if (!decoder.decode(decodedSize)) {
+            decoder.markInvalid();
+            return false;
+        }
+
+        if (!WTF::isInBounds<size_t>(decodedSize)) {
+            decoder.markInvalid();
+            return false;
+        }
+
+        auto size = static_cast<size_t>(decodedSize);
 
         // Since we know the total size of the elements, we can allocate the vector in
@@ -384 +391 @@
         temp.grow(size);
 
-        Checked<size_t> checkedSize(size);
-        if (!decoder.decodeFixedLengthData(reinterpret_cast<uint8_t*>(temp.data()), (checkedSize * sizeof(T)).unsafeGet(), alignof(T))) {
+        if (!decoder.decodeFixedLengthData(reinterpret_cast<uint8_t*>(temp.data()), size * sizeof(T), alignof(T))) {
             decoder.markInvalid();
             return false;
@@ -397 +403 @@
     {
         uint64_t decodedSize;
-        if (!decoder.decode(decodedSize))
-            return WTF::nullopt;
-
-        auto size = safeCast<size_t>(decodedSize);
+        if (!decoder.decode(decodedSize)) {
+            decoder.markInvalid();
+            return WTF::nullopt;
+        }
+
+        if (!WTF::isInBounds<size_t>(decodedSize)) {
+            decoder.markInvalid();
+            return WTF::nullopt;
+        }
+
+        auto size = static_cast<size_t>(decodedSize);
 
         // Since we know the total size of the elements, we can allocate the vector in
@@ -413 +426 @@
         vector.grow(size);
 
-        Checked<size_t> checkedSize(size);
-        if (!decoder.decodeFixedLengthData(reinterpret_cast<uint8_t*>(vector.data()), (checkedSize * sizeof(T)).unsafeGet(), alignof(T))) {
+        if (!decoder.decodeFixedLengthData(reinterpret_cast<uint8_t*>(vector.data()), size * sizeof(T), alignof(T))) {
             decoder.markInvalid();
             return WTF::nullopt;