CVE-2015-4590: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: The extractFrom function in Internals/QuotedString.cpp in Arduino JSON before 4.5 allows remote attackers to cause a denial of service (crash) via a JSON string with a \ (backslash) followed by a terminator, as demonstrated by "\\\0", which triggers a buffer overflow and over-read.
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.
Published: 2015-06-22T18:59Z
Repository: https://github.com/bblanchon/ArduinoJson
Language: C++
Files changed: CHANGELOG.md, src/Internals/QuotedString.cpp, test/QuotedString_ExtractFrom_Tests.cpp
Commit date: 2015-06-10 21:33:20 +0200
Commit message: Fix buffer overflow (pull request #81)

Diff: diff --git a/CHANGELOG.md b/CHANGELOG.md
index 01c7cc9..d43202a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,8 +1,15 @@
 Arduino JSON: change log
 ========================
 
+v4.5
+----
+
+* Fixed buffer overflow when input contains a backslash followed by a terminator (issue #81)
+
+**Upgrading is recommended** since previous versions contain a potential security risk.
+
 v4.4
 ----
 
 * Added `JsonArray::measureLength()` and `JsonObject::measureLength()` (issue #75)
 
diff --git a/src/Internals/QuotedString.cpp b/src/Internals/QuotedString.cpp
index 3a90def..45a6c48 100644
--- a/src/Internals/QuotedString.cpp
+++ b/src/Internals/QuotedString.cpp
@@ -56,48 +56,46 @@ static char unescapeChar(char c) {
 }
 
 static inline bool isQuote(char c) { return c == '\"' || c == '\''; }
 
 char *QuotedString::extractFrom(char *input, char **endPtr) {
-  char firstChar = *input;
-
-  if (!isQuote(firstChar)) {
-    // must start with a quote
-    return NULL;
-  }
-
-  char stopChar = firstChar;  // closing quote is the same as opening quote
-
   char *startPtr = input + 1;  // skip the quote
   char *readPtr = startPtr;
   char *writePtr = startPtr;
   char c;
 
+  char firstChar = *input;
+  char stopChar = firstChar;  // closing quote is the same as opening quote
+
+  if (!isQuote(firstChar)) goto ERROR_OPENING_QUOTE_MISSING;
+
   for (;;) {
     c = *readPtr++;
 
-    if (c == '\0') {
-      // premature ending
-      return NULL;
-    }
+    if (c == '\0') goto ERROR_CLOSING_QUOTE_MISSING;
 
-    if (c == stopChar) {
-      // closing quote
-      break;
-    }
+    if (c == stopChar) goto SUCCESS;
 
     if (c == '\\') {
       // replace char
       c = unescapeChar(*readPtr++);
+      if (c == '\0') goto ERROR_ESCAPE_SEQUENCE_INTERRUPTED;
     }
 
     *writePtr++ = c;
   }
 
+SUCCESS:
   // end the string here
   *writePtr = '\0';
 
   // update end ptr
   *endPtr = readPtr;
 
+  // return pointer to unquoted string
   return startPtr;
+
+ERROR_OPENING_QUOTE_MISSING:
+ERROR_CLOSING_QUOTE_MISSING:
+ERROR_ESCAPE_SEQUENCE_INTERRUPTED:
+  return NULL;
 }
diff --git a/test/QuotedString_ExtractFrom_Tests.cpp b/test/QuotedString_ExtractFrom_Tests.cpp
index 2f6747f..94d0626 100644
--- a/test/QuotedString_ExtractFrom_Tests.cpp
+++ b/test/QuotedString_ExtractFrom_Tests.cpp
@@ -14,10 +14,15 @@ class QuotedString_ExtractFrom_Tests : public testing::Test {
   void whenInputIs(const char *json) {
     strcpy(_jsonString, json);
     _result = QuotedString::extractFrom(_jsonString, &_trailing);
   }
 
+  void whenInputIs(const char *json, size_t len) {
+    memcpy(_jsonString, json, len);
+    _result = QuotedString::extractFrom(_jsonString, &_trailing);
+  }
+
   void resultMustBe(const char *expected) { EXPECT_STREQ(expected, _result); }
 
   void trailingMustBe(const char *expected) {
     EXPECT_STREQ(expected, _trailing);
   }
@@ -132,5 +137,10 @@ TEST_F(QuotedString_ExtractFrom_Tests, EscapedTab) {
 
 TEST_F(QuotedString_ExtractFrom_Tests, AllEscapedCharsTogether) {
   whenInputIs("\"1\\\"2\\\\3\\/4\\b5\\f6\\n7\\r8\\t9\"");
   resultMustBe("1\"2\\3/4\b5\f6\n7\r8\t9");
 }
+
+TEST_F(QuotedString_ExtractFrom_Tests, UnterminatedEscapeSequence) {
+  whenInputIs("\"\\\0\"", 4);
+  resultMustBe(0);
+}

Vulnerable code:   char firstChar = *input;

  if (!isQuote(firstChar)) {
    // must start with a quote
    return NULL;
  }

  char stopChar = firstChar;  // closing quote is the same as opening quote

    if (c == '\0') {
      // premature ending
      return NULL;
    }
    if (c == stopChar) {
      // closing quote
      break;
    }
Fixed code: v4.5
----

* Fixed buffer overflow when input contains a backslash followed by a terminator (issue #81)

**Upgrading is recommended** since previous versions contain a potential security risk.

  char firstChar = *input;
  char stopChar = firstChar;  // closing quote is the same as opening quote

  if (!isQuote(firstChar)) goto ERROR_OPENING_QUOTE_MISSING;

    if (c == '\0') goto ERROR_CLOSING_QUOTE_MISSING;
    if (c == stopChar) goto SUCCESS;
      if (c == '\0') goto ERROR_ESCAPE_SEQUENCE_INTERRUPTED;
SUCCESS:
  // return pointer to unquoted string

ERROR_OPENING_QUOTE_MISSING:
ERROR_CLOSING_QUOTE_MISSING:
ERROR_ESCAPE_SEQUENCE_INTERRUPTED:
  return NULL;
  void whenInputIs(const char *json, size_t len) {
    memcpy(_jsonString, json, len);
    _result = QuotedString::extractFrom(_jsonString, &_trailing);
  }


TEST_F(QuotedString_ExtractFrom_Tests, UnterminatedEscapeSequence) {
  whenInputIs("\"\\\0\"", 4);
  resultMustBe(0);
}