Skip to content
This repository has been archived by the owner on Nov 9, 2021. It is now read-only.

Not properly escaping quotes in setFields #350

Open
seanlynch opened this issue Mar 21, 2018 · 22 comments
Open

Not properly escaping quotes in setFields #350

seanlynch opened this issue Mar 21, 2018 · 22 comments

Comments

@seanlynch
Copy link

seanlynch commented Mar 21, 2018

> 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 seanlynch changed the title Not properly quoting strings in setFields Not properly escaping quotes in setFields Mar 21, 2018
@seanlynch
Copy link
Author

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
Copy link

fredck commented Apr 9, 2018

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
Copy link

Nox-404 commented May 14, 2018

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
Copy link

Nox-404 commented May 14, 2018

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
Copy link
Author

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
Copy link

Nostradamos commented May 15, 2018

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
Copy link
Author

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
Copy link

Arilas commented Jun 1, 2018

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
Copy link

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

@Nox-404
Copy link

Nox-404 commented Jul 9, 2018

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
Copy link
Owner

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
Copy link

fredck commented Jul 16, 2018

@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
Copy link
Owner

@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
Copy link
Author

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
Copy link

fredck commented Jul 31, 2018

@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
Copy link
Author

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

@seanlynch
Copy link
Author

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

@hiddentao
Copy link
Owner

I'd like to keep this issue open.

@hiddentao hiddentao reopened this Aug 2, 2018
@yibn2008
Copy link

yibn2008 commented Sep 20, 2018

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
Copy link

@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?

Screen Shot 2019-06-18 at 1 08 07 PM

@hiddentao
Copy link
Owner

@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
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants