[turbofan] Harden ArrayPrototypePop and ArrayPrototypeShift

sergeiglazunov · Commit Bot · commit d4aafa4022b7 · 2021-04-15T11:02:10.000Z
An exploitation technique that abuses `pop` and `shift` to create a JS array with a negative length was publicly disclosed some time ago. Add extra checks to break the technique. Bug: chromium:1198696 Change-Id: Ie008e9ae60bbdc3b25ca3a986d3cdc5e3cc00431 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2823707 Reviewed-by: Georg Neis <neis@chromium.org> Commit-Queue: Sergei Glazunov <glazunov@google.com> Cr-Commit-Position: refs/heads/master@{#73973}
diff --git a/src/compiler/js-call-reducer.cc b/src/compiler/js-call-reducer.cc
@@ -5393,24 +5393,31 @@ Reduction JSCallReducer::ReduceArrayPrototypePop(Node* node) {
       }
 
       // Compute the new {length}.
-      length = graph()->NewNode(simplified()->NumberSubtract(), length,
-                                jsgraph()->OneConstant());
+      Node* new_length = graph()->NewNode(simplified()->NumberSubtract(),
+                                          length, jsgraph()->OneConstant());
+
+      // This extra check exists solely to break an exploitation technique
+      // that abuses typer mismatches.
+      new_length = efalse = graph()->NewNode(
+          simplified()->CheckBounds(p.feedback(),
+                                    CheckBoundsFlag::kAbortOnOutOfBounds),
+          new_length, length, efalse, if_false);
 
       // Store the new {length} to the {receiver}.
       efalse = graph()->NewNode(
           simplified()->StoreField(AccessBuilder::ForJSArrayLength(kind)),
-          receiver, length, efalse, if_false);
+          receiver, new_length, efalse, if_false);
 
       // Load the last entry from the {elements}.
       vfalse = efalse = graph()->NewNode(
           simplified()->LoadElement(AccessBuilder::ForFixedArrayElement(kind)),
-          elements, length, efalse, if_false);
+          elements, new_length, efalse, if_false);
 
       // Store a hole to the element we just removed from the {receiver}.
       efalse = graph()->NewNode(
           simplified()->StoreElement(
               AccessBuilder::ForFixedArrayElement(GetHoleyElementsKind(kind))),
-          elements, length, jsgraph()->TheHoleConstant(), efalse, if_false);
+          elements, new_length, jsgraph()->TheHoleConstant(), efalse, if_false);
     }
 
     control = graph()->NewNode(common()->Merge(2), if_true, if_false);
@@ -5586,19 +5593,27 @@ Reduction JSCallReducer::ReduceArrayPrototypeShift(Node* node) {
         }
 
         // Compute the new {length}.
-        length = graph()->NewNode(simplified()->NumberSubtract(), length,
-                                  jsgraph()->OneConstant());
+        Node* new_length = graph()->NewNode(simplified()->NumberSubtract(),
+                                            length, jsgraph()->OneConstant());
+
+        // This extra check exists solely to break an exploitation technique
+        // that abuses typer mismatches.
+        new_length = etrue1 = graph()->NewNode(
+            simplified()->CheckBounds(p.feedback(),
+                                      CheckBoundsFlag::kAbortOnOutOfBounds),
+            new_length, length, etrue1, if_true1);
 
         // Store the new {length} to the {receiver}.
         etrue1 = graph()->NewNode(
             simplified()->StoreField(AccessBuilder::ForJSArrayLength(kind)),
-            receiver, length, etrue1, if_true1);
+            receiver, new_length, etrue1, if_true1);
 
         // Store a hole to the element we just removed from the {receiver}.
         etrue1 = graph()->NewNode(
             simplified()->StoreElement(AccessBuilder::ForFixedArrayElement(
                 GetHoleyElementsKind(kind))),
-            elements, length, jsgraph()->TheHoleConstant(), etrue1, if_true1);
+            elements, new_length, jsgraph()->TheHoleConstant(), etrue1,
+            if_true1);
       }
 
       Node* if_false1 = graph()->NewNode(common()->IfFalse(), branch1);
