Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SQL injection vulnerability #19

Merged
merged 2 commits into from Nov 29, 2017

Conversation

Projects
None yet
3 participants
@c-w
Copy link
Member

commented Nov 29, 2017

The visits endpoint doesn't do any parameter escaping. This pull request fixes that.

@c-w c-w requested review from anthturner and timfpark Nov 29, 2017

@timfpark

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

LGTM - nice catch.

@c-w c-w merged commit b632d9c into master Nov 29, 2017

@c-w c-w deleted the fix-sql-injection branch Nov 29, 2017

@anthturner

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

LGTM as well, for what it's worth. Just to mirror my feedback here so it's tracked for the future, while this fix will work in our environment as-deployed, be cautious with the "replace all single-quotes with two single-quotes" strategy to prevent injections. Tricks like double-encoding or the use of equivalent UTF-8 characters can cause localization-related headaches on some configurations that may accidentally re-open this hole, and really this mostly just works because Postgres is very specific about its syntax. Similar tricks would be easily bypassed in MS-SQL or MySQL (without ANSI_QUOTES) with backslashes or comment injection.

More information available from OWASP here: https://www.owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet

@c-w

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

Yeah, I agree. We should really let the database connector handle this instead of doing string interpolation to build up the queries. But that's more work than I have capacity to do right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.