CVE-2020-7660: Deserialization of Untrusted Data (CWE-502)
value: serialize-javascript prior to 3.1.0 allows remote attackers to inject arbitrary code via the function "deleteFunctions" within "index.js".
CWE description: The product deserializes untrusted data without sufficiently verifying that the resulting data will be valid.
CVSS v3 base score: 8.1
Published: 2020-06-01T15:15Z
Repository: https://github.com/yahoo/serialize-javascript
Language: JSON
Files changed: index.js, package-lock.json, package.json, test/unit/serialize.js
Commit date: 2020-05-20 22:52:22 +0900
Commit message: Don't replace regex / function placeholders within string literals (#79)

Previously we weren't checking if the quote that started the placeholder
was escaped or not, meaning an object like

    {"foo": /1"/, "bar": "a\"@__R-<UID>-0__@"}

Would be serialized as

    {"foo": /1"/, "bar": "a\/1"/}

meaning an attacker could escape out of `bar` if they controlled both
`foo` and `bar` and were able to guess the value of `<UID>`.

UID was generated once on startup, was chosen using `Math.random()` and
had a keyspace of roughly 4 billion, so within the realm of an online
attack.

Here's a simple example that will cause `console.log()` to be called when
the `serialize()`d version is `eval()`d

    eval('('+ serialize({"foo": /1" + console.log(1)/i, "bar": '"@__R-<UID>-0__@'}) + ')');

Where `<UID>` is the guessed `UID`.

This fixes the issue by ensuring that placeholders are not preceded by
a backslash.

We also switch to a higher entropy `UID` to prevent people from guessing it.

Co-authored-by: Jordan Milne <jmilne@uber.com>
Co-authored-by: Ryan Siebert <ryan.siebert@energyaustralia.com.au>

Diff: diff --git a/index.js b/index.js
index 841bb7e..26163d8 100644
--- a/index.js
+++ b/index.js
@@ -4,13 +4,16 @@ Copyrights licensed under the New BSD License.
 See the accompanying LICENSE file for terms.
 */
 
 'use strict';
 
+var randomBytes = require('randombytes');
+
 // Generate an internal UID to make the regexp pattern harder to guess.
-var UID                 = Math.floor(Math.random() * 0x10000000000).toString(16);
-var PLACE_HOLDER_REGEXP = new RegExp('"@__(F|R|D|M|S|U|I)-' + UID + '-(\\d+)__@"', 'g');
+var UID_LENGTH          = 16;
+var UID                 = generateUID();
+var PLACE_HOLDER_REGEXP = new RegExp('(\\\\)?"@__(F|R|D|M|S|U|I)-' + UID + '-(\\d+)__@"', 'g');
 
 var IS_NATIVE_CODE_REGEXP = /\{\s*\[native code\]\s*\}/g;
 var IS_PURE_FUNCTION = /function.*?\(/;
 var IS_ARROW_FUNCTION = /.*?=>.*?/;
 var UNSAFE_CHARS_REGEXP   = /[<>\/\u2028\u2029]/g;
@@ -29,10 +32,19 @@ var ESCAPED_CHARS = {
 
 function escapeUnsafeChars(unsafeChar) {
     return ESCAPED_CHARS[unsafeChar];
 }
 
+function generateUID() {
+    var bytes = randomBytes(UID_LENGTH);
+    var result = '';
+    for(var i=0; i<UID_LENGTH; ++i) {
+        result += bytes[i].toString(16);
+    }
+    return result;
+}
+
 function deleteFunctions(obj){
     var functionKeys = [];
     for (var key in obj) {
         if (typeof obj[key] === "function") {
             functionKeys.push(key);
@@ -185,11 +197,18 @@ module.exports = function serialize(obj, options) {
     }
 
     // Replaces all occurrences of function, regexp, date, map and set placeholders in the
     // JSON string with their string representations. If the original value can
     // not be found, then `undefined` is used.
-    return str.replace(PLACE_HOLDER_REGEXP, function (match, type, valueIndex) {
+    return str.replace(PLACE_HOLDER_REGEXP, function (match, backSlash, type, valueIndex) {
+        // The placeholder may not be preceded by a backslash. This is to prevent
+        // replacing things like `"a\"@__R-<UID>-0__@"` and thus outputting
+        // invalid JS.
+        if (backSlash) {
+            return match;
+        }
+
         if (type === 'D') {
             return "new Date(\"" + dates[valueIndex].toISOString() + "\")";
         }
 
         if (type === 'R') {
diff --git a/package-lock.json b/package-lock.json
index fbf7630..f101e96 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1590,10 +1590,18 @@
       "dev": true,
       "requires": {
         "fromentries": "^1.2.0"
       }
     },
+    "randombytes": {
+      "version": "2.1.0",
+      "resolved": "https://registry.npmjs.org/randombytes/-/randombytes-2.1.0.tgz",
+      "integrity": "sha512-vYl3iOX+4CKUWuxGi9Ukhie6fsqXqS9FE2Zaic4tNFD2N2QQaXOMFbuKK4QmDHC0JO6B1Zp41J0LpT0oR68amQ==",
+      "requires": {
+        "safe-buffer": "^5.1.0"
+      }
+    },
     "readdirp": {
       "version": "3.2.0",
       "resolved": "https://registry.npmjs.org/readdirp/-/readdirp-3.2.0.tgz",
       "integrity": "sha512-crk4Qu3pmXwgxdSgGhgA/eXiJAPQiX4GMOZZMXnqKxHX7TaoL+3gQVo/WeuAiogr07DpnfjIMpXXa+PAIvwPGQ==",
       "dev": true,
@@ -1647,12 +1655,11 @@
       }
     },
     "safe-buffer": {
       "version": "5.1.2",
       "resolved": "https://registry.npmjs.org/safe-buffer/-/safe-buffer-5.1.2.tgz",
-      "integrity": "sha512-Gd2UZBJDkXlY7GbJxfsE8/nvKkUEU1G38c1siN6QP6a9PT9MmHB8GnpscSmMJSoF8LOIrt8ud/wPtojys4G6+g==",
-      "dev": true
+      "integrity": "sha512-Gd2UZBJDkXlY7GbJxfsE8/nvKkUEU1G38c1siN6QP6a9PT9MmHB8GnpscSmMJSoF8LOIrt8ud/wPtojys4G6+g=="
     },
     "semver": {
       "version": "5.7.1",
       "resolved": "https://registry.npmjs.org/semver/-/semver-5.7.1.tgz",
       "integrity": "sha512-sauaDf/PZdVgrLTNYHRtpXa1iRiKcaebiKQ1BJdpQlWH2lCvexQdX55snPFyK7QzpudqbCI0qXFfOasHdyNDGQ==",
diff --git a/package.json b/package.json
index 36073d1..12b3954 100644
--- a/package.json
+++ b/package.json
@@ -27,7 +27,10 @@
   "devDependencies": {
     "benchmark": "^2.1.4",
     "chai": "^4.1.0",
     "mocha": "^7.0.0",
     "nyc": "^15.0.0"
+  },
+  "dependencies": {
+    "randombytes": "^2.1.0"
   }
 }
diff --git a/test/unit/serialize.js b/test/unit/serialize.js
index 3944344..88b3603 100644
--- a/test/unit/serialize.js
+++ b/test/unit/serialize.js
@@ -1,11 +1,25 @@
 /* global describe, it, beforeEach */
 'use strict';
 
+// temporarily monkeypatch `crypto.randomBytes` so we'll have a
+// predictable UID for our tests
+var crypto = require('crypto');
+var oldRandom = crypto.randomBytes;
+crypto.randomBytes = function(len, cb) {
+    var buf = Buffer.alloc(len);
+    buf.fill(0x00);
+    if (cb)
+        cb(null, buf);
+    return buf;
+};
+
 var serialize = require('../../'),
     expect    = require('chai').expect;
 
+crypto.randomBytes = oldRandom;
+
 describe('serialize( obj )', function () {
     it('should be a function', function () {
         expect(serialize).to.be.a('function');
     });
 
@@ -491,6 +505,19 @@ describe('serialize( obj )', function () {
             expect(serialize([1], 1)).to.equal('[\n 1\n]');
             expect(serialize([1], ' ')).to.equal('[\n 1\n]');
             expect(serialize([1], 2)).to.equal('[\n  1\n]');
         });
     });
+
+    describe('placeholders', function() {
+        it('should not be replaced within string literals', function () {
+            // Since we made the UID deterministic this should always be the placeholder
+            var fakePlaceholder = '"@__R-0000000000000000-0__@';
+            var serialized = serialize({bar: /1/i, foo: fakePlaceholder}, {uid: 'foo'});
+            var obj = eval('(' + serialized + ')');
+            expect(obj).to.be.a('Object');
+            expect(obj.foo).to.be.a('String');
+            expect(obj.foo).to.equal(fakePlaceholder);
+        });
+    });
+
 });

Vulnerable code: var UID                 = Math.floor(Math.random() * 0x10000000000).toString(16);
var PLACE_HOLDER_REGEXP = new RegExp('"@__(F|R|D|M|S|U|I)-' + UID + '-(\\d+)__@"', 'g');
    return str.replace(PLACE_HOLDER_REGEXP, function (match, type, valueIndex) {
      "integrity": "sha512-Gd2UZBJDkXlY7GbJxfsE8/nvKkUEU1G38c1siN6QP6a9PT9MmHB8GnpscSmMJSoF8LOIrt8ud/wPtojys4G6+g==",
      "dev": true
Fixed code: var randomBytes = require('randombytes');

var UID_LENGTH          = 16;
var UID                 = generateUID();
var PLACE_HOLDER_REGEXP = new RegExp('(\\\\)?"@__(F|R|D|M|S|U|I)-' + UID + '-(\\d+)__@"', 'g');
function generateUID() {
    var bytes = randomBytes(UID_LENGTH);
    var result = '';
    for(var i=0; i<UID_LENGTH; ++i) {
        result += bytes[i].toString(16);
    }
    return result;
}

    return str.replace(PLACE_HOLDER_REGEXP, function (match, backSlash, type, valueIndex) {
        // The placeholder may not be preceded by a backslash. This is to prevent
        // replacing things like `"a\"@__R-<UID>-0__@"` and thus outputting
        // invalid JS.
        if (backSlash) {
            return match;
        }

    "randombytes": {
      "version": "2.1.0",
      "resolved": "https://registry.npmjs.org/randombytes/-/randombytes-2.1.0.tgz",
      "integrity": "sha512-vYl3iOX+4CKUWuxGi9Ukhie6fsqXqS9FE2Zaic4tNFD2N2QQaXOMFbuKK4QmDHC0JO6B1Zp41J0LpT0oR68amQ==",
      "requires": {
        "safe-buffer": "^5.1.0"
      }
    },
      "integrity": "sha512-Gd2UZBJDkXlY7GbJxfsE8/nvKkUEU1G38c1siN6QP6a9PT9MmHB8GnpscSmMJSoF8LOIrt8ud/wPtojys4G6+g=="
  },
  "dependencies": {
    "randombytes": "^2.1.0"
// temporarily monkeypatch `crypto.randomBytes` so we'll have a
// predictable UID for our tests
var crypto = require('crypto');
var oldRandom = crypto.randomBytes;
crypto.randomBytes = function(len, cb) {
    var buf = Buffer.alloc(len);
    buf.fill(0x00);
    if (cb)
        cb(null, buf);
    return buf;
};

crypto.randomBytes = oldRandom;


    describe('placeholders', function() {
        it('should not be replaced within string literals', function () {
            // Since we made the UID deterministic this should always be the placeholder
            var fakePlaceholder = '"@__R-0000000000000000-0__@';
            var serialized = serialize({bar: /1/i, foo: fakePlaceholder}, {uid: 'foo'});
            var obj = eval('(' + serialized + ')');
            expect(obj).to.be.a('Object');
            expect(obj.foo).to.be.a('String');
            expect(obj.foo).to.equal(fakePlaceholder);
        });
    });