Skip to content

Commit

Permalink
fix(security): improve results object creation (#2574)
Browse files Browse the repository at this point in the history
Fixes a potential Prototype Pollution attack vulnerability reported by Vsevolod Kokorin (Slonser) of Solidlab
  • Loading branch information
wellwelwel authored Apr 9, 2024
1 parent 71115d8 commit 4a964a3
Show file tree
Hide file tree
Showing 6 changed files with 166 additions and 12 deletions.
2 changes: 1 addition & 1 deletion .nycrc
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"reporter": ["text", "lcov", "cobertura"],
"statements": 88,
"branches": 84,
"functions": 78,
"functions": 77,
"lines": 88,
"checkCoverage": true,
"clean": true
Expand Down
12 changes: 10 additions & 2 deletions lib/parsers/binary_parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,13 @@ function compile(fields, options, config) {
if (options.rowsAsArray) {
parserFn(`const result = new Array(${fields.length});`);
} else {
parserFn('const result = {};');
parserFn('const result = Object.create(null);');
parserFn(`Object.defineProperty(result, "constructor", {
value: Object.create(null),
writable: false,
configurable: false,
enumerable: false
});`);
}

// Global typeCast
Expand Down Expand Up @@ -154,7 +160,9 @@ function compile(fields, options, config) {
)}]`;
} else if (options.nestTables === true) {
tableName = helpers.srcEscape(fields[i].table);
parserFn(`if (!result[${tableName}]) result[${tableName}] = {};`);
parserFn(
`if (!result[${tableName}]) result[${tableName}] = Object.create(null);`,
);
lvalue = `result[${tableName}][${fieldName}]`;
} else if (options.rowsAsArray) {
lvalue = `result[${i.toString(10)}]`;
Expand Down
19 changes: 10 additions & 9 deletions lib/parsers/text_parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,6 @@ function compile(fields, options, config) {

const parserFn = genFunc();

/* eslint-disable no-trailing-spaces */
/* eslint-disable no-spaced-func */
/* eslint-disable no-unexpected-multiline */
parserFn('(function () {')('return class TextRow {');

// constructor method
Expand All @@ -134,7 +131,13 @@ function compile(fields, options, config) {
if (options.rowsAsArray) {
parserFn(`const result = new Array(${fields.length});`);
} else {
parserFn('const result = {};');
parserFn('const result = Object.create(null);');
parserFn(`Object.defineProperty(result, "constructor", {
value: Object.create(null),
writable: false,
configurable: false,
enumerable: false
});`);
}

const resultTables = {};
Expand All @@ -146,7 +149,9 @@ function compile(fields, options, config) {
}
resultTablesArray = Object.keys(resultTables);
for (let i = 0; i < resultTablesArray.length; i++) {
parserFn(`result[${helpers.srcEscape(resultTablesArray[i])}] = {};`);
parserFn(
`result[${helpers.srcEscape(resultTablesArray[i])}] = Object.create(null);`,
);
}
}

Expand Down Expand Up @@ -191,10 +196,6 @@ function compile(fields, options, config) {
parserFn('}');
parserFn('};')('})()');

/* eslint-enable no-trailing-spaces */
/* eslint-enable no-spaced-func */
/* eslint-enable no-unexpected-multiline */

if (config.debug) {
helpers.printDebugWithCode(
'Compiled text protocol row parser',
Expand Down
1 change: 1 addition & 0 deletions test/common.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ exports.createConnection = function (args) {
typeCast: args && args.typeCast,
namedPlaceholders: args && args.namedPlaceholders,
connectTimeout: args && args.connectTimeout,
nestTables: args && args.nestTables,
ssl: (args && args.ssl) ?? config.ssl,
};

Expand Down
72 changes: 72 additions & 0 deletions test/esm/unit/parsers/prototype-binary-results.test.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { test, describe, assert } from 'poku';
import { createConnection, describeOptions } from '../../../common.test.cjs';

const connection = createConnection().promise();

describe('Binary Parser: Prototype Sanitization', describeOptions);

Promise.all([
test(async () => {
const expected = [{}];
expected[0].test = 2;

const [results] = await connection.query('SELECT 1+1 AS `test`');

assert.notDeepStrictEqual(
results,
expected,
`Ensure "results" doesn't contain a standard object ({})`,
);
}),
test(async () => {
const expected = [Object.create(null)];
expected[0].test = 2;

const [results] = await connection.execute('SELECT 1+1 AS `test`');

assert.deepStrictEqual(results, expected, 'Ensure clean object "results"');
assert.strictEqual(
Object.getPrototypeOf(results[0]),
null,
'Ensure clean properties in results items',
);
assert.strictEqual(
typeof results[0].toString,
'undefined',
'Re-check prototypes (manually) in results columns',
);
assert.strictEqual(
typeof results[0].test.toString,
'function',
'Ensure that the end-user is able to use prototypes',
);
assert.strictEqual(
results[0].test.toString(),
'2',
'Ensure that the end-user is able to use prototypes (manually): toString',
);
assert.strictEqual(
results[0].test.toFixed(2),
'2.00',
'Ensure that the end-user is able to use prototypes (manually): toFixed',
);

results[0].customProp = true;
assert.strictEqual(
results[0].customProp,
true,
'Ensure that the end-user is able to use custom props',
);
}),
test(async () => {
const [result] = await connection.execute('SET @1 = 1;');

assert.strictEqual(
result.constructor.name,
'ResultSetHeader',
'Ensure constructor name in result object',
);
}),
]).then(async () => {
await connection.end();
});
72 changes: 72 additions & 0 deletions test/esm/unit/parsers/prototype-text-results.test.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { test, describe, assert } from 'poku';
import { createConnection, describeOptions } from '../../../common.test.cjs';

const connection = createConnection().promise();

describe('Text Parser: Prototype Sanitization', describeOptions);

Promise.all([
test(async () => {
const expected = [{}];
expected[0].test = 2;

const [results] = await connection.query('SELECT 1+1 AS `test`');

assert.notDeepStrictEqual(
results,
expected,
`Ensure "results" doesn't contain a standard object ({})`,
);
}),
test(async () => {
const expected = [Object.create(null)];
expected[0].test = 2;

const [results] = await connection.query('SELECT 1+1 AS `test`');

assert.deepStrictEqual(results, expected, 'Ensure clean object "results"');
assert.strictEqual(
Object.getPrototypeOf(results[0]),
null,
'Ensure clean properties in results items',
);
assert.strictEqual(
typeof results[0].toString,
'undefined',
'Re-check prototypes (manually) in results columns',
);
assert.strictEqual(
typeof results[0].test.toString,
'function',
'Ensure that the end-user is able to use prototypes',
);
assert.strictEqual(
results[0].test.toString(),
'2',
'Ensure that the end-user is able to use prototypes (manually): toString',
);
assert.strictEqual(
results[0].test.toFixed(2),
'2.00',
'Ensure that the end-user is able to use prototypes (manually): toFixed',
);

results[0].customProp = true;
assert.strictEqual(
results[0].customProp,
true,
'Ensure that the end-user is able to use custom props',
);
}),
test(async () => {
const [result] = await connection.query('SET @1 = 1;');

assert.strictEqual(
result.constructor.name,
'ResultSetHeader',
'Ensure constructor name in result object',
);
}),
]).then(async () => {
await connection.end();
});

0 comments on commit 4a964a3

Please sign in to comment.