CVE-2018-11595: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: Espruino before 1.99 allows attackers to cause a denial of service (application crash) and a potential Escalation of Privileges with a user crafted input file via a Buffer Overflow during syntax parsing, because strncat is misused.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
CVSS v3 base score: 7.8
Published: 2018-05-31T16:29Z
Repository: https://github.com/espruino/Espruino
Language: C
Files changed: ChangeLog, src/jslex.c, src/jspin.c, src/jsutils.c, src/jsvar.c
Commit date: 2018-05-21 10:35:49 +0100
Commit message: Fix strncat/cpy bounding issues (fix #1425)

Diff: diff --git a/ChangeLog b/ChangeLog
index a857a1621..b09d6f6a0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,9 +1,10 @@
             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)
+            Fix strncat/cpy bounding issues (fix #1425)
 
      1v98 : Allow Crypto SHA1 without SHA256/512 (for ESP8266 where flash is scarce)
             Add better docs for the form of Wifi callback functions
             Modify ESP8266/ESP32 callbacks to match the node.js style used elsewhere
             nRF52: fix pin.toggle() on software-negated pins
diff --git a/src/jslex.c b/src/jslex.c
index b6d02738c..190209c51 100644
--- a/src/jslex.c
+++ b/src/jslex.c
@@ -812,23 +812,18 @@ void jslTokenAsString(int token, char *str, size_t len) {
     strncpy(str, &tokenNames[p], len);
     return;
   }
 
   assert(len>=10);
-  strncpy(str, "?[",len);
-  itostr(token, &str[2], 10);
-  strncat(str, "]",len);
+  espruino_snprintf(str, len, "?[%d]", token);
 }
 
 void jslGetTokenString(char *str, size_t len) {
   if (lex->tk == LEX_ID) {
-    strncpy(str, "ID:", len);
-    strncat(str, jslGetTokenValueAsString(), len);
+    espruino_snprintf(str, len, "ID:%s", jslGetTokenValueAsString());
   } else if (lex->tk == LEX_STR) {
-    strncpy(str, "String:'", len);
-    strncat(str, jslGetTokenValueAsString(), len);
-    strncat(str, "'", len);
+    espruino_snprintf(str, len, "String:'%s'", jslGetTokenValueAsString());
   } else
     jslTokenAsString(lex->tk, str, len);
 }
 
 char *jslGetTokenValueAsString() {
diff --git a/src/jspin.c b/src/jspin.c
index 8fdd42163..f06bd9926 100644
--- a/src/jspin.c
+++ b/src/jspin.c
@@ -177,11 +177,11 @@ void jshGetPinString(char *result, Pin pin) {
       result[0]='V';
       itostr(pin-JSH_PORTV_OFFSET,&result[1],10);
 #endif
 #endif
     } else {
-      strncpy(result, "undefined", 10);
+      strcpy(result, "undefined");
     }
   }
 
 /**
  * Given a var, convert it to a pin ID (or PIN_UNDEFINED if it doesn't exist). safe for undefined!
@@ -377,14 +377,14 @@ void jshPinFunctionToString(JshPinFunction pinFunc, JshPinFunctionToStringFlags
 
   if (!devStr) {
     jsiConsolePrintf("Couldn't convert pin function %d\n", pinFunc);
     return;
   }
-  if (flags & JSPFTS_DEVICE) strncat(buf, devStr, bufSize);
+  if (flags & JSPFTS_DEVICE) strncat(buf, devStr, bufSize-1);
   if (flags & JSPFTS_DEVICE_NUMBER) itostr(devIdx, &buf[strlen(buf)], 10);
-  if (flags & JSPFTS_SPACE) strncat(buf, " ", bufSize);
-  if (infoStr && (flags & JSPFTS_TYPE)) strncat(buf, infoStr, bufSize);
+  if (flags & JSPFTS_SPACE) strncat(buf, " ", bufSize-(strlen(buf)+1));
+  if (infoStr && (flags & JSPFTS_TYPE)) strncat(buf, infoStr, bufSize-(strlen(buf)+1));
 }
 
 /** Prints a list of capable pins, eg:
  jshPrintCapablePins(..., "PWM", JSH_TIMER1, JSH_TIMERMAX, 0,0, false)
  jshPrintCapablePins(..., "SPI", JSH_SPI1, JSH_SPIMAX, JSH_MASK_INFO,JSH_SPI_SCK, false)
diff --git a/src/jsutils.c b/src/jsutils.c
index be879cc19..6384089e2 100644
--- a/src/jsutils.c
+++ b/src/jsutils.c
@@ -602,15 +602,16 @@ void itostr_extra(JsVarInt vals,char *str,bool signedVal, unsigned int base) {
   }
   str[digits] = 0;
 }
 
 void ftoa_bounded_extra(JsVarFloat val,char *str, size_t len, int radix, int fractionalDigits) {
+  assert(len>9); // in case if strcpy
   const JsVarFloat stopAtError = 0.0000001;
-  if (isnan(val)) strncpy(str,"NaN",len);
+  if (isnan(val)) strcpy(str,"NaN");
   else if (!isfinite(val)) {
-    if (val<0) strncpy(str,"-Infinity",len);
-    else strncpy(str,"Infinity",len);
+    if (val<0) strcpy(str,"-Infinity");
+    else strcpy(str,"Infinity");
   } else {
     if (val<0) {
       if (--len <= 0) { *str=0; return; } // bounds check
       *(str++) = '-';
       val = -val;
diff --git a/src/jsvar.c b/src/jsvar.c
index 4f0e050ed..c7564f176 100644
--- a/src/jsvar.c
+++ b/src/jsvar.c
@@ -1184,10 +1184,11 @@ JsVar *jsvGetValueOf(JsVar *v) {
 If the buffer length is exceeded, the returned value will == len */
 size_t jsvGetString(const JsVar *v, char *str, size_t len) {
   const char *s = jsvGetConstString(v);
   if (s) {
     strncpy(str, s, len);
+    str[len-1] = 0;
     return strlen(s);
   } else if (jsvIsInt(v)) {
     itostr(v->varData.integer, str, 10);
     return strlen(str);
   } else if (jsvIsFloat(v)) {
@@ -1216,11 +1217,11 @@ size_t jsvGetString(const JsVar *v, char *str, size_t len) {
     if (stringVar) {
       size_t l = jsvGetString(stringVar, str, len); // call again - but this time with converted var
       jsvUnLock(stringVar);
       return l;
     } else {
-      strncpy(str, "", len);
+      str[0] = 0;
       jsExceptionHere(JSET_INTERNALERROR, "Variable type cannot be converted to string");
       return 0;
     }
   }
 }

Vulnerable code:   strncpy(str, "?[",len);
  itostr(token, &str[2], 10);
  strncat(str, "]",len);
    strncpy(str, "ID:", len);
    strncat(str, jslGetTokenValueAsString(), len);
    strncpy(str, "String:'", len);
    strncat(str, jslGetTokenValueAsString(), len);
    strncat(str, "'", len);
      strncpy(result, "undefined", 10);
  if (flags & JSPFTS_DEVICE) strncat(buf, devStr, bufSize);
  if (flags & JSPFTS_SPACE) strncat(buf, " ", bufSize);
  if (infoStr && (flags & JSPFTS_TYPE)) strncat(buf, infoStr, bufSize);
  if (isnan(val)) strncpy(str,"NaN",len);
    if (val<0) strncpy(str,"-Infinity",len);
    else strncpy(str,"Infinity",len);
      strncpy(str, "", len);
Fixed code:             Fix strncat/cpy bounding issues (fix #1425)
  espruino_snprintf(str, len, "?[%d]", token);
    espruino_snprintf(str, len, "ID:%s", jslGetTokenValueAsString());
    espruino_snprintf(str, len, "String:'%s'", jslGetTokenValueAsString());
      strcpy(result, "undefined");
  if (flags & JSPFTS_DEVICE) strncat(buf, devStr, bufSize-1);
  if (flags & JSPFTS_SPACE) strncat(buf, " ", bufSize-(strlen(buf)+1));
  if (infoStr && (flags & JSPFTS_TYPE)) strncat(buf, infoStr, bufSize-(strlen(buf)+1));
  assert(len>9); // in case if strcpy
  if (isnan(val)) strcpy(str,"NaN");
    if (val<0) strcpy(str,"-Infinity");
    else strcpy(str,"Infinity");
    str[len-1] = 0;
      str[0] = 0;