CVE-2018-11598: Out-of-bounds Read (CWE-125)
value: Espruino before 1.99 allows attackers to cause a denial of service (application crash) and a potential Information Disclosure with user crafted input files via a Buffer Overflow or Out-of-bounds Read during syntax parsing of certain for loops in jsparse.c.
CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 7.1
Published: 2018-05-31T16:29Z
Repository: https://github.com/espruino/Espruino
Language: Other
Files changed: ChangeLog, src/jsparse.c
Commit date: 2018-05-23 17:27:51 +0100
Commit message: Fix bug if using an undefined member of an object for for..in (fix #1437)

Diff: diff --git a/ChangeLog b/ChangeLog
index d91c8fb6d..c60267149 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,10 +1,11 @@
             Allow changeInterval with large (>32 bit) intervals (fix #1438)
             changeInterval now changes the interval immediately when it's called inside the interval it is changing (fix #1440)
             Fix parsing of try..catch when not executing (fix #1439)
             Add extra ReferenceError checks, even if variable is not used
             Allow Ctrl-C out of while...continue loop (fix #1441)
+            Fix bug if using an undefined member of an object for for..in (fix #1437)
 
      1v99 : Increase jslMatch error buffer size to handle "UNFINISHED TEMPLATE LITERAL" string (#1426)
             nRF5x: Make FlashWrite cope with flash writes > 4k
             Increase max size of native strings on platforms that support it - 16 bit to 32 bit (#1432)
             Fix stack size detection on Linux (fix #1427)
diff --git a/src/jsparse.c b/src/jsparse.c
index 136e511dd..e0af90ecc 100644
--- a/src/jsparse.c
+++ b/src/jsparse.c
@@ -120,10 +120,20 @@ void jspReplaceWith(JsVar *dst, JsVar *src) {
     }
     jsvUnLock(parent);
   }
 }
 
+void jspReplaceWithOrAddToRoot(JsVar *dst, JsVar *src) {
+  /* If we're assigning to this and we don't have a parent,
+   * add it to the symbol table root */
+  if (!jsvGetRefs(dst) && jsvIsName(dst)) {
+    if (!jsvIsArrayBufferName(dst) && !jsvIsNewChild(dst))
+      jsvAddName(execInfo.root, dst);
+  }
+  jspReplaceWith(dst, src);
+}
+
 bool jspeiAddScope(JsVar *scope) {
   if (execInfo.scopeCount >= JSPARSE_MAX_SCOPES) {
     jsExceptionHere(JSET_ERROR, "Maximum number of scopes exceeded");
     jspSetError(false);
     return false;
@@ -1969,17 +1979,11 @@ NO_INLINE JsVar *__jspeAssignmentExpression(JsVar *lhs) {
     rhs = jspeAssignmentExpression();
     rhs = jsvSkipNameAndUnLock(rhs); // ensure we get rid of any references on the RHS
 
     if (JSP_SHOULD_EXECUTE && lhs) {
       if (op=='=') {
-        /* If we're assigning to this and we don't have a parent,
-         * add it to the symbol table root */
-        if (!jsvGetRefs(lhs) && jsvIsName(lhs)) {
-          if (!jsvIsArrayBufferName(lhs) && !jsvIsNewChild(lhs))
-            jsvAddName(execInfo.root, lhs);
-        }
-        jspReplaceWith(lhs, rhs);
+        jspReplaceWithOrAddToRoot(lhs, rhs);
       } else {
         if (op==LEX_PLUSEQUAL) op='+';
         else if (op==LEX_MINUSEQUAL) op='-';
         else if (op==LEX_MULEQUAL) op='*';
         else if (op==LEX_DIVEQUAL) op='/';
@@ -2328,16 +2332,10 @@ NO_INLINE JsVar *jspeStatementFor() {
     if (JSP_SHOULD_EXECUTE && !jsvIsName(forStatement)) {
       jsvUnLock(forStatement);
       jsExceptionHere(JSET_ERROR, "FOR a IN b - 'a' must be a variable name, not %t", forStatement);
       return 0;
     }
-    bool addedIteratorToScope = false;
-    if (JSP_SHOULD_EXECUTE && !jsvGetRefs(forStatement)) {
-      // if the variable did not exist, add it to the scope
-      addedIteratorToScope = true;
-      jsvAddName(execInfo.root, forStatement);
-    }
     JSP_MATCH_WITH_CLEANUP_AND_RETURN(LEX_R_IN, jsvUnLock(forStatement), 0);
     JsVar *array = jsvSkipNameAndUnLock(jspeExpression());
     JSP_MATCH_WITH_CLEANUP_AND_RETURN(')', jsvUnLock2(forStatement, array), 0);
     JslCharPos forBodyStart = jslCharPosClone(&lex->tokenStart);
     JSP_SAVE_EXECUTE();
@@ -2369,11 +2367,11 @@ NO_INLINE JsVar *jspeStatementFor() {
             JsVar *indexValue = jsvIsName(loopIndexVar) ?
                 jsvCopyNameOnly(loopIndexVar, false/*no copy children*/, false/*not a name*/) :
                 loopIndexVar;
             if (indexValue) { // could be out of memory
               assert(!jsvIsName(indexValue) && jsvGetRefs(indexValue)==0);
-              jsvSetValueOfName(forStatement, indexValue);
+              jspReplaceWithOrAddToRoot(forStatement, indexValue);
               if (indexValue!=loopIndexVar) jsvUnLock(indexValue);
 
               jsvIteratorNext(&it);
 
               jslSeekToP(&forBodyStart);
@@ -2408,13 +2406,10 @@ NO_INLINE JsVar *jspeStatementFor() {
     }
     jslSeekToP(&forBodyEnd);
     jslCharPosFree(&forBodyStart);
     jslCharPosFree(&forBodyEnd);
 
-    if (addedIteratorToScope) {
-      jsvRemoveChild(execInfo.root, forStatement);
-    }
     jsvUnLock2(forStatement, array);
   } else { // ----------------------------------------------- NORMAL FOR LOOP
 #ifdef JSPARSE_MAX_LOOP_ITERATIONS
     int loopCount = JSPARSE_MAX_LOOP_ITERATIONS;
 #endif

Vulnerable code:         /* If we're assigning to this and we don't have a parent,
         * add it to the symbol table root */
        if (!jsvGetRefs(lhs) && jsvIsName(lhs)) {
          if (!jsvIsArrayBufferName(lhs) && !jsvIsNewChild(lhs))
            jsvAddName(execInfo.root, lhs);
        }
        jspReplaceWith(lhs, rhs);
    bool addedIteratorToScope = false;
    if (JSP_SHOULD_EXECUTE && !jsvGetRefs(forStatement)) {
      // if the variable did not exist, add it to the scope
      addedIteratorToScope = true;
      jsvAddName(execInfo.root, forStatement);
    }
              jsvSetValueOfName(forStatement, indexValue);
    if (addedIteratorToScope) {
      jsvRemoveChild(execInfo.root, forStatement);
    }
Fixed code:             Fix bug if using an undefined member of an object for for..in (fix #1437)
void jspReplaceWithOrAddToRoot(JsVar *dst, JsVar *src) {
  /* If we're assigning to this and we don't have a parent,
   * add it to the symbol table root */
  if (!jsvGetRefs(dst) && jsvIsName(dst)) {
    if (!jsvIsArrayBufferName(dst) && !jsvIsNewChild(dst))
      jsvAddName(execInfo.root, dst);
  }
  jspReplaceWith(dst, src);
}

        jspReplaceWithOrAddToRoot(lhs, rhs);
              jspReplaceWithOrAddToRoot(forStatement, indexValue);