Out-of-bounds accesses due to a missing check for MAX_STORAGE_VECTOR_LENGTH in unshif...
authorrmorisset@apple.com <rmorisset@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Mar 2018 12:05:34 +0000 (12:05 +0000)
committerrmorisset@apple.com <rmorisset@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Mar 2018 12:05:34 +0000 (12:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183657
JSTests:

Reviewed by Keith Miller.

* stress/large-unshift-splice.js: Added.
(make_contig_arr):

Source/JavaScriptCore:

<rdar://problem/38464399>

Reviewed by Keith Miller.

There was just a missing check in unshiftCountForIndexingType.
I've also replaced 'return false' by 'return true' in the case of an 'out-of-memory' exception, because 'return false' means 'please continue to the slow path',
and the slow path has an assert that there is no unhandled exception (line 360 of ArrayPrototype.cpp).
Finally, I made the assert in ensureLength a release assert as it would have caught this bug and prevented it from being a security risk.

* runtime/ArrayPrototype.cpp:
(JSC::unshift):
* runtime/JSArray.cpp:
(JSC::JSArray::unshiftCountWithAnyIndexingType):
* runtime/JSObject.h:
(JSC::JSObject::ensureLength):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230101 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/large-unshift-splice.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/ArrayPrototype.cpp
Source/JavaScriptCore/runtime/JSArray.cpp
Source/JavaScriptCore/runtime/JSObject.h

index 2317721..cfb478e 100644 (file)
@@ -1,3 +1,13 @@
+2018-03-30  Robin Morisset  <rmorisset@apple.com>
+
+        Out-of-bounds accesses due to a missing check for MAX_STORAGE_VECTOR_LENGTH in unshiftCountForAnyIndexingType
+        https://bugs.webkit.org/show_bug.cgi?id=183657
+
+        Reviewed by Keith Miller.
+
+        * stress/large-unshift-splice.js: Added.
+        (make_contig_arr):
+
 2018-03-28  Robin Morisset  <rmorisset@apple.com>
 
         appendQuotedJSONString stops on arithmetic overflow instead of propagating it upwards
diff --git a/JSTests/stress/large-unshift-splice.js b/JSTests/stress/large-unshift-splice.js
new file mode 100644 (file)
index 0000000..bccdada
--- /dev/null
@@ -0,0 +1,16 @@
+//@ skip if $memoryLimited
+
+function make_contig_arr(sz)
+{
+    let a = []; 
+    while (a.length < sz / 8)
+        a.push(3.14); 
+    a.length *= 8;
+    return a;
+}
+
+let ARRAY_LENGTH = 0x10000000;
+let a = make_contig_arr(ARRAY_LENGTH);
+let b = make_contig_arr(0xff00);
+b.unshift(a.length-0x10000, 0);
+Array.prototype.splice.apply(a, b);
index 95b2489..74205d1 100644 (file)
@@ -1,3 +1,23 @@
+2018-03-30  Robin Morisset  <rmorisset@apple.com>
+
+        Out-of-bounds accesses due to a missing check for MAX_STORAGE_VECTOR_LENGTH in unshiftCountForAnyIndexingType
+        https://bugs.webkit.org/show_bug.cgi?id=183657
+        <rdar://problem/38464399>
+
+        Reviewed by Keith Miller.
+
+        There was just a missing check in unshiftCountForIndexingType.
+        I've also replaced 'return false' by 'return true' in the case of an 'out-of-memory' exception, because 'return false' means 'please continue to the slow path',
+        and the slow path has an assert that there is no unhandled exception (line 360 of ArrayPrototype.cpp).
+        Finally, I made the assert in ensureLength a release assert as it would have caught this bug and prevented it from being a security risk.
+
+        * runtime/ArrayPrototype.cpp:
+        (JSC::unshift):
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::unshiftCountWithAnyIndexingType):
+        * runtime/JSObject.h:
+        (JSC::JSObject::ensureLength):
+
 2018-03-29  Mark Lam  <mark.lam@apple.com>
 
         Add some pointer profiling support to B3 and Air.
index a295b85..3e2843f 100644 (file)
@@ -348,7 +348,7 @@ void unshift(ExecState* exec, JSObject* thisObj, unsigned header, unsigned curre
     RELEASE_ASSERT(currentCount <= (length - header));
 
     // Guard against overflow.
-    if (count > (UINT_MAX - length)) {
+    if (count > UINT_MAX - length) {
         throwOutOfMemoryError(exec, scope);
         return;
     }
index fd6770d..b473d40 100644 (file)
@@ -1060,10 +1060,13 @@ bool JSArray::unshiftCountWithAnyIndexingType(ExecState* exec, unsigned startInd
             scope.release();
             return unshiftCountWithArrayStorage(exec, startIndex, count, ensureArrayStorage(vm));
         }
-        
+
+        if (oldLength + count > MAX_STORAGE_VECTOR_LENGTH)
+            return false;
+
         if (!ensureLength(vm, oldLength + count)) {
             throwOutOfMemoryError(exec, scope);
-            return false;
+            return true;
         }
         butterfly = this->butterfly();
 
@@ -1104,10 +1107,13 @@ bool JSArray::unshiftCountWithAnyIndexingType(ExecState* exec, unsigned startInd
             scope.release();
             return unshiftCountWithArrayStorage(exec, startIndex, count, ensureArrayStorage(vm));
         }
-        
+
+        if (oldLength + count > MAX_STORAGE_VECTOR_LENGTH)
+            return false;
+
         if (!ensureLength(vm, oldLength + count)) {
             throwOutOfMemoryError(exec, scope);
-            return false;
+            return true;
         }
         butterfly = this->butterfly();
         
index 6c6598e..bed80df 100644 (file)
@@ -982,7 +982,7 @@ protected:
     // the array is contiguous.
     bool WARN_UNUSED_RETURN ensureLength(VM& vm, unsigned length)
     {
-        ASSERT(length <= MAX_STORAGE_VECTOR_LENGTH);
+        RELEASE_ASSERT(length <= MAX_STORAGE_VECTOR_LENGTH);
         ASSERT(hasContiguous(indexingType()) || hasInt32(indexingType()) || hasDouble(indexingType()) || hasUndecided(indexingType()));
 
         if (m_butterfly->vectorLength() < length) {