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

*: replace %s formatting for SQL statements with lexbase.EncodeSQLString throughout the codebase #69428

Open
Azhng opened this issue Aug 26, 2021 · 4 comments
Labels
A-security C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. S-3 Medium-low impact: incurs increased costs for some users (incl lower avail, recoverable bad data) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-sql-queries SQL Queries Team

Comments

@Azhng
Copy link
Contributor

Azhng commented Aug 26, 2021

Currently, we have various places [1] where we use fmt.Sprintf() with %s to format SQL statements. This is prone to SQL injections bugs. Instead we should be using lexbase.EncodeSQLString to properly escape the statements.

edit (knz): note that tree.Name has a String() method that already does the right thing, so it's OK to do %s with a tree.Name. With string, not so much.


[1]: for instance (not comprehensive):

Jira issue: CRDB-9593

@Azhng Azhng added the C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. label Aug 26, 2021
@knz knz added the A-security label Aug 26, 2021
@knz
Copy link
Contributor

knz commented Aug 26, 2021

@Azhng you're missing a hyperlink I think? Can you add it?

cc @catj-cockroach - can you put this on the seceng radar?

@knz knz changed the title *: replace `%s% formatting for SQL statements with lexbase.EncodeSQLString throughout the codebase *: replace %s formatting for SQL statements with lexbase.EncodeSQLString throughout the codebase Aug 26, 2021
@knz
Copy link
Contributor

knz commented Aug 26, 2021

NB: @petermattis has implemented a framework for CC managed-service that helps fix this. Peter, can you point the participants here to the code you've authored? thx

@petermattis
Copy link
Collaborator

https://github.com/cockroachlabs/managed-service/tree/master/pkg/safesql

@knz knz added this to Triage in SQL Queries via automation Aug 26, 2021
@knz knz added this to Triage in SQL Sessions - Deprecated via automation Aug 26, 2021
@blathers-crl blathers-crl bot added T-sql-queries SQL Queries Team T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Aug 26, 2021
@yuzefovich yuzefovich moved this from Triage to Backlog in SQL Queries Aug 31, 2021
@rafiss rafiss moved this from Triage to Shorter term backlog in SQL Sessions - Deprecated Sep 23, 2021
@RichardJCai
Copy link
Contributor

Ideally we could have a linter for this but that's pretty hard to do.

Another idea is to update our randomized testing framework to try doing SQL injection attacks on show commands and see if parse ever fails with the error that we're trying to parse multiple statements and our injection attack can contain crdb_internal.panic()

@exalate-issue-sync exalate-issue-sync bot removed the T-sql-queries SQL Queries Team label Jun 1, 2023
@mgartner mgartner moved this from Backlog (DO NOT ADD NEW ISSUES) to Bugs to Fix in SQL Queries Jun 22, 2023
@mgartner mgartner added the S-3 Medium-low impact: incurs increased costs for some users (incl lower avail, recoverable bad data) label Jun 22, 2023
@yuzefovich yuzefovich added the T-sql-queries SQL Queries Team label Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-security C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. S-3 Medium-low impact: incurs increased costs for some users (incl lower avail, recoverable bad data) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-sql-queries SQL Queries Team
Projects
Status: Bugs to Fix
SQL Queries
Bugs to Fix
SQL Sessions - Deprecated
Shorter term backlog
Development

No branches or pull requests

6 participants