Update QuotedString.cpp

[ghost]

After seeing how useful this library has proven to be to within the Arduino community, I decided to fuzz the parser.

All the fuzzing and testing was done in a Linux x64 environment, using AFL and LLVM's AddressSanitizer in order to hunt for weird behavior.

Shortly after beginning to fuzz the parser, I encountered a crash with the following sequence:

00000000  7b 22 20 20 20 20 20 20  20 20 20 20 20 20 20 20  |{"              |
00000010  20 20 20 20 20 20 20 20  20 20 20 20 20 20 20 20  |                |
*
00000030  33 20 20 20 20 20 20 20  20 20 20 20 20 20 20 20  |3               |
00000040  20 20 20 20 20 20 20 20  20 20 20 20 20 20 20 20  |                |
00000050  20 20 20 20 20 20 20 20  20 76 20 20 20 20 20 20  |         v      |
00000060  20 20 20 20 20 20 20 20  20 20 20 20 20 20 20 20  |                |
*
00000080  20 33 20 20 20 20 20 20  20 20 20 20 20 20 20 20  | 3              |
00000090  20 20 20 20 20 20 20 20  20 20 20 20 20 20 20 20  |                |
*
000000e0  20 20 20 4b 5c 5c 5c 00                           |   K\\\.|
000000e7

This interesting sequence produces a buffer over-read and overflow in the stack or heap, depending on where the JSON string is located, and can cause memory corruptions that crash the program.

The culprit is the escaped characters at the end of the quoted string, they cause the extractFrom function to miss the NULL terminator on the next cycle.

The original extractFrom code is as follows:

char *QuotedString::extractFrom(char *input, char **endPtr) {
  ...
  for (;;) {
    c = *readPtr++;

    if (c == '\0') {
      // premature ending
      return NULL;
    }

    if (c == stopChar) {
      // closing quote
      break;
    }

>>>
    if (c == '\\') {
      // replace char
      c = unescapeChar(*readPtr++);
    }
>>>

    *writePtr++ = c;
  }
  ...
}

As you can see, this operation will eventually lead the NULL check to miss the string's null terminator, effectively causing a buffer over-read and overflow which can lead to security issues.

In order to illustrate the issue, let [] denote the pointer position, let ! denote NULL, and let this be the pointer state:

"[\]\\!"

The unescapeChar function will move the pointer to:

"\[\]\!"

At the beginning of the next cycle, the pointer will move one more step to:

"\\[\]!"

And the unescapeChar function will once again move the pointer to:

"\\\[!]"

And, finally, at the beginning of the next cycle, the pointer will move and skip the null terminator:

"\\\![]"

The fix I propose simply checks to see if we've reached a premature ending after unescaping the character:

--- a/src/Internals/QuotedString.cpp
+++ b/src/Internals/QuotedString.cpp
@@ -73,6 +73,7 @@ char *QuotedString::extractFrom(char *input, char **endPtr) {
   char c;

   for (;;) {
     c = *readPtr++;

     if (c == '\0') {
@@ -90,6 +91,11 @@ char *QuotedString::extractFrom(char *input, char **endPtr) {
       c = unescapeChar(*readPtr++);
     }

+    if (c == '\0') {
+      // premature ending
+      return NULL;
+    }
+
     *writePtr++ = c;
   }

The ArduinoJson library passes all built-in tests with this fix.

[bblanchon]

Good catch 👍
I manually merged your PR in 5e7b9ec and released v4.5.
Thank you so much!

[ghost]

Thanks! You're very welcome.

[ghost]

Benoît, could you also include something like "Special thanks to Giancarlo Canales Barreto" in the change log entry? It's a customary thing in the OSS community, but I forgot to ask earlier.

Regards.

[bblanchon]

Done. Thanks again 😉

[ghost]

Likewise!

[CDSRV]

Does this close CVE-2015-4590 ? https://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2015-4590

[bblanchon]

Yes.