CVE-2020-1914: Always-Incorrect Control Flow Implementation (CWE-670)
value: A logic vulnerability when handling the SaveGeneratorLong instruction in Facebook Hermes prior to commit b2021df620824627f5a8c96615edbd1eb7fdddfc allows attackers to potentially read out of bounds or theoretically execute arbitrary code 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: The code contains a control flow path that does not reflect the algorithm that the path is intended to implement, leading to incorrect behavior any time this path is navigated.
CVSS v3 base score: 9.8
Published: 2020-10-08T19:15Z
Repository: https://github.com/facebook/hermes
Language: C++
Files changed: include/hermes/VM/Interpreter.h, lib/VM/Interpreter-slowpaths.cpp, lib/VM/Interpreter.cpp, test/hermes/es6/generator.js
Commit date: 2020-10-01 09:40:07 -0700
Commit message: Fix CVE-2020-1914 by using NEXTINST for SaveGeneratorLong

Summary:
If `SaveGeneratorLong` was emitted, it would accidentally jump to the
wrong next instruction, based on how long SaveGenerator was.

Make a callout function to handle the common case, and handle the dispatch
within each case of the interpreter loop.

Fixes CVE-2020-1914

Reviewed By: neildhar

Differential Revision: D24024242

fbshipit-source-id: 3bcb88daa740f0d50e91771a49eb212551ce8bd8

Diff: diff --git a/include/hermes/VM/Interpreter.h b/include/hermes/VM/Interpreter.h
index 83a86d6ba..766e43810 100644
--- a/include/hermes/VM/Interpreter.h
+++ b/include/hermes/VM/Interpreter.h
@@ -35,10 +35,18 @@ class Interpreter {
       RuntimeModule *runtimeModule,
       unsigned funcIndex,
       Handle<Environment> envHandle,
       NativeArgs args);
 
+  /// Suspend the generator function and yield to the caller.
+  /// \param resumeIP Is the IP where the generator should resume from when it
+  ///   is resumed.
+  static void saveGenerator(
+      Runtime *runtime,
+      PinnedHermesValue *frameRegs,
+      const Inst *resumeIP);
+
   /// Slow path for ReifyArguments resReg, lazyReg
   /// It assumes that he fast path has handled the case when 'lazyReg' is
   /// already initialized. It creates a new 'arguments' object and populates it
   /// with the argument values.
   static CallResult<Handle<Arguments>> reifyArgumentsSlowPath(
diff --git a/lib/VM/Interpreter-slowpaths.cpp b/lib/VM/Interpreter-slowpaths.cpp
index 95f37da76..26bcd4327 100644
--- a/lib/VM/Interpreter-slowpaths.cpp
+++ b/lib/VM/Interpreter-slowpaths.cpp
@@ -7,19 +7,30 @@
 
 #define DEBUG_TYPE "vm"
 #include "JSLib/JSLibInternal.h"
 #include "hermes/VM/Casting.h"
 #include "hermes/VM/Interpreter.h"
+#include "hermes/VM/StackFrame-inline.h"
 #include "hermes/VM/StringPrimitive.h"
 
 #include "Interpreter-internal.h"
 
 using namespace hermes::inst;
 
 namespace hermes {
 namespace vm {
 
+void Interpreter::saveGenerator(
+    Runtime *runtime,
+    PinnedHermesValue *frameRegs,
+    const Inst *resumeIP) {
+  auto *innerFn = vmcast<GeneratorInnerFunction>(FRAME.getCalleeClosure());
+  innerFn->saveStack(runtime);
+  innerFn->setNextIP(resumeIP);
+  innerFn->setState(GeneratorInnerFunction::State::SuspendedYield);
+}
+
 ExecutionStatus Interpreter::caseDirectEval(
     Runtime *runtime,
     PinnedHermesValue *frameRegs,
     const Inst *ip) {
   auto *result = &O1REG(DirectEval);
diff --git a/lib/VM/Interpreter.cpp b/lib/VM/Interpreter.cpp
index 7886dc714..31bc66c35 100644
--- a/lib/VM/Interpreter.cpp
+++ b/lib/VM/Interpreter.cpp
@@ -999,10 +999,20 @@ CallResult<HermesValue> Interpreter::interpretFunction(
   ip = runtime->getCurrentIP();      \
   runtime->invalidateCurrentIP();
 
 #endif // NDEBUG
 
+/// \def DONT_CAPTURE_IP(expr)
+/// \param expr A call expression to a function external to the interpreter. The
+///   expression should not make any allocations and the IP should be set
+///   immediately following this macro.
+#define DONT_CAPTURE_IP(expr)      \
+  do {                             \
+    NoAllocScope noAlloc(runtime); \
+    (void)expr;                    \
+  } while (false)
+
   LLVM_DEBUG(dbgs() << "interpretFunction() called\n");
 
   ScopedNativeDepthTracker depthTracker{runtime};
   if (LLVM_UNLIKELY(depthTracker.overflowed())) {
     return runtime->raiseStackOverflow(Runtime::StackOverflowKind::NativeStack);
@@ -1796,29 +1806,22 @@ tailCall:
         ip = NEXTINST(CompleteGenerator);
         DISPATCH;
       }
 
       CASE(SaveGenerator) {
-        nextIP = IPADD(ip->iSaveGenerator.op1);
-        goto doSaveGen;
+        DONT_CAPTURE_IP(
+            saveGenerator(runtime, frameRegs, IPADD(ip->iSaveGenerator.op1)));
+        ip = NEXTINST(SaveGenerator);
+        DISPATCH;
       }
       CASE(SaveGeneratorLong) {
-        nextIP = IPADD(ip->iSaveGeneratorLong.op1);
-        goto doSaveGen;
+        DONT_CAPTURE_IP(saveGenerator(
+            runtime, frameRegs, IPADD(ip->iSaveGeneratorLong.op1)));
+        ip = NEXTINST(SaveGeneratorLong);
+        DISPATCH;
       }
 
-    doSaveGen : {
-      auto *innerFn = vmcast<GeneratorInnerFunction>(
-          runtime->getCurrentFrame().getCalleeClosure());
-
-      innerFn->saveStack(runtime);
-      innerFn->setNextIP(nextIP);
-      innerFn->setState(GeneratorInnerFunction::State::SuspendedYield);
-      ip = NEXTINST(SaveGenerator);
-      DISPATCH;
-    }
-
       CASE(StartGenerator) {
         auto *innerFn = vmcast<GeneratorInnerFunction>(
             runtime->getCurrentFrame().getCalleeClosure());
         if (innerFn->getState() ==
             GeneratorInnerFunction::State::SuspendedStart) {
diff --git a/test/hermes/es6/generator.js b/test/hermes/es6/generator.js
index 2cbe3c2fd..9513d7893 100644
--- a/test/hermes/es6/generator.js
+++ b/test/hermes/es6/generator.js
@@ -352,5 +352,20 @@ function* generator() {
 var iterator = generator();
 print(iterator.next().value);
 iterator.return(123);
 // CHECK-NEXT: 1
 // CHECK-NEXT: get return
+
+// Make sure using SaveGeneratorLong works.
+function* saveGeneratorLong() {
+    yield* [1];
+    // Waste some registers, to change SaveGenerator to SaveGeneratorLong.
+    [].push(0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0);
+}
+print(saveGeneratorLong().next().value);
+// CHECK-NEXT: 1

Vulnerable code:         nextIP = IPADD(ip->iSaveGenerator.op1);
        goto doSaveGen;
        nextIP = IPADD(ip->iSaveGeneratorLong.op1);
        goto doSaveGen;
    doSaveGen : {
      auto *innerFn = vmcast<GeneratorInnerFunction>(
          runtime->getCurrentFrame().getCalleeClosure());

      innerFn->saveStack(runtime);
      innerFn->setNextIP(nextIP);
      innerFn->setState(GeneratorInnerFunction::State::SuspendedYield);
      ip = NEXTINST(SaveGenerator);
      DISPATCH;
    }

Fixed code:   /// Suspend the generator function and yield to the caller.
  /// \param resumeIP Is the IP where the generator should resume from when it
  ///   is resumed.
  static void saveGenerator(
      Runtime *runtime,
      PinnedHermesValue *frameRegs,
      const Inst *resumeIP);

#include "hermes/VM/StackFrame-inline.h"
void Interpreter::saveGenerator(
    Runtime *runtime,
    PinnedHermesValue *frameRegs,
    const Inst *resumeIP) {
  auto *innerFn = vmcast<GeneratorInnerFunction>(FRAME.getCalleeClosure());
  innerFn->saveStack(runtime);
  innerFn->setNextIP(resumeIP);
  innerFn->setState(GeneratorInnerFunction::State::SuspendedYield);
}

/// \def DONT_CAPTURE_IP(expr)
/// \param expr A call expression to a function external to the interpreter. The
///   expression should not make any allocations and the IP should be set
///   immediately following this macro.
#define DONT_CAPTURE_IP(expr)      \
  do {                             \
    NoAllocScope noAlloc(runtime); \
    (void)expr;                    \
  } while (false)

        DONT_CAPTURE_IP(
            saveGenerator(runtime, frameRegs, IPADD(ip->iSaveGenerator.op1)));
        ip = NEXTINST(SaveGenerator);
        DISPATCH;
        DONT_CAPTURE_IP(saveGenerator(
            runtime, frameRegs, IPADD(ip->iSaveGeneratorLong.op1)));
        ip = NEXTINST(SaveGeneratorLong);
        DISPATCH;

// Make sure using SaveGeneratorLong works.
function* saveGeneratorLong() {
    yield* [1];
    // Waste some registers, to change SaveGenerator to SaveGeneratorLong.
    [].push(0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0);
}
print(saveGeneratorLong().next().value);
// CHECK-NEXT: 1