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

@JordanMilne JordanMilne commented Oct 13, 2016

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

@yahoocla yahoocla commented Oct 13, 2016

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

@JordanMilne JordanMilne commented Oct 13, 2016

CLA signed

@ericf
Copy link
Collaborator

@ericf 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

@JordanMilne JordanMilne commented Oct 14, 2016

@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

@yahoocla yahoocla commented Oct 15, 2016

CLA is valid!

Copy link
Collaborator

@ericf ericf left a comment

@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

@ericf ericf Oct 17, 2016

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

Copy link
Author

@JordanMilne JordanMilne Oct 21, 2016

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

@ericf ericf Oct 21, 2016

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

@ericf ericf Oct 17, 2016

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

Copy link
Author

@JordanMilne JordanMilne Oct 21, 2016

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

@ericf ericf Oct 21, 2016

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

@ericf ericf Oct 17, 2016

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

@JordanMilne JordanMilne Oct 21, 2016

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 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 Oct 21, 2016
@JordanMilne
Copy link
Author

@JordanMilne JordanMilne commented Oct 21, 2016

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

@okuryu okuryu Jul 17, 2017

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

Copy link
Collaborator

@okuryu okuryu left a comment

please resolve the conflicts.

@eugef
Copy link

@eugef 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

@JordanMilne JordanMilne commented Feb 28, 2020

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
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants