Security issue with param escaping?

[thekiur]

Hi, im running node-mysql latest on node-latest.
Somebody using the acunetix vulnerability scanner has triggered this error:
UNKNOWN COLUMN '$acunetix' IN WHERE CLAUSE.
The query: SELECT id, email FROM accounts WHERE username = ?

How is this possible? Its very dangerous to our application, please respond quickly.

[mk-pmb]

The problem seems to be about params that are not strings. Although I'll continue to sanitize all my user inputs (to avoid username impersonation attacks like admіn posing as admin), I'd expect the query engine to convert any param to a string if it should have been one in the first place. If it already was, String(param) should be of low cost.

[sidorares]

@thekiur @mk-pmb can you post code samples?

[thekiur]

We can confirm that the problem is caused by passing objects to the query call.
The objects come from the express bodyParser middleware.
We were simply passing req.body.username as the parameter for that query.
The acunetic vulnerability tester injected an object there.
We are not sure on the severity of this issue, but its unexpected to say atleast.
As we experienced, this can crash a live application in production mode if you dont expect any db errors.

There is no code to show: its as simple as passing a req.body.something to the .query call of node-mysql when using express with the bodyparser middleware. Running the vulnerability scanner against https://gist.github.com/ssafejava/9a2d77704712a8769322 causes the exception to be thrown.

[dougwilson]

This is not an issue with escaping with this library; this library is properly escaping all values and column names. The security issue is just with the way you are combining express and this library, such that you were expecting to get a string from express, so you were only expecting the ? to expand according to string rules.

req.body properties can be anything with bodyParser and as such you need to at least verify what you are using is a string before passing to your query.

[mk-pmb]

I consider prepared statements as intended to mitigate lack of input validation in the params in general. Therefor, limiting it to the case where input has already been validated as being a string, in my opinion misses the point.
Yours, MK

[dougwilson]

These are not prepared statements, they are done client-side and have various rules for how ? is replaced depending on the data type, which is documented. If you want to be sure you are using the string-based ? replacement though the API, you have to give the API a string. If you don't want to validate at all, you can use the String() function:
conn.query('SELECT * FROM user WHERE username = ?', [String(req.body.username)]')

The purpose if it doing stuff different for objects is to help people who want to easily use SET:
conn.query('UPDATE user SET ? WHERE id = ?', [{username: 'bob', password: '1234'}, 43])

Please see the "Different value types are escaped differently, here is how:" section in https://github.com/felixge/node-mysql#escaping-query-values

[mk-pmb]

I see. Looks like an unlucky case of embrace and extend. I wish you had opted for something like ?? in that case. Probably too late to change the interface?

Edit: Not really embrace and extend, as you wrote they aren't prepared statements. Rather just a pitfall for people who learn from tutorials and conclude topical similarity from visual similarity.
Edit 2: I see, ?? is already used for column names.

[mk-pmb]

I can't see how it's the type system's fault when programmers assume that a mechanism that looks like prepared statements will defuse any data they pass in. Let's at least blame it at the programmers for trusting visual similarity instead of reading the manual thoroughly.

[dougwilson]

@mk-pmb sure, though this module only has a small Readme, which has all the ? stuff explained (https://github.com/felixge/node-mysql#escaping-query-values), so it's not even some weird hidden feature. Unfortunately if people on the Internet are writing tutorials about this module and giving incomplete or wrong information, it's hard for us to even try to police that.

[skeggse]

@mk-pmb it's the programmers role to understand the libraries he/she is using at least to the extend they are documented before including them in any production environment. If the library isn't fully documented, that's on the creator, but since this is an open-source world you can't really blame somebody for dedicating their time towards creating something for free.

Inferring functionality from syntax is useful, but think rationally: if the ? operator accepts strings, would it only accepts strings? What if it accepted other data types? Jumping to blind assumptions about a library is a recipe for disaster, and good security protocols still mandate data validation.

Libraries and languages that make it easier to start developing are extremely useful, but I fear it gives a novice developer a misplaced sense of confidence. It's easy to build a small application, and when it "just works" assume nothing could possibly go wrong.

[mk-pmb]

Jumping to blind assumptions about a library is a recipe for disaster, and good security protocols still mandate data validation.

I agree with that. And still, lots of people do it. So for all software that I manage, I'll try and have it be compatible with everyday flawed humans, in hopes to lessen the risk and impact of errors in software based on mine, written by fallible humans.

BOfH would ship a GNU/Linux distro where the default shell acts fully like bash, just that on every line starting with an uppercase letter, the meaning of && and || is swapped. Might even document it properly. You'd read the manual and probably wouldn't use it. However, if the next day a toy drone crashes into your car because it's pilot didn't read the manual as thoroughly as you did, your expectations of how humans should act had much less impact than how they really do act. And I'd still partially blame that BOfH.

Update: Thanks for making it opt-in.

[dougwilson]

Please, this issue doesn't need any more comments. It is still open as a tracking issue for me. There are coming changes that will affects this module and even things like express which will make any kind of "shoot yourself in the foot" operations opt-in. As an example, for this module ? really should strictly only result in a single entry in the SQL (i.e. numbers, strings, null, etc.). Anything over that should be opt-in (on the connection-level or one-off on the query level to reduce accidental exposure.

These are changes that are coming I listed, not speculation. Please just know that this issue is taken seriously.

[SystemParadox]

Are there any circumstances where this would lead to an injection attack?

As far as I can work out so far this appears to only ever result in syntax errors.

[dresende]

@SystemParadox: I don't think so. The report seems to be badly explained and seems to be related to constructing SQL based on user input without any check.

Good usage:

db.query("SELECT * FROM users WHERE id = ?", [ +req.params.id ], next);

No harm on that, casting forces it to be a number. Even if it wasn't a number and the + was omitted, it's just fine (or else you would have problems when UPDATing columns with binary data - there's tests for that).

The problem here seems to be with something more like:

// BAD! BAD!
db.query("SELECT * FROM " + req.params.table + " WHERE ....", next);

[skeggse]

@SystemParadox yeah I just took a look at the formatting and escaping code. I don't see any way that passing unvalidated data to be interpolated into the query could result in an injection vulnerability. Without validation you can easily get a syntax error.