Skip to content

Commit

Permalink
Don't replace escaped regex / function placeholders in strings
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 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.
  • Loading branch information
Jordan Milne committed Oct 13, 2016
1 parent adfee60 commit 0dd61b8
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 2 deletions.
7 changes: 5 additions & 2 deletions index.js
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
Expand Down
18 changes: 18 additions & 0 deletions 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');
Expand Down Expand Up @@ -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 () {
Expand Down

0 comments on commit 0dd61b8

Please sign in to comment.