From 612aa103e22f53abd74465f746e6e790e9226314 Mon Sep 17 00:00:00 2001
From: Coleman Watts <coleman@civicrm.org>
Date: Thu, 28 May 2020 15:08:43 -0400
Subject: [PATCH] CRM_Utils_JS - Improve validation of strings

Runs strings through json_decode to ensure they are valid.
Optionally throws an exception on error.
---
 CRM/Utils/JS.php                   | 43 +++++++++++++++++++++++-------
 tests/phpunit/CRM/Utils/JSTest.php |  9 ++++---
 2 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/CRM/Utils/JS.php b/CRM/Utils/JS.php
index 27e52d0d1f..da228d2413 100644
--- a/CRM/Utils/JS.php
+++ b/CRM/Utils/JS.php
@@ -125,26 +125,51 @@ class CRM_Utils_JS {
    * ]
    *
    * @param string $js
+   * @param bool $throwException
    * @return mixed
-   * @throws Exception
+   * @throws CRM_Core_Exception
    */
-  public static function decode($js) {
+  public static function decode($js, $throwException = FALSE) {
     $js = trim($js);
     $first = substr($js, 0, 1);
     $last = substr($js, -1);
-    if ($last === $first && ($first === "'" || $first === '"')) {
-      // Use a temp placeholder for escaped backslashes
-      $backslash = chr(0) . 'backslash' . chr(0);
-      return str_replace(['\\\\', "\\'", '\\"', '\\&', '\\/', $backslash], [$backslash, "'", '"', '&', '/', '\\'], substr($js, 1, -1));
+    if ($first === "'" && $last === "'") {
+      $js = self::convertSingleQuoteString($js, $throwException);
     }
-    if (($first === '{' && $last === '}') || ($first === '[' && $last === ']')) {
+    elseif (($first === '{' && $last === '}') || ($first === '[' && $last === ']')) {
       $obj = self::getRawProps($js);
       foreach ($obj as $idx => $item) {
-        $obj[$idx] = self::decode($item);
+        $obj[$idx] = self::decode($item, $throwException);
       }
       return $obj;
     }
-    return json_decode($js);
+    $result = json_decode($js);
+    if ($throwException && $result === NULL && $js !== 'null') {
+      throw new CRM_Core_Exception(json_last_error_msg());
+    }
+    return $result;
+  }
+
+  /**
+   * @param string $str
+   * @return string|null
+   * @throws CRM_Core_Exception
+   */
+  public static function convertSingleQuoteString(string $str, $throwException) {
+    // json_decode can only handle double quotes around strings, so convert single-quoted strings
+    $backslash = chr(0) . 'backslash' . chr(0);
+    $str = str_replace(['\\\\', '\\"', '"', '\\&', '\\/', $backslash], [$backslash, '"', '\\"', '&', '/', '\\'], substr($str, 1, -1));
+    // Ensure the string doesn't terminate early by checking that all single quotes are escaped
+    $pos = -1;
+    while (($pos = strpos($str, "'", $pos + 1)) !== FALSE) {
+      if (($pos - strlen(rtrim(substr($str, 0, $pos)))) % 2) {
+        if ($throwException) {
+          throw new CRM_Core_Exception('Invalid string passed to CRM_Utils_JS::decode');
+        }
+        return NULL;
+      }
+    }
+    return '"' . $str . '"';
   }
 
   /**
diff --git a/tests/phpunit/CRM/Utils/JSTest.php b/tests/phpunit/CRM/Utils/JSTest.php
index 36b6fef533..56b758bc81 100644
--- a/tests/phpunit/CRM/Utils/JSTest.php
+++ b/tests/phpunit/CRM/Utils/JSTest.php
@@ -215,6 +215,9 @@ class CRM_Utils_JSTest extends CiviUnitTestCase {
         ' [{a: {aa: true}, b: [false, null, {x: 1, y: 2, z: 3}] , "c": -1}, ["fee", "fie", \'foe\']]',
         [['a' => ['aa' => TRUE], 'b' => [FALSE, NULL, ['x' => 1, 'y' => 2, 'z' => 3]], "c" => -1], ["fee", "fie", "foe"]],
       ],
+      ["'Hi\&'", 'Hi&'],
+      ['"Hello " + alert("XSS") + " sucker"', NULL],
+      ["'Hello ' + alert('XSS') + ' sucker'", NULL],
     ];
   }
 
@@ -234,8 +237,8 @@ class CRM_Utils_JSTest extends CiviUnitTestCase {
         "{a: 'Apple', b: 'Banana', c: [0, -2, 3.15]}",
       ],
       [
-        ['a' => ['foo', 'bar'], 'b' => ["'a'" => ['foo/bar&', 'bar(foo)'], 'b' => ['a' => ["fo\\\\'oo", '"bar"'], 'b' => []]]],
-        "{a: ['foo', 'bar'], b: {\"'a'\": ['foo/bar&', 'bar(foo)'], b: {a: ['fo\\\\\\\\\\'oo', '\"bar\"'], b: {}}}}",
+        ['a' => ['foo', 'bar'], 'b' => ["'a'" => ['foo/bar&', 'bar(foo)'], 'b' => ['a' => ["fo\\'oo", '"bar"'], 'b' => []]]],
+        "{a: ['foo', 'bar'], b: {\"'a'\": ['foo/bar&', 'bar(foo)'], b: {a: ['fo\\\\\\'oo', '\"bar\"'], b: {}}}}",
       ],
       [TRUE, 'true'],
       [' ', "' '"],
@@ -299,7 +302,7 @@ class CRM_Utils_JSTest extends CiviUnitTestCase {
         '{status: /^http:\/\/civicrm\.com/.test(url) ? \'good\' : \'bad\', "foo&": getFoo("Some \"quoted\" thing"), "ba\'[(r": function() {return "bar"}}',
       ],
       [
-        '{"some\"key": typeof foo === \'number\' ? true : false , "O\'Really?": ",((,", \'A"quote"\': 1 + 1 , "\\\\\\&\\/" : 0}',
+        '{"some\"key": typeof foo === \'number\' ? true : false , "O\'Really?": ",((,", \'A"quote"\': 1 + 1 , "\\\\&\\/" : 0}',
         ['some"key' => 'typeof foo === \'number\' ? true : false', "O'Really?" => '",((,"', 'A"quote"' => '1 + 1', '\\&/' => '0'],
         '{\'some"key\': typeof foo === \'number\' ? true : false, "O\'Really?": ",((,", \'A"quote"\': 1 + 1, "\\\\&/": 0}',
       ],
-- 
2.25.1