Handle escaping for different SQL_MODE parameters

[kiprobinson]

mysql.escape() and prepared statements do not escape strings properly with different SQL_MODE parameters are used.

My company has our MySQL database default to NO_BACKSLASH_ESCAPES mode. In this mode ' must be escaped as '', rather than \'. We have found out (the hard way) that this library always assumes MySQL is operating in standard mode.

This is especially surprising in prepared statements (or what appear to be prepared statements but just borrow the syntax):

dbconn.query('select * from mytable where tableName = ?', [userName], function(e, r) { ... });

In other languages, code like this doesn't need to care what SQL_MODE is set to, the DB driver handles that for you, you just parameterize your queries and you're protected from SQL Injection.

As a workaround, I'm now running SET SESSION SQL_MODE = '' immediately after creating the DB connection. But it would be much better if this were somehow incorporated as an option. Either a global config for the mysql object, or a config option on the DB connection. Or at the very least there should be a big warning somewhere in the documentation.

Example of a potential vulnerability:

var username = "' OR userid=17 --";
dbconn.query('select * from users where username = ?', [username], function(e,r) { ... });

In this case, query is executed like:

select * from users where username = '\' OR userid=17 --'

In NO_BACKSLASH_ESCAPES mode, that means: where username is a backslash or userid=17. Assuming there's no user named backslash, but userid 17 is somehow interesting, then you've exposed data.

[dougwilson]

Pull requests for any of the options are certainly welcome!

[kiprobinson]

Created pull request 1830. This is just a warning in the ReadMe.

I may try to implement something. Ideally this would be a configuration on the DB connection, but mysql.escape() doesn't take a connection object. I wouldn't want to require passing an optional flag to mysql.escape() everytime either, as developers would be likely to forget it once in a while. So I'm thinking the best course of action would be to add a global config for the module, something like:

var mysql = require('mysql');
mysql.config({
  noBackslashEscapes: true
});

From then on, any calls to mysql.escape(), or when running .query() with ? replacement variables, would escape strings using the rules of NO_BACKSLASH_ESCAPES mode.

What do you think of this idea? I don't want to go through the effort of learning your code and coding it if this is just going to be rejected. :)

If there is another approach you would prefer please let me know.

[dougwilson]

It's possible, but then you'll be in a similar (or worse?) situation if you are connecting to two servers in the same process where one has NO_BACKSLASH_ESCAPES and one does not.

[kiprobinson]

Yes I thought about that too. In my experience that would be uncommon, but maybe for someone else it is not.

I just noticed in re-reading your docs that there is also a connection.escape() method. So I think you could make it a connection-specific flag. And add a warning around the part in the ReadMe where you suggest doing mysql.escape(). Something like this:

var mysql      = require('mysql');
var conn1 = mysql.createConnection({
  host     : 'example.org',
  user     : 'bob',
  password : 'secret'
});
var conn2 = mysql.createConnection({
  host     : 'example.org',
  user     : 'bob',
  password : 'secret',
  noBackslashEscapes : true
});

console.log(conn1.escape("hello ' world \\ goodbye"));   //prints:  'Hello \' world \\ goodbye'
console.log(conn2.escape("hello ' world \\ goodbye"));   //prints:  'Hello '' world \ goodbye'
console.log(mysql.escape("hello ' world \\ goodbye"));   //prints:  'Hello \' world \\ goodbye'
console.log(mysql.escape("hello ' world \\ goodbye", {noBackslashEscapes:true}));   //prints:  'Hello '' world \ goodbye'

[dougwilson]

Without telemetry, it's impossible to know what is common and what is not; best just to support as much as possible. I think that makes sense. I'm not very familiar with the NO_BACKSLASH_ESCAPES and didn't find a section on how things like 0x00 0xa0 and the like are supposed to be escaped when that is enabled. Do you know all the escaping rules when NO_BLACKSLASH_ESCAPES is on? Maybe some of those just don't need escaping in that case?

[dougwilson]

In fact, it looks like the server will indicate in the initial handshake if it is in NO_BACKSLASH_ESCAPES mode, so I would say use that from the server to set an internal flag instead of requiring the user to specify it as a connection option so the handling is always correct.

[kiprobinson]

Hmm okay. I don't think I've ever encountered those kinds of characters in strings. My company uses the flag because of porting from SQL Server to MySQL some years back, and the no backslash mode makes it behave like SQL Server.

Using the handshake value might actually break the code for anyone using the same workaround as me: In my production code I'm calling SET SESSION SQL_MODE = '' after connecting. If the handshake indicates no_backslash_escapes mode, but then I change it for my session, I'm not sure if the connection object would know about that. (I don't know much about the low-level stuff.)

[dougwilson]

Ah, yea. So that would mean that after every query the status flags from the OK packet should be checked for the NO_BACKSLASH_ESCAPES and update as necessary, which would automatically account for that I believe.

Re chars in string, in order to implement escapes without backslashes, probably need to understand what the escaping rules actually are. I think the rules for ' is clear: it becomes ''. What about the " character? I assume that becomes "" as well? Perhaps each character that needs to be escaped is just doubled?

[kiprobinson]

Why are you escaping " anyway? You always add the single-quote before and after the string, so you know your string is always going to use single-quote as the delimiter.

console.log(mysql.escape("'Test'")); //prints: '\'Test\''
console.log(mysql.escape('"Test"')); //prints: '\"Test\"',  but you could just return: '"Test"'

[dougwilson]

That's how the module was before I was here, so don't have an answer for you on the why.