cockroachdb / cockroach

CockroachDB - the open source, cloud-native distributed SQL database.
https://www.cockroachlabs.com
Other
29.5k stars 3.7k forks source link

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

Open Azhng opened 2 years ago

Azhng commented 2 years ago

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

knz commented 2 years ago

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

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

knz commented 2 years ago

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 commented 2 years ago

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

RichardJCai commented 2 years ago

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()