SQL Injection with default blueprints in Waterline

[jamsea]

I have a model called "patients" which is using the default find blueprint in sails (it's controller definition is just module.exports = {};). I have a sinking suspicion it may have to do with node-mysql not actually supporting prepared statements (https://github.com/felixge/node-mysql#escaping-query-values).

I'm able to recreate the issue on any string field by passing in \\\" in "startsWith" as a where criteria. E.g. this:

http://localhost:3000/v1/patients?where={"firstName":{"startsWith":"\\\" OR 1=1; -- "}}

Returns all records in the patients table. Scary.

I have:

 "sails": "^0.11.2",
 "sails-mysql": "^0.11.0"

In my package.json. Has anyone else experienced anything similar?

[devinivy]

Hey, this is scary! I'm on it. Most likely lives in waterline-sequel. Worth noting that there's a general trend of trying to move these adapters to be backed by knex for query-building. See https://github.com/waterlinejs/postgresql-adapter for example.

[kevinburkeshyp]

Any idea whether this is limited to MySQL or whether other adapters are also affected?

-kevin

On Nov 16, 2015, at 19:05, devin ivy notifications@github.com wrote:

Hey, this is scary! I'm on it. Most likely lives in waterline-sequel. Worth noting that there's a general trend of trying to move these adapters to be backed by knex for query-building. See https://github.com/waterlinejs/postgresql-adapter for example.

—
Reply to this email directly or view it on GitHub.

[devinivy]

This would affect any adapter backed by waterline-sequel. I know that sails-postgresql would be included.

[evilpacket]

Can you tell me what all modules are impacted so that we can properly create an advisory for node security? Also any additional details or recommendations for remediation that you suggest for users at this time until a fix is made available.

[devinivy]

I can only speak to sails-mysql and sails-postgresql. @atiertant might want to look into sails-oracledb. @tjwebb I think that your new adapters are okay, but I they use waterline-sequel for some tasks, right?

The easiest way to test this against waterline adapters (which should be the only relevant modules) integration-style is to add tests to the waterline-adapter-test repo. I'll push a partial patch tonight that can hopefully be tightened-up and released soon. CC @particlebanana.

I can't complete every bit of work on this tonight, but @evilpacket if you would like to better understand the exploit, see,
https://github.com/balderdashy/waterline-sequel/blob/master/sequel/lib/criteriaProcessor.js#L777 and in turn,
https://github.com/balderdashy/waterline-sequel/blob/master/sequel/lib/utils.js#L39

And feel free to add to the PR, which I'll post shortly.

Suggestion: if you're using blueprints, disallow startsWith, endsWith, like, and contains queries. Might want @tjwebb or someone from balderdash/sailsjs-proper to chime in here.

[devinivy]

Related PR: balderdashy/waterline-sequel#66.

[kevinburkeshyp]

@evilpacket as far as I can determine, any application that passes untrusted input to any of Waterline's like, contains, startsWith, and endsWith queries, starting with waterline-sequel version 0.5.0 through the current version, is vulnerable to SQL injection. (The MySQL and Postgres adapters certainly use waterline-sequel, and the SQLite adapter probably uses it as well, but I haven't verified this. I'm not sure which other adapters use waterline-sequel).

Sails blueprint routes are especially vulnerable because they pass the where parameter directly to the database, though it's possible people are using these queries in regular applications as well.

[devinivy]

Actually it appears as though sails-postgresql is okay because it parametrizes those queries. % and _ still are not escaped in sails-postgresql. The best way to check, of course, is to try it out or write an adapter test.

[devinivy]

Now the patch should be complete, pending review. The more eyes on this the better!

[particlebanana]

Ok merged the waterline-sequel patch. Working on an integration test to prove it's fixed the I will publish a patch to npm for both waterline-sequel and sails-mysql. Because postgresql uses parameterized queries it doesn't seem to be affected.

Thanks for all your help @jamsea @devinivy and @kevinburkeshyp

[particlebanana]

Ok everything should be published so installing the latest version of sails-mysql (0.11.2) should take care of this. Can someone confirm thats true.

[kevinburkeshyp]

Because postgresql uses parameterized queries it doesn't seem to be affected.

confirmed! thanks @particlebanana @devinivy!

[devinivy]

👍