Not properly escaping quotes in setFields

[seanlynch]

> console.log(squel.insert().into('buh').setFields({foo: "bar'baz"}).toString());
INSERT INTO buh (foo) VALUES ('bar'baz')

This seems about as basic as it gets.

[seanlynch]

Just to be clear this bug is almost certainly causing SQL injection vulnerabilities in users' apps. I've reported it to [email protected] as well since I've received no response from the developers.

[fredck]

This is the one thing you would count a SQL builder library would do for you. Kinda depressing to figure out it does now. Is this by design? I think it is worth an urgent check.

[Nox-404]

Hello there,
I don't think this is really an issue, to avoid injection use prepared statements with your sql driver.
You can then use .toParams() to split your query and values

[Nox-404]

Here is an example (pool is a mysql2 pool):

// If the injection works, the query will get more than one row
const injection = "1') union SELECT id FROM table2 WHERE (id > '1";
const queryBuilder = squel.select().field('id').from('table1').where('id = ?', injection);
const q = queryBuilder.toParam();

// Without prepared statement
pool.execute(queryBuilder.toString(), [], (err, results, headers) => {
  console.log('not-prepared', err, results.length > 1);
});
// output: not-prepared null true
// The injection worked and retrieved all ids from table2

// With prepared statements
pool.execute(q.text, q.values, (err, results, headers) => {
  console.log('prepared', err, results.length > 1);
});
// output: prepared null false
// The injection did not work

[seanlynch]

I know how to use prepared statements. They do not work for my use case because I wish to output SQL to use elsewhere. But as it turns out I didn't need Squel at all.

This is most definitely an issue, because the functionality is there.

[Nostradamos]

I like squel but this is a killing stroke for squel if it remains unfixed. Already providing an unsafe toString() method is provoking unsafe code. Please either remove any way to have the parameters mixed in unsafely or add a method which properly sanitizes parameters. Or at least add proper warnings to the toString() function or anywhere developers can read it.

[seanlynch]

NPM contacted the author back in April and issued a security advisory about this. I think at this point the only responsible conclusion is that this project is abandoned and should be removed from NPM.

https://nodesecurity.io/advisories/575

[Arilas]

I think that squel just need to throw exception when user trying to call toString() and have params. Don't ever use toString() when you have ability to use prepare statements. Even for logging it's security hole to store sensitive user data in log

[vekexasia]

damn i need toString. any way to get properly escaped sql?

[Nox-404]

Maybe squel could add an escape method so that users can call that escape method on every params before adding them, and then provide a global option to apply that method on every params to ease the escaping.

Let me know of what you think of this, and if I get some time, I'll try to implement it...
Also, if you have some idea of what to escape, feel free to let me know

[hiddentao]

Hey all, sorry about the delay in reply but I thought squel already provided a solution for such special cases via stringFormatter:

console.log(squel.insert({ stringFormatter: s => `"${s}"` }).into('buh').setFields({foo: "bar'baz"}).toString());

INSERT INTO buh (foo) VALUES ("bar'baz")

Does this not work anymore?

Documented here -> https://hiddentao.com/squel/#custom_formatting

You should also be able to set it globally via squel.cls.DefaultQueryBuilderOptions (see https://hiddentao.com/squel/api.html#cls_defaultquerybuilderoptions) to ensure it applies to all your squel builders.

(On a side note, I know that the documentation needs improving)

[fredck]

@hiddentao thanks for the reply but (1) this is far from a "special case" - it's one of the most basic SQL injection security issues and (2) that "solution", other than overcomplicated, hardly solves the problem as it switches the issue from ' to ", simply.

[hiddentao]

@fredck Yeah I get your point. The guiding philosophy at the time was that parameterized queries were the best way around this. The nice thing about the custom formatter override is that you can make it as simple or as complicated as you wish - what I presented above is just a simple example. It's possible that squel could come with a built-in intelligent formatter, though I'm guessing different db engines may need slighly different rules with respect to this. Right now, I'm open to a PR which accomplishes this.

[seanlynch]

I don't use this package anymore, and I don't seem to be able to keep this issue out of my activity feed. Please feel free to open a separate issue if you still care.

[fredck]

@seanlynch, I'm sorry to say that, but this seems to be your problem and closing the issue is far from being a way to solve it. If unsubscribing to the issue doesn't help, you may setup a message filter at your side or whatever. This is an open source space and your move seems inappropriate.

I would ask either you or anyone else to reopen the issue until it gets solved. Thanks!

[seanlynch]

I guess that'll teach me to report security problems in packages I don't use!

[seanlynch]

If you care that much, @fredck , open another issue.

[hiddentao]

I'd like to keep this issue open.

[yibn2008]

Before this PR #367 being accepted and new version released, you can use https://github.com/yibn2008/safe-squel instead. I've added escape support by default (mysql will use backslash to escape)

[LandonSchropp]

@hiddentao Is there any update on this issue? My team got a GitHub security vulnerability alert about this. Our app isn't currently vulnerable because we're using toParam, but the response to this makes us nervous. Having a SQL vulnerability issue open for over a year in a SQL query building library doesn't inspire confidence.

Are you open to following up on #367 and fixing this? Should we be concerned about other potential security vulnerabilities?

[hiddentao]

@LandonSchropp Unfortunately I have very little time to work on Squel at the moment so issues that require deeper consideration or a larger amount of work tend to be de-prioritised ahead of easier fixes. On the other hand I'm happy to accept a pull request that fixes this.

Perhaps I can update the documentation (and maybe even output a console warning) the first time toString() is called encouraging one to use toParam() instead.

[LandonSchropp]

@hiddentao That's understandable, and I appreciate the quick response. I think a console warning would be a helpful callout.