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

sql injection in "IN" statement #5671

Closed
leibale opened this issue Mar 29, 2016 · 10 comments
Closed

sql injection in "IN" statement #5671

leibale opened this issue Mar 29, 2016 · 10 comments

Comments

@leibale
Copy link

leibale commented Mar 29, 2016

i'm using mssql.
sequelize version is 3.19.3.

sample:

const Sequelize = require('sequelize'),
    database = new Sequelize( ... );

database.query('SELECT * FROM Table WHERE Id IN (:ids)', {
  replacements: {
    ids: [1, 2, 3, "'asdf"]
  }
}).catch(function (err) {
  // got sql syntax error
});

because tedious doesn't support arrays as parameters you must add a parameter for every item in the array and use it in the query (IN (:ids0, :ids1, :ids2, :ids3)).

now there is an option to do sql injection.

@mickhansen
Copy link
Contributor

Sequelize handles replacements before passing it along to the driver IIRC.
Under what case are you sayings there's an injection issue? IN (:ids0, :ids1, :ids2, :ids3) would not be vulnerable.

@leibale
Copy link
Author

leibale commented Mar 29, 2016

Let me explain myself.
Sequelize accepts strings in an array as parameters.
Instead of adding the parameters as a parameter, it injects them as strings straight into the sql query.

example

const Sequelize = require('sequelize'),
    database = new Sequelize( ... );

database.query('SELECT * FROM Table WHERE Name IN (:names)', {
  replacements: {
    names: ["test", "'); DELETE Table WHERE Id = 1 --')"]
  }
});

the query will be

SELECT Id FROM Table WHERE Name IN ('test', '\'); DELETE Table WHERE Id = 1 --')

in mssql backslash isn't a special character.

@mickhansen
Copy link
Contributor

\ does not escape ' in mssql?
Well original implementation of mssql in sequelize does have a major bug then, anything we escape should not be injectable.

@leibale
Copy link
Author

leibale commented Mar 29, 2016

in mssql \ doesn't escape ', instead of escaping with \ you should escape with double '.
pls let me know if someone started to deal with it.

tnx

@mickhansen
Copy link
Contributor

Yeah it should actually already be doing that: https://github.com/sequelize/sequelize/blob/master/lib/sql-string.js#L61

@leibale
Copy link
Author

leibale commented Mar 29, 2016

so this is not working as you expected.. i'm debugging this now.

@leibale
Copy link
Author

leibale commented Mar 29, 2016

found the mistake. creating PR now.

@leibale
Copy link
Author

leibale commented Mar 29, 2016

somebody already fixed this one in 23952a2.
when are you going to publish a new version to npm?

@leibale leibale closed this as completed Mar 29, 2016
@mickhansen
Copy link
Contributor

That version is already on npm, as is evident from the tags on that commit

@leibale
Copy link
Author

leibale commented Mar 29, 2016

right, my mistake.
tnx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants