[Improvement] Potential SQL injection point in generateDropDatabaseSql

[justinmclean]

What would you like to be improved?

The database name is passed into generateDropDatabaseSql and is passed on to executeQuery without checking or validating it's contents.

How should we improve?

Verify the database name before calling executeQuery.

[henrybear327]

Hello @justinmclean,

I would like to work on this issue, but may I request more hints on how to go about fixing this issue?

Thank you!

[justinmclean]

These links may help:
https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/07-Input_Validation_Testing/05-Testing_for_SQL_Injection
https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html

In this case, I would check if the name is in an expected format with a regular expression or similar.

[mchades]

Has the issue been resolved after merging #3053?

[zivali]

I saw #3053 which originally seems to fix this issue is causing bug report for listing MySQL schema '//' in #3101. So currently we do not applyCapabilitiesOnName when dropping schema #3126.
But '/' is not a classic character in MySQL identifier.

Should we still verify the naming before dropping using the capability framework?
If so, there may be other bug reports which request supports for special characters in the future.

[mchades]

But '/' is not a classic character in MySQL identifier.

Although / is not a classic character in MySQL, there are indeed real user scenarios that require it to be included in the database name, so we need to allow users to operate it.

Should we still verify the naming before dropping using the capability framework?
If so, there may be other bug reports which request supports for special characters in the future.

Is there any other way to prevent potential SQL injection in generateDropDatabaseSql other than checking the name? (such as prepared-statements-with-parameterized-queries?)

If not, I think we can verify the naming before dropping using the capability framework firstly to resolve this issue. If there are other users who need to use special characters in real scenarios, we will gradually relax restrictions or make the name spec capability configurable for users. WDYT?

[zivali]

Prepared statement seems to be designed to handle data values, not for SQL identifiers like table names or database names.
So when creating and deleting table/database, most JDBC doesn't support using prepared statement e.g. MySQL
I think we may still need to validate the naming. Does Gravitino collect user logs so that we may know what are some special characters that are currently in use by users?

[mchades]

This regular expression already contains the special characters that are known to be used by the user
https://github.com/datastrato/gravitino/blob/a09420dd90a413a4f4944fc628286d20f9950d1c/core/src/main/java/com/datastrato/gravitino/connector/capability/Capability.java#L87-L97

[zivali]

Got it, thanks! I think we can add separate regex name pattern for the 4 JDBC catalog and validate naming when creating and dropping table/database for now. What do you think? :)