From 0dd61b8a9af3fb41c0ca24a3638d07cca0a62b0a Mon Sep 17 00:00:00 2001
From: Jordan Milne <jmilne@uber.com>
Date: Thu, 13 Oct 2016 15:30:22 -0700
Subject: [PATCH] Don't replace escaped regex / function placeholders in
 strings

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 is generated once on startup, is chosen using `Math.random()` and
has 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.
---
 index.js               |  7 +++++--
 test/unit/serialize.js | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/index.js b/index.js
index 0f4c3d3..b1484d8 100644
--- a/index.js
+++ b/index.js
@@ -10,7 +10,7 @@ var isRegExp = require('util').isRegExp;
 
 // 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)-' + UID + '-(\\d+)__@"', 'g');
+var PLACE_HOLDER_REGEXP = new RegExp('(\\\\)?"@__(F|R)-' + UID + '-(\\d+)__@"', 'g');
 
 var IS_NATIVE_CODE_REGEXP = /\{\s*\[native code\]\s*\}/g;
 var UNSAFE_CHARS_REGEXP   = /[<>\/\u2028\u2029]/g;
@@ -92,7 +92,10 @@ module.exports = function serialize(obj, options) {
     // Replaces all occurrences of function and regexp 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) {
+        if (backSlash) {
+            return match;
+        }
         if (type === 'R') {
             return regexps[valueIndex].toString();
         }
diff --git a/test/unit/serialize.js b/test/unit/serialize.js
index 882351e..89b76d3 100644
--- a/test/unit/serialize.js
+++ b/test/unit/serialize.js
@@ -1,9 +1,15 @@
 /* global describe, it, beforeEach */
 'use strict';
 
+// Temporarily replace `Math.random` so `UID` will be deterministic
+var oldRandom = Math.random;
+Math.random = function(){return 0.5};
+
 var serialize = require('../../'),
     expect    = require('chai').expect;
 
+Math.random = oldRandom;
+
 describe('serialize( obj )', function () {
     it('should be a function', function () {
         expect(serialize).to.be.a('function');
@@ -160,6 +166,18 @@ describe('serialize( obj )', function () {
             expect(re).to.be.a('RegExp');
             expect(re.source).to.equal('\\..*');
         });
+
+        it('should ignore placeholders with leading backslashes', function(){
+            // Since we made the UID deterministic this should always be the placeholder
+            var placeholder = '@__R-8000000000-0__@';
+            var obj = eval('(' + serialize({
+                "bar": /1/i,
+                "foo": '"' + placeholder
+            }) + ')');
+            expect(obj).to.be.a('Object');
+            expect(obj.foo).to.be.a('String');
+            expect(obj.foo).to.equal('"' + placeholder);
+        });
     });
 
     describe('XSS', function () {