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

Vulnerability disucssion #1408

Open
brianc opened this Issue Aug 13, 2017 · 4 comments

Comments

4 participants
@brianc
Owner

brianc commented Aug 13, 2017

announcement

The issue was caused by a performance optimization in result parsing. Result parsing uses a form of eval to generate a constructor for result rows to utilize v8's hidden classes. The vulnerability can be seen whenever a result column has a name or alias which is executable JavaScript code.

Please let me know if you have any questions or concerns.

@sehrope

This comment has been minimized.

Contributor

sehrope commented Aug 13, 2017

@benjie

This comment has been minimized.

benjie commented Aug 15, 2017

Would it be safer to use a core language feature instead of js-string-escape by switching to JSON.stringify()? I would also expect this to be more performant? i.e.

 return "\nthis[" +
    // Fields are escaped to prevent code execution vulnerabilities
    // See https://github.com/brianc/node-postgres/issues/1408
    JSON.stringify(String(fieldName)) +
    "] = " +

Disclaimer: I'm not absolutely certain that JSON.stringify() is secure enough for this... But I'd be extremely (!!) surprised if it wasn't since eval('(' + jsonString + ')') can be used to interpret JSON!

@sehrope

This comment has been minimized.

Contributor

sehrope commented Aug 15, 2017

@benjie No that wouldn't work as there's a couple oddball edge cases involving Unicode line separators separate from \n and \r. They're valid as individual chars in JSON but would need to be escaped in JS: https://stackoverflow.com/questions/23752156/are-all-json-objects-also-valid-javascript-objects

Using the JSON.stringify(...) approach would break if one them appeared within a column name. Here's an example of this in practice:

const assert = require('assert');

const columnNames = [
    `foo`,
    `foo bar`,
    `foo'bar`,
    `foo"bar`,
    `foo\\bar`,
    `foo\nbar`,
    `foo\u2028bar`,
    `foo\u2029bar`,
];

for (const columnName of columnNames) {
    try {
        console.log('columnName: =>%s<=', columnName);
        console.log('columnName as hex: %s', new Buffer(columnName).toString('hex'));

        const quotedColumnName = JSON.stringify(columnName);
        const code = 'row[' + quotedColumnName + '] = 1;';
        console.log('code: %s', code);
        console.log('code as hex: %s', new Buffer(code).toString('hex'));

        const row = {};
        eval(code);

        const value = row[columnName];
        assert(1 === value, 'Value is supposed to be 1');
    } catch (e) {
        console.error('Error running eval: %s', e.stack);
    }

    console.log();
}

And here's the output of running it on Node.js v8.3.0. Older versions should be the same as the string handling has been part of JS since the beginning. Check out the output for the last two examples to see the break.

columnName: =>foo<=
columnName as hex: 666f6f
code: row["foo"] = 1;
code as hex: 726f775b22666f6f225d203d20313b

columnName: =>foo bar<=
columnName as hex: 666f6f20626172
code: row["foo bar"] = 1;
code as hex: 726f775b22666f6f20626172225d203d20313b

columnName: =>foo'bar<=
columnName as hex: 666f6f27626172
code: row["foo'bar"] = 1;
code as hex: 726f775b22666f6f27626172225d203d20313b

columnName: =>foo"bar<=
columnName as hex: 666f6f22626172
code: row["foo\"bar"] = 1;
code as hex: 726f775b22666f6f5c22626172225d203d20313b

columnName: =>foo\bar<=
columnName as hex: 666f6f5c626172
code: row["foo\\bar"] = 1;
code as hex: 726f775b22666f6f5c5c626172225d203d20313b

columnName: =>foo
bar<=
columnName as hex: 666f6f0a626172
code: row["foo\nbar"] = 1;
code as hex: 726f775b22666f6f5c6e626172225d203d20313b

columnName: =>foo
bar<=
columnName as hex: 666f6fe280a8626172
code: row["foo
bar"] = 1;
code as hex: 726f775b22666f6fe280a8626172225d203d20313b
Error running eval: SyntaxError: Invalid or unexpected token
    at Object.<anonymous> (/tmp/20171015-1113124/fun-with-escapes.js:25:14)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
    at Function.Module._load (module.js:462:3)
    at Function.Module.runMain (module.js:609:10)
    at startup (bootstrap_node.js:158:16)
    at bootstrap_node.js:578:3

columnName: =>foo
bar<=
columnName as hex: 666f6fe280a9626172
code: row["foo
bar"] = 1;
code as hex: 726f775b22666f6fe280a9626172225d203d20313b
Error running eval: SyntaxError: Invalid or unexpected token
    at Object.<anonymous> (/tmp/20171015-1113124/fun-with-escapes.js:25:14)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
    at Function.Module._load (module.js:462:3)
    at Function.Module.runMain (module.js:609:10)
    at startup (bootstrap_node.js:158:16)
    at bootstrap_node.js:578:3
@benjie

This comment has been minimized.

benjie commented Aug 15, 2017

That's absolutely fascinating @sehrope; thanks for explaining in such great detail!

dreamfall added a commit to sysadminmike/couch-to-postgres that referenced this issue Oct 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment