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

[Improvement] Possible SQL injection in MysqlDatabaseOperations.java #4211

Open
justinmclean opened this issue Jul 19, 2024 · 5 comments · May be fixed by #4222
Open

[Improvement] Possible SQL injection in MysqlDatabaseOperations.java #4211

justinmclean opened this issue Jul 19, 2024 · 5 comments · May be fixed by #4222
Labels
good first issue Good for newcomers improvement Improvements on everything

Comments

@justinmclean
Copy link
Member

What would you like to be improved?

databaseName in generateDropDatabaseSql is not validated for any potential SQL issues.

How should we improve?

Validate databaseName

@justinmclean justinmclean added good first issue Good for newcomers improvement Improvements on everything labels Jul 19, 2024
@ria28
Copy link

ria28 commented Jul 20, 2024

I would like to work on this issue.
To validate, do we just want to check that if databaseName is null or empty, then throw an exception?

ria28 added a commit to ria28/gravitino that referenced this issue Jul 20, 2024
… preventing possible SQL injection in MysqlDatabaseOperations.java
@zivali
Copy link
Contributor

zivali commented Jul 21, 2024

@justinmclean Hi, I think before databaseName is passed into generateDropDatabaseSql, we already validate it with the capability framework (PR #2335). There are some tests that cover this validation in CatalogMysqlIT. Do we need to validate again in generateDropDatabaseSql?

@justinmclean
Copy link
Member Author

We would need to more check more than if it null or empty. I'm not 100% sure that the current check in the capability formwork is enough as it tests for what might be a valid name, rather than malicious SQL. I can see delete calls dropDatabase which calls generateDropDatabaseSql and there doesn't seem to be any check on the name in that code path that I can see.

@justinmclean
Copy link
Member Author

@zivali what are you thoughts on this?

@zivali
Copy link
Contributor

zivali commented Jul 25, 2024

Per my understanding, JdbcDatabaseOperations.delete currently will only be called by the dropSchema. During the dropSchema API call, the data will be routed to SchemaNormalizeDispatcher, in which we apply the capability formwork with CapabilityHelpers.applyCapabilities. This uses a whitelist approach to validate names. Tests cover some malicious SQL can be found in CatalogMysqlIT

I think if we assume JdbcDatabaseOperations.delete will be called by any other class in the future and won't be routed through SchemaNormalizeDispatcher, adding more validation here in generateDropDatabaseSql won't hurt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers improvement Improvements on everything
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants