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

critical(sql): escape limit and order by arguments to counter possibl… #5167

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
critical(sql): escape limit and order by arguments to counter possibl…
…e injection attack
  • Loading branch information
mickhansen authored and spence committed Jan 7, 2016
commit f282d85e60e3df5e57ecdb82adccb4eaef404f03
5 changes: 5 additions & 0 deletions changelog.md
@@ -1,3 +1,8 @@
# Future
- [CRITICAL] Fixed injection vulnerability for order/limit
- [FIXED] MySQL throws error when null GEOMETRY data results in empty buffer [#4953](https://github.com/sequelize/sequelize/issues/4953)
- [ADDED] Support for benchmarking the execution time for SQL queries [#488](https://github.com/sequelize/sequelize/issues/488)

# 3.13.0
- [FIXED] timestamp columns are no longer undefined for associations loaded with `separate`. [#4740](https://github.com/sequelize/sequelize/issues/4740)
- [FIXED] Mark unscoped model as `.scoped`, to prevent injection of default scope on includes [#4663](https://github.com/sequelize/sequelize/issues/4663)
Expand Down
6 changes: 3 additions & 3 deletions lib/dialects/abstract/query-generator.js
Expand Up @@ -1742,12 +1742,12 @@ var QueryGenerator = {
addLimitAndOffset: function(options, model) {
var fragment = '';
if (options.offset && !options.limit) {
fragment += ' LIMIT ' + options.offset + ', ' + 18440000000000000000;
fragment += ' LIMIT ' + this.escape(options.offset) + ', ' + 18440000000000000000;
} else if (options.limit) {
if (options.offset) {
fragment += ' LIMIT ' + options.offset + ', ' + options.limit;
fragment += ' LIMIT ' + this.escape(options.offset) + ', ' + this.escape(options.limit);
} else {
fragment += ' LIMIT ' + options.limit;
fragment += ' LIMIT ' + this.escape(options.limit);
}
}

Expand Down
4 changes: 2 additions & 2 deletions lib/dialects/mssql/query-generator.js
Expand Up @@ -590,11 +590,11 @@ var QueryGenerator = {
}

if (options.offset || options.limit) {
fragment += ' OFFSET ' + offset + ' ROWS';
fragment += ' OFFSET ' + this.escape(offset) + ' ROWS';
}

if (options.limit) {
fragment += ' FETCH NEXT ' + options.limit + ' ROWS ONLY';
fragment += ' FETCH NEXT ' + this.escape(options.limit) + ' ROWS ONLY';
}
}

Expand Down
4 changes: 2 additions & 2 deletions lib/dialects/postgres/query-generator.js
Expand Up @@ -422,8 +422,8 @@ var QueryGenerator = {

addLimitAndOffset: function(options) {
var fragment = '';
if (options.limit) fragment += ' LIMIT ' + options.limit;
if (options.offset) fragment += ' OFFSET ' + options.offset;
if (options.limit) fragment += ' LIMIT ' + this.escape(options.limit);
if (options.offset) fragment += ' OFFSET ' + this.escape(options.offset);

return fragment;
},
Expand Down
15 changes: 0 additions & 15 deletions lib/dialects/sqlite/query-generator.js
Expand Up @@ -89,21 +89,6 @@ var QueryGenerator = {
return !!value ? 1 : 0;
},

addLimitAndOffset: function(options){
var fragment = '';
if (options.offset && !options.limit) {
fragment += ' LIMIT ' + options.offset + ', ' + 10000000000000;
} else if (options.limit) {
if (options.offset) {
fragment += ' LIMIT ' + options.offset + ', ' + options.limit;
} else {
fragment += ' LIMIT ' + options.limit;
}
}

return fragment;
},

addColumnQuery: function(table, key, dataType) {
var query = 'ALTER TABLE <%= table %> ADD <%= attribute %>;'
, attributes = {};
Expand Down
76 changes: 76 additions & 0 deletions test/unit/sql/offset-limit.test.js
@@ -0,0 +1,76 @@
'use strict';

/* jshint -W110 */
var Support = require(__dirname + '/../support')
, DataTypes = require(__dirname + '/../../../lib/data-types')
, Model = require(__dirname + '/../../../lib/model')
, util = require('util')
, expectsql = Support.expectsql
, current = Support.sequelize
, sql = current.dialect.QueryGenerator;

// Notice: [] will be replaced by dialect specific tick/quote character when there is not dialect specific expectation but only a default expectation

suite(Support.getTestDialectTeaser('SQL'), function() {
suite('offset/limit', function () {
var testsql = function (options, expectation) {
var model = options.model;

test(util.inspect(options, {depth: 2}), function () {
return expectsql(
sql.addLimitAndOffset(
options,
model
),
expectation
);
});
};

testsql({
limit: 10,
order: [
['email', 'DESC'] // for MSSQL
]
}, {
default: ' LIMIT 10',
mssql: ' OFFSET 0 ROWS FETCH NEXT 10 ROWS ONLY'
});

testsql({
limit: 10,
offset: 20,
order: [
['email', 'DESC'] // for MSSQL
]
}, {
default: ' LIMIT 20, 10',
postgres: ' LIMIT 10 OFFSET 20',
mssql: ' OFFSET 20 ROWS FETCH NEXT 10 ROWS ONLY'
});

testsql({
limit: "';DELETE FROM user",
order: [
['email', 'DESC'] // for MSSQL
]
}, {
default: " LIMIT ''';DELETE FROM user'",
mysql: " LIMIT '\\';DELETE FROM user'",
mssql: " OFFSET 0 ROWS FETCH NEXT N''';DELETE FROM user' ROWS ONLY"
});

testsql({
limit: 10,
offset: "';DELETE FROM user",
order: [
['email', 'DESC'] // for MSSQL
]
}, {
sqlite: " LIMIT ''';DELETE FROM user', 10",
postgres: " LIMIT 10 OFFSET ''';DELETE FROM user'",
mysql: " LIMIT '\\';DELETE FROM user', 10",
mssql: " OFFSET N''';DELETE FROM user' ROWS FETCH NEXT 10 ROWS ONLY"
});
});
});
9 changes: 7 additions & 2 deletions test/unit/sql/select.test.js
Expand Up @@ -36,9 +36,14 @@ suite(Support.getTestDialectTeaser('SQL'), function() {
],
where: {
email: 'jon.snow@gmail.com'
}
},
order: [
['email', 'DESC']
],
limit: 10
}, {
default: "SELECT [email], [first_name] AS [firstName] FROM [User] WHERE [User].[email] = 'jon.snow@gmail.com';"
default: "SELECT [email], [first_name] AS [firstName] FROM [User] WHERE [User].[email] = 'jon.snow@gmail.com' ORDER BY [email] DESC LIMIT 10;",
mssql: "SELECT [email], [first_name] AS [firstName] FROM [User] WHERE [User].[email] = N'jon.snow@gmail.com' ORDER BY [email] DESC OFFSET 0 ROWS FETCH NEXT 10 ROWS ONLY;"
});

testsql({
Expand Down