Vulnerability disucssion

[brianc]

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]

Kudos to @brianc for immediately releasing a fix for this. Given the severity, turnaround time and response was great.

[benjie]

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]

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

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

[davidje13]

Since this issue is still mentioned prominently in the documentation, I think it's worth noting that 72db790 has since removed this risk at source (by removing the dynamic eval-ed code entirely).

It would be good to get confirmation that this library no-longer contains any use of eval or Function, since that's a really easy way to get confidence that nothing too damaging is likely to be discovered in the future.

[benjie]

And just to round out more of the discussion (not that it's relevant to the current node-postgres implementation), JSON is now a concrete subset of JS, so the edge cases @sehrope mentions above are no longer of concern and JSON.stringify is now sufficient.

https://github.com/tc39/proposal-json-superset

[brianc]

It would be good to get confirmation that this library no-longer contains any use of eval or Function, since that's a really easy way to get confidence that nothing too damaging is likely to be discovered in the future.

Yup, there's no new Function or eval in the codebase anymore.

[rluvaton]

There is no way to add it back for performance improvements? I see you fixed that security issue and it was removed due to memory leak (which I have a feeling it can be solved without removing it)