SQL injection is possible with mysql.escape function #342

Closed
sebdej opened this Issue Nov 23, 2012 · 12 comments

Comments

Projects
None yet
3 participants

sebdej commented Nov 23, 2012

SQL injection is possible despite using mysql.escape:

var object = { 'a` = 2; INSERT INTO test_inject SET `a': 1 };

conn.query('INSERT INTO test_inject SET ?', object, function (err, results) { });

SqlString.objectToValues is the problem, it doesn't properly escape the object keys.

Collaborator

dresende commented Nov 23, 2012

Well.. it is. But one of the flags of the connection disallows multiple statements so you don't need to be worried about statement injection :-)

dresende closed this Nov 23, 2012

Collaborator

dresende commented Nov 23, 2012

Of course you can remove that flag but that's not the default behavior. If you remove it, use at your own risk.

sebdej commented Nov 23, 2012

With multipleStatements = false I still can do :

var update = { 'a`=1 WHERE \'\' <> \'': 1 };
var where = { 'id`\' OR `a': 1 };

var query = conn.query('UPDATE test_inject SET ? WHERE ?', [ update, where ], function (err, results) { });

It's again SQL injection (even if this code is a bad practice). The solution is simple, escape the ` character in column names.

Collaborator

dresende commented Nov 23, 2012

I don't understand. What is that supposed to do?

sebdej commented Nov 23, 2012

From documentation :

In order to avoid SQL Injection attacks, you should always escape any user provided data before using it inside a SQL query. You can do so using the connection.escape() method:
...
Alternatively, you can use ? characters as placeholders for values you would like to have escaped like this

The problem is not to understand what my code is doing, the problem is that you can inject SQL in column identifiers but the documentation says it's not possible if you use the escape function.

Why would you allow the user to specify the column name? Sorting / grouping / filtering data in a CRUD service for example.

If this you're still not convinced by this example, well forget it, I will just patch my local copy.

Collaborator

dresende commented Nov 23, 2012

I'm not completely convinced, but that doesn't mean we can't do something about it. Still, your example is a very bad practice. Even if you allow a user to do some kind of sorting you should match against a list of allowable columns.. but if you're going to patch feel free to post the change or do a pull request :)

Collaborator

felixge commented Nov 24, 2012

@dresende I think the issue is that we don't escape object keys here: https://github.com/felixge/node-mysql/blob/master/lib/protocol/SqlString.js#L111

I think we should escape them, and that should close this injection problem.

Collaborator

dresende commented Nov 24, 2012

Yeah, I'll look into what is a possible identifier and try to escape anything else.

dresende reopened this Nov 24, 2012

sebdej commented Nov 26, 2012

Patch is available here: https://github.com/sebdej/node-mysql/commit/075080bcbf1d9e540e1dc774c79b1fa53230d3cb

Sorry I'm a GitHub newbie, I don't known if it is the right way to proceed.

Collaborator

dresende commented Nov 26, 2012

I'll look into it. For the best github experience, I advise you to do a branch on your fork and then commit changes to it. After that you can go to github and create the pull request to this repository. It's easy and allows you to make different requests without mixing commits.

But I'll look into your patch.

dresende closed this in bc8f3df Nov 26, 2012

sebdej commented Nov 27, 2012

Thank you, but you should forbid qualified identifier as object key :

console.log(mysql.escape({ 'a.b.c': 1 })); // `a`.`b`.`c` = 1

It's a potential vulnerability.

Collaborator

dresende commented Nov 27, 2012

Ok, I'll change it. Basically, you want a 2nd param that can forbid qualified identifiers to avoid fetching from tables/databases you don't want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment