Skip to content

Commit

Permalink
Don't replace regex / function placeholders within string literals
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Jordan Milne authored and JordanMilne committed Oct 21, 2016
1 parent adfee60 commit 31ad2d3
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 4 deletions.
25 changes: 21 additions & 4 deletions index.js
Expand Up @@ -6,11 +6,13 @@ See the accompanying LICENSE file for terms.

'use strict';

var isRegExp = require('util').isRegExp;
var isRegExp = require('util').isRegExp;
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)-' + UID + '-(\\d+)__@"', 'g');
var UID_LENGTH = 16;
var UID = generateUID();
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;
Expand All @@ -29,6 +31,15 @@ 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;
}

module.exports = function serialize(obj, options) {
options || (options = {});

Expand Down Expand Up @@ -92,7 +103,13 @@ 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) {
// 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 === 'R') {
return regexps[valueIndex].toString();
}
Expand Down
3 changes: 3 additions & 0 deletions package.json
Expand Up @@ -30,5 +30,8 @@
"istanbul": "^0.3.2",
"mocha": "^1.21.4",
"xunit-file": "0.0.5"
},
"dependencies": {
"randombytes": "^2.0.3"
}
}
26 changes: 26 additions & 0 deletions test/unit/serialize.js
@@ -1,9 +1,23 @@
/* 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 = new Buffer(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');
Expand Down Expand Up @@ -111,6 +125,18 @@ describe('serialize( obj )', function () {
});
});

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);
});
});

describe('regexps', function () {
it('should serialize constructed regexps', function () {
var re = new RegExp('asdf');
Expand Down

0 comments on commit 31ad2d3

Please sign in to comment.