Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't replace escaped regex / function placeholders in strings #22

Conversation

JordanMilne
Copy link

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.

@yahoocla
Copy link

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@JordanMilne
Copy link
Author

CLA signed

@ericf
Copy link
Collaborator

ericf commented Oct 14, 2016

@JordanMilne thanks for digging into this! Do you think it also makes sense to generate a UID with a larger set of possible values?

@JordanMilne
Copy link
Author

@ericf Yeah, I think that would be sensible. As long as UID is still guessable you could still do things like have

{"foo": /1/, "bar":"@__R-<UID>-0__@"}

serialize as

{"foo": /1/, "bar": /1/}

Far less useful in an attack IMO but might still be a problem.

Math.random() isn't a great source of entropy if you want to prevent attacks like that since it's generally not backed by a CSPRNG, though. I'll add a commit to use a CSPRNG where available and add more entropy to UID.

@yahoocla
Copy link

CLA is valid!

Copy link
Collaborator

@ericf ericf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JordanMilne thanks for updating this to add a better source of entropy! I added a few comments…

@@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why you chose this package over the built-in crypto.randomBytes()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used that so as not to break the serializer when run in a browser. If the serializer intended to be node-only I'll just use crypto.randomBytes()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention is node-only. But it's true that I don't know if people are bundling it for the browser. So let's stick with what you have using randombytes.

@@ -37,6 +52,7 @@ module.exports = function serialize(obj, options) {
options = {space: options};
}

var uid = options.uid || UID;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea to allow options.uid. But curious why you added it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need a deterministic value for UID for the regression test I added, the other option is to monkeypatch crypto.randomBytes() in the tests like I was doing with Math.random(), but that felt a little brittle. I'm fine with whichever you prefer

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the monkey patch for the tests because I don't want people setting options.uid and getting pwned.

// 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now with a better source of entropy, is the backslash escape still required, or did you want to leave it for extra protection?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any harm in having it. I'm more confident that this will correctly prevent user input from escaping strings than I am in my knowledge of CSPRNGs :)

@ericf
Copy link
Collaborator

ericf commented Oct 21, 2016

@JordanMilne if you don't mind replacing options.uid which monkey patching randombytes in the test, then I'll merge this and get a release out. Thanks!

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.
@JordanMilne JordanMilne force-pushed the dont_replace_escaped_placeholders branch from 58bd2b6 to 31ad2d3 Compare October 21, 2016 22:40
@JordanMilne
Copy link
Author

switched to monkeypatching crypto.randomBytes in the test and squashed

var crypto = require('crypto');
var oldRandom = crypto.randomBytes;
crypto.randomBytes = function(len, cb) {
var buf = new Buffer(len);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new Buffer() is deprecated. I think we should use Buffer.from() instead.

Copy link
Collaborator

@okuryu okuryu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please resolve the conflicts.

@eugef
Copy link

eugef commented Feb 28, 2020

@JordanMilne @okuryu this issue is still reported as a security vulnerability of the serialize-javascript package.

Are there any plans to merge the fix?

@JordanMilne
Copy link
Author

I'm not sure if a fix was merged, you should be able to check by seeing if the testcase in the PR repros.

I'm not likely to update this to work with the new serialization code if it turns out this does still repro, as I no longer work on any code that has serialize-javascript as a dependency. Feel free to use this code as the basis for another PR if you like though, I've already signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants