Skip to content

Commit

Permalink
fix(security): improve supportBigNumbers and bigNumberStrings sanitiz…
Browse files Browse the repository at this point in the history
…ation (#2572)

Fixes a potential RCE attack vulnerability reported by Vsevolod Kokorin (Slonser) of Solidlab
  • Loading branch information
wellwelwel authored Apr 9, 2024
1 parent 8a818ce commit 74abf9e
Show file tree
Hide file tree
Showing 4 changed files with 160 additions and 24 deletions.
9 changes: 6 additions & 3 deletions lib/parsers/binary_parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@ for (const t in Types) {
}

function readCodeFor(field, config, options, fieldNum) {
const supportBigNumbers =
options.supportBigNumbers || config.supportBigNumbers;
const bigNumberStrings = options.bigNumberStrings || config.bigNumberStrings;
const supportBigNumbers = Boolean(
options.supportBigNumbers || config.supportBigNumbers,
);
const bigNumberStrings = Boolean(
options.bigNumberStrings || config.bigNumberStrings,
);
const timezone = options.timezone || config.timezone;
const dateStrings = options.dateStrings || config.dateStrings;
const unsigned = field.flags & FieldFlags.UNSIGNED;
Expand Down
47 changes: 26 additions & 21 deletions lib/parsers/text_parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@ for (const t in Types) {
}

function readCodeFor(type, charset, encodingExpr, config, options) {
const supportBigNumbers =
options.supportBigNumbers || config.supportBigNumbers;
const bigNumberStrings = options.bigNumberStrings || config.bigNumberStrings;
const supportBigNumbers = Boolean(
options.supportBigNumbers || config.supportBigNumbers,
);
const bigNumberStrings = Boolean(
options.bigNumberStrings || config.bigNumberStrings,
);
const timezone = options.timezone || config.timezone;
const dateStrings = options.dateStrings || config.dateStrings;

Expand Down Expand Up @@ -85,22 +88,24 @@ function compile(fields, options, config) {
db: field.schema,
table: field.table,
name: field.name,
string: function(encoding = field.encoding) {
string: function (encoding = field.encoding) {
if (field.columnType === Types.JSON && encoding === field.encoding) {
// Since for JSON columns mysql always returns charset 63 (BINARY),
// we have to handle it according to JSON specs and use "utf8",
// see https://github.com/sidorares/node-mysql2/issues/1661
console.warn(`typeCast: JSON column "${field.name}" is interpreted as BINARY by default, recommended to manually set utf8 encoding: \`field.string("utf8")\``);
console.warn(
`typeCast: JSON column "${field.name}" is interpreted as BINARY by default, recommended to manually set utf8 encoding: \`field.string("utf8")\``,
);
}

return _this.packet.readLengthCodedString(encoding);
},
buffer: function() {
buffer: function () {
return _this.packet.readLengthCodedBuffer();
},
geometry: function() {
geometry: function () {
return _this.packet.parseGeometryValue();
}
},
};
}

Expand All @@ -109,9 +114,7 @@ function compile(fields, options, config) {
/* eslint-disable no-trailing-spaces */
/* eslint-disable no-spaced-func */
/* eslint-disable no-unexpected-multiline */
parserFn('(function () {')(
'return class TextRow {'
);
parserFn('(function () {')('return class TextRow {');

// constructor method
parserFn('constructor(fields) {');
Expand All @@ -127,22 +130,22 @@ function compile(fields, options, config) {

// next method
parserFn('next(packet, fields, options) {');
parserFn("this.packet = packet;");
parserFn('this.packet = packet;');
if (options.rowsAsArray) {
parserFn(`const result = new Array(${fields.length});`);
} else {
parserFn("const result = {};");
parserFn('const result = {};');
}

const resultTables = {};
let resultTablesArray = [];

if (options.nestTables === true) {
for (let i=0; i < fields.length; i++) {
for (let i = 0; i < fields.length; i++) {
resultTables[fields[i].table] = 1;
}
resultTablesArray = Object.keys(resultTables);
for (let i=0; i < resultTablesArray.length; i++) {
for (let i = 0; i < resultTablesArray.length; i++) {
parserFn(`result[${helpers.srcEscape(resultTablesArray[i])}] = {};`);
}
}
Expand All @@ -154,7 +157,7 @@ function compile(fields, options, config) {
parserFn(`// ${fieldName}: ${typeNames[fields[i].columnType]}`);
if (typeof options.nestTables === 'string') {
lvalue = `result[${helpers.srcEscape(
fields[i].table + options.nestTables + fields[i].name
fields[i].table + options.nestTables + fields[i].name,

This comment has been minimized.

Copy link
@MrSwitch

MrSwitch Apr 15, 2024

Why does this line end with a comma ,?

This comment has been minimized.

Copy link
@wellwelwel

wellwelwel Apr 15, 2024

Author Collaborator

Hi @MrSwitch, this is automatically inserted by Prettier.

This comment has been minimized.

Copy link
@MrSwitch

MrSwitch Apr 15, 2024

Thanks, i realised how dumb i was being and tried to remove my comment... and failed.

This comment has been minimized.

Copy link
@MrSwitch

MrSwitch Apr 15, 2024

Thanks @wellwelwel for your patch #2591. It passed our tests with success 💯 🎉

)}]`;
} else if (options.nestTables === true) {
lvalue = `result[${helpers.srcEscape(fields[i].table)}][${fieldName}]`;
Expand All @@ -172,11 +175,13 @@ function compile(fields, options, config) {
fields[i].characterSet,
encodingExpr,
config,
options
options,
);
if (typeof options.typeCast === 'function') {
parserFn(`${lvalue} = options.typeCast(this.wrap${i}, function() { return ${readCode} });`);
} else {
parserFn(
`${lvalue} = options.typeCast(this.wrap${i}, function() { return ${readCode} });`,
);
} else {
parserFn(`${lvalue} = ${readCode};`);
}
}
Expand All @@ -193,11 +198,11 @@ function compile(fields, options, config) {
if (config.debug) {
helpers.printDebugWithCode(
'Compiled text protocol row parser',
parserFn.toString()
parserFn.toString(),
);
}
if (typeof options.typeCast === 'function') {
return parserFn.toFunction({wrap});
return parserFn.toFunction({ wrap });
}
return parserFn.toFunction();
}
Expand Down
66 changes: 66 additions & 0 deletions test/esm/unit/parsers/big-numbers-strings-sanitization.test.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { describe, test, assert } from 'poku';
import { createConnection, describeOptions } from '../../../common.test.cjs';

const connection = createConnection().promise();

const sql = 'SELECT 9007199254740991+100 AS `total`';

describe('bigNumberStrings Sanitization', describeOptions);

Promise.all([
test(async () => {
const [results] = await connection.query({
sql,
supportBigNumbers: true,
bigNumberStrings: true,
});

assert.strictEqual(
typeof results[0].total,
'string',
'Valid bigNumberStrings enabled',
);
}),
test(async () => {
const [results] = await connection.query({
sql,
supportBigNumbers: false,
bigNumberStrings: false,
});

assert.strictEqual(
typeof results[0].total,
'number',
'Valid bigNumberStrings disabled',
);
}),

test(async () => {
const [results] = await connection.query({
sql,
supportBigNumbers: 'text',
bigNumberStrings: 'text',
});

assert.strictEqual(
typeof results[0].total,
'string',
'bigNumberStrings as a random string should be enabled',
);
}),
test(async () => {
const [results] = await connection.query({
sql,
supportBigNumbers: '',
bigNumberStrings: '',
});

assert.strictEqual(
typeof results[0].total,
'number',
'bigNumberStrings as an empty string should be disabled',
);
}),
]).then(async () => {
await connection.end();
});
62 changes: 62 additions & 0 deletions test/esm/unit/parsers/support-big-numbers-sanitization.test.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import { describe, test, assert } from 'poku';
import { createConnection, describeOptions } from '../../../common.test.cjs';

const connection = createConnection().promise();

const sql = 'SELECT 9007199254740991+100 AS `total`';

describe('supportBigNumbers Sanitization', describeOptions);

Promise.all([
test(async () => {
const [results] = await connection.query({
sql,
supportBigNumbers: true,
});

assert.strictEqual(
typeof results[0].total,
'string',
'Valid supportBigNumbers enabled',
);
}),
test(async () => {
const [results] = await connection.query({
sql,
supportBigNumbers: false,
});

assert.strictEqual(
typeof results[0].total,
'number',
'Valid supportBigNumbers disabled',
);
}),

test(async () => {
const [results] = await connection.query({
sql,
supportBigNumbers: 'text',
});

assert.strictEqual(
typeof results[0].total,
'string',
'supportBigNumbers as a random string should be enabled',
);
}),
test(async () => {
const [results] = await connection.query({
sql,
supportBigNumbers: '',
});

assert.strictEqual(
typeof results[0].total,
'number',
'supportBigNumbers as an empty string should be disabled',
);
}),
]).then(async () => {
await connection.end();
});

0 comments on commit 74abf9e

Please sign in to comment.