-
Notifications
You must be signed in to change notification settings - Fork 181
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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 <[email protected]> Co-authored-by: Ryan Siebert <[email protected]>
- Loading branch information
1 parent
1ac487e
commit f21a6fb
Showing
4 changed files
with
61 additions
and
5 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,9 +6,12 @@ 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.*?\(/; | ||
|
@@ -31,6 +34,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); | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
kimhanse
|
||
} | ||
return result; | ||
} | ||
|
||
function deleteFunctions(obj){ | ||
var functionKeys = []; | ||
for (var key in obj) { | ||
|
@@ -187,7 +199,14 @@ 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() + "\")"; | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,5 +29,8 @@ | |
"chai": "^4.1.0", | ||
"mocha": "^7.0.0", | ||
"nyc": "^15.0.0" | ||
}, | ||
"dependencies": { | ||
"randombytes": "^2.1.0" | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Can this not also use
UID_LENGTH
?