CVE-2020-1913: Incorrect Conversion between Numeric Types (CWE-681)
value: An Integer signedness error in the JavaScript Interpreter in Facebook Hermes prior to commit 2c7af7ec481ceffd0d14ce2d7c045e475fd71dc6 allows attackers to cause a denial of service attack or a potential RCE via crafted JavaScript. Note that this is only exploitable if the application using Hermes permits evaluation of untrusted JavaScript. Hence, most React Native applications are not affected.
CWE description: When converting from one data type to another, such as long to integer, data can be omitted or translated in a way that produces unexpected values. If the resulting values are used in a sensitive context, then dangerous behaviors may occur.
CVSS v3 base score: 8.1
Published: 2020-09-09T19:15Z
Repository: https://github.com/facebook/hermes
Language: JavaScript
Files changed: lib/VM/Interpreter.cpp, test/Optimizer/switch_stmt.js, test/hermes/switch-regress1.js
Commit date: 2020-08-14 13:50:33 -0700
Commit message: Fix backward branching in SwitchImm.

Summary:
`SwitchImm` can subtract from `ip`, but it was reading the offset
from a `uint32_t *` instead of `int32_t *`.
Change the type to the correct one.

Reviewed By: dulinriley

Differential Revision: D23114516

fbshipit-source-id: d4ddab94eb16dc1ce479590851eeb1ab428bd5d6

Diff: diff --git a/lib/VM/Interpreter.cpp b/lib/VM/Interpreter.cpp
index ba9b37a19..fde941022 100644
--- a/lib/VM/Interpreter.cpp
+++ b/lib/VM/Interpreter.cpp
@@ -3385,12 +3385,13 @@ tailCall:
             // this SwitchImm starts.
             const uint8_t *tablestart = (const uint8_t *)llvh::alignAddr(
                 (const uint8_t *)ip + ip->iSwitchImm.op2, sizeof(uint32_t));
 
             // Read the offset from the table.
-            const uint32_t *loc =
-                (const uint32_t *)tablestart + uintVal - ip->iSwitchImm.op4;
+            // Must be signed to account for backwards branching.
+            const int32_t *loc =
+                (const int32_t *)tablestart + uintVal - ip->iSwitchImm.op4;
 
             ip = IPADD(*loc);
             DISPATCH;
           }
         }
diff --git a/test/Optimizer/switch_stmt.js b/test/Optimizer/switch_stmt.js
new file mode 100644
index 000000000..b42ff203f
--- /dev/null
+++ b/test/Optimizer/switch_stmt.js
@@ -0,0 +1,36 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+// RUN: %hermes -O -dump-ir %s | %FileCheck %s
+
+function backwards_branch() {
+  for (var i = 0; i < 4; i++) {
+    switch (i) {
+      case 0:
+      case 1:
+      case 2:
+      case 3:
+      case 4:
+      case 5:
+      case 6:
+      case 7:
+      case 8:
+      case 9:
+        break;
+    }
+    i = 2;
+  }
+}
+
+// CHECK-LABEL: function backwards_branch() : undefined
+// CHECK-NEXT: frame = []
+// CHECK-NEXT: %BB0:
+// CHECK-NEXT:   %0 = BranchInst %BB1
+// CHECK-NEXT: %BB1:
+// CHECK-NEXT:   %1 = PhiInst 0 : number, %BB0, 3 : number, %BB1
+// CHECK-NEXT:   %2 = SwitchInst %1 : number, %BB1, 0 : number, %BB1, 1 : number, %BB1, 2 : number, %BB1, 3 : number, %BB1, 4 : number, %BB1, 5 : number, %BB1, 6 : number, %BB1, 7 : number, %BB1, 8 : number, %BB1, 9 : number, %BB1
+// CHECK-NEXT: function_end
diff --git a/test/hermes/switch-regress1.js b/test/hermes/switch-regress1.js
new file mode 100644
index 000000000..5df612a7b
--- /dev/null
+++ b/test/hermes/switch-regress1.js
@@ -0,0 +1,43 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+// RUN: %hermes -O0 %s | %FileCheck --match-full-lines %s
+// RUN: %hermes -O %s | %FileCheck --match-full-lines %s
+// RUN: %hermes -O -emit-binary -out %t.hbc %s && %hermes %t.hbc | %FileCheck --match-full-lines %s
+
+// Ensure that switch statements that are backwards branches work properly.
+
+(function() {
+  var j = 0;
+  for (var i = 0; i < 4; i++) {
+    ++j;
+    if (j > 5) {
+      return;
+    }
+    print(i);
+    switch (i) {
+      case 0:
+      case 1:
+      case 2:
+      case 3:
+      case 4:
+      case 5:
+      case 6:
+      case 7:
+      case 8:
+      case 9:
+        break;
+    }
+    i = 2;
+  }
+})();
+
+// CHECK: 0
+// CHECK: 3
+// CHECK: 3
+// CHECK: 3
+// CHECK: 3

Vulnerable code:             const uint32_t *loc =
                (const uint32_t *)tablestart + uintVal - ip->iSwitchImm.op4;
Fixed code:             // Must be signed to account for backwards branching.
            const int32_t *loc =
                (const int32_t *)tablestart + uintVal - ip->iSwitchImm.op4;
/**
 * Copyright (c) Facebook, Inc. and its affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

// RUN: %hermes -O -dump-ir %s | %FileCheck %s

function backwards_branch() {
  for (var i = 0; i < 4; i++) {
    switch (i) {
      case 0:
      case 1:
      case 2:
      case 3:
      case 4:
      case 5:
      case 6:
      case 7:
      case 8:
      case 9:
        break;
    }
    i = 2;
  }
}

// CHECK-LABEL: function backwards_branch() : undefined
// CHECK-NEXT: frame = []
// CHECK-NEXT: %BB0:
// CHECK-NEXT:   %0 = BranchInst %BB1
// CHECK-NEXT: %BB1:
// CHECK-NEXT:   %1 = PhiInst 0 : number, %BB0, 3 : number, %BB1
// CHECK-NEXT:   %2 = SwitchInst %1 : number, %BB1, 0 : number, %BB1, 1 : number, %BB1, 2 : number, %BB1, 3 : number, %BB1, 4 : number, %BB1, 5 : number, %BB1, 6 : number, %BB1, 7 : number, %BB1, 8 : number, %BB1, 9 : number, %BB1
// CHECK-NEXT: function_end
/**
 * Copyright (c) Facebook, Inc. and its affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

// RUN: %hermes -O0 %s | %FileCheck --match-full-lines %s
// RUN: %hermes -O %s | %FileCheck --match-full-lines %s
// RUN: %hermes -O -emit-binary -out %t.hbc %s && %hermes %t.hbc | %FileCheck --match-full-lines %s

// Ensure that switch statements that are backwards branches work properly.

(function() {
  var j = 0;
  for (var i = 0; i < 4; i++) {
    ++j;
    if (j > 5) {
      return;
    }
    print(i);
    switch (i) {
      case 0:
      case 1:
      case 2:
      case 3:
      case 4:
      case 5:
      case 6:
      case 7:
      case 8:
      case 9:
        break;
    }
    i = 2;
  }
})();

// CHECK: 0
// CHECK: 3
// CHECK: 3
// CHECK: 3
// CHECK: 3