[#2179] Improvement: Improve security when creating and dropping schemas and tables #2335

zivali · 2024-02-24T16:32:34Z

What changes were proposed in this pull request?

Improve security when creating and dropping schemas and tables.

This PR adds the following checks for identifier names using the capability framework

Regex check
- As a best practice, it's generally advised to avoid including spaces in database names. In this PR, database names that include space will be considered illegal.
String length check, since SQL injection usually requires using longer string
- Mysql: at most 64 characters
- Postgresql: at most 63 characters

We refer to specifications of the earliest version of DB that gravitino currently supports:

Postgresql identifier rules: https://www.postgresql.org/docs/12/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS
Mysql identifier naming: https://dev.mysql.com/doc/refman/5.7/en/identifiers.html
Mysql identifier length limit: https://dev.mysql.com/doc/refman/5.7/en/identifier-length.html

Why are the changes needed?

Fix: #2179

Does this PR introduce any user-facing change?

Add name identifier checks before attempting to create or drop schemas and tables.

How was this patch tested?

Add IT tests.

.../src/test/java/com/datastrato/gravitino/catalog/jdbc/operation/SqliteDatabaseOperations.java

.../src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlDatabaseOperations.java

...n/java/com/datastrato/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java

...trato/gravitino/integration/test/catalog/jdbc/postgresql/TestPostgreSqlSchemaOperations.java

yuqi1129 · 2024-02-26T12:36:39Z

@zivali
Can you also survey the table naming conventions besides the database name?

...on/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcDatabaseOperations.java

...n/java/com/datastrato/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java

.../src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlDatabaseOperations.java

zivali · 2024-02-27T15:49:19Z

@zivali Can you also survey the table naming conventions besides the database name?

Absolutely! Do you mean we should apply similar checks in JdbcTableOperations (MysqlTableOperations / SqliteTableOperations / PostgreSqlTableOperations)?

p.s. I modified the PR per comments. Thanks for help reviewing!

simarjeetss · 2024-03-03T05:11:15Z

hey @justinmclean, is this improvement done? If not, may I look into this?

justinmclean · 2024-03-06T00:02:15Z

@simarjeetss IThanks for offering to help, but as this is almost complete, I woudl suggest you find another issue to work on.

simarjeetss · 2024-03-07T06:17:45Z

@simarjeetss IThanks for offering to help, but as this is almost complete, I woudl suggest you find another issue to work on.

Sure, thanks for letting me know!

yuqi1129 · 2024-03-07T07:43:02Z

@jerryshao
Is this related to the issue #1652? If so, should we temporarily merge it?

justinmclean · 2024-03-14T01:01:01Z

I would say it is not related to #1652. Might consider merging this as it is an overall improvement to the existing code?

justinmclean · 2024-04-12T06:20:56Z

@zivali Are you still interested in working on this? No issue if you can't, it would just be good to know.

zivali · 2024-04-14T12:39:04Z

@justinmclean Hi, I am still working on it. Will update here when finished. Thank you!

mchades · 2024-04-14T14:17:22Z

I'm introducing a catalog capability framework(#2819), I think this should be one of the catalog capabilities - name specification capability.

If you want, I think you can continue to work based on #2819 or after it is merged

zivali · 2024-04-15T01:46:52Z

@mchades Got it. Will integrate this PR to #2819 Thanks for the info!

mchades · 2024-04-15T09:53:01Z

.../src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlDatabaseOperations.java

+    // dollar signs
+    // \w matches [a-zA-Z0-9_]
+    // \p{L} matches any kind of letter from any language
+    if (!databaseName.matches("^[\\w\\p{L}$]*$")) {


Is this a rule from experience? Or is it an official MySQL constraint?

Oh, I just saw the description in your PR, it's a limitation of MySQL official.

Yes, this is the most basic permitted characters in unquoted identifiers for MySQL. I think most DB users uses only these characters. But other characters can also be used with quoted identifiers. Do you think we should include those characters as well?

I think we can start with stricter restrictions and gradually relax them as needed.

mchades · 2024-04-29T14:10:00Z

The name specification capability of catalog is now available, you can rebase the main branch and proceed with the PR

zivali · 2024-06-17T13:47:12Z

@mchades @justinmclean Could you help review again when you have time? Thank you!

mchades · 2024-06-18T10:08:12Z

core/src/main/java/com/datastrato/gravitino/catalog/TableNormalizeDispatcher.java

-    // The constraints of the name spec may be more strict than underlying catalog,
-    // and for compatibility reasons, we only apply case-sensitive capabilities here.
-    return dispatcher.dropTable(applyCaseSensitive(ident, Capability.Scope.TABLE, dispatcher));
+    return dispatcher.dropTable(normalizeNameIdentifier(ident));


after these changes, can I drop a Mysql table named a&b by Gravitino?

No, as the current regex pattern in this PR does not try to match the character &.
Should we add & to the regex pattern?

No, this is just my assumption.

We can gradually relax the strategy after encountering real needs, or make this rule configurable when the demand is frequent.

mchades

LGTM, thanks for your contributions!

…g schemas and tables (apache#2335) ### What changes were proposed in this pull request? Improve security when creating and dropping schemas and tables. This PR adds the following checks for identifier names using the capability framework - Regex check - As a best practice, it's generally advised to avoid including spaces in database names. In this PR, database names that include space will be considered illegal. - String length check, since SQL injection usually requires using longer string - Mysql: at most 64 characters - Postgresql: at most 63 characters We refer to specifications of the earliest version of DB that gravitino currently supports: - Postgresql identifier rules: https://www.postgresql.org/docs/12/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS - Mysql identifier naming: https://dev.mysql.com/doc/refman/5.7/en/identifiers.html - Mysql identifier length limit: https://dev.mysql.com/doc/refman/5.7/en/identifier-length.html ### Why are the changes needed? Fix: apache#2179 ### Does this PR introduce _any_ user-facing change? Add name identifier checks before attempting to create or drop schemas and tables. ### How was this patch tested? Add IT tests.

justinmclean reviewed Feb 25, 2024

View reviewed changes

.../src/test/java/com/datastrato/gravitino/catalog/jdbc/operation/SqliteDatabaseOperations.java Outdated Show resolved Hide resolved

justinmclean reviewed Feb 25, 2024

View reviewed changes

.../src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlDatabaseOperations.java Outdated Show resolved Hide resolved

justinmclean reviewed Feb 25, 2024

View reviewed changes

...n/java/com/datastrato/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java Outdated Show resolved Hide resolved

justinmclean reviewed Feb 25, 2024

View reviewed changes

...trato/gravitino/integration/test/catalog/jdbc/postgresql/TestPostgreSqlSchemaOperations.java Outdated Show resolved Hide resolved

yuqi1129 reviewed Feb 26, 2024

View reviewed changes

zivali force-pushed the dev branch from 6cf0356 to 9424435 Compare February 27, 2024 16:24

zivali force-pushed the dev branch from 9424435 to 0220868 Compare March 15, 2024 13:20

mchades reviewed Apr 15, 2024

View reviewed changes

mchades assigned zivali Apr 24, 2024

zivali force-pushed the dev branch from d89dd90 to c1b1d1d Compare June 4, 2024 15:43

zivali marked this pull request as draft June 4, 2024 15:44

zivali force-pushed the dev branch from c1b1d1d to 4050322 Compare June 4, 2024 15:45

zivali force-pushed the dev branch 2 times, most recently from 635678b to 488b8e2 Compare June 15, 2024 07:13

zivali changed the title ~~[#2179] Improvement: Potential SQL injection point in generateDropDat…~~ [#2179] Improvement: Improve security when creating and dropping schemas and tables Jun 15, 2024

zivali closed this Jun 15, 2024

zivali reopened this Jun 15, 2024

zivali closed this Jun 16, 2024

zivali reopened this Jun 16, 2024

zivali closed this Jun 16, 2024

zivali reopened this Jun 16, 2024

zivali force-pushed the dev branch from 488b8e2 to f265e55 Compare June 17, 2024 11:13

zivali marked this pull request as ready for review June 17, 2024 11:13

mchades self-requested a review June 17, 2024 14:24

mchades reviewed Jun 18, 2024

View reviewed changes

zivali added 2 commits June 18, 2024 19:15

Improvement: Add jdbc capabilities for creating and dropping schema

b9c7095

Improvement: Add jdbc capabilities for creating and dropping table

3e59aff

mchades force-pushed the dev branch from f265e55 to 3e59aff Compare June 18, 2024 11:15

mchades approved these changes Jun 18, 2024

View reviewed changes

mchades merged commit 8e7293e into apache:main Jun 18, 2024
33 checks passed

zivali mentioned this pull request Jul 21, 2024

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2179] Improvement: Improve security when creating and dropping schemas and tables #2335

[#2179] Improvement: Improve security when creating and dropping schemas and tables #2335

zivali commented Feb 24, 2024 •

edited

Loading

yuqi1129 commented Feb 26, 2024

zivali commented Feb 27, 2024 •

edited

Loading

simarjeetss commented Mar 3, 2024

justinmclean commented Mar 6, 2024

simarjeetss commented Mar 7, 2024

yuqi1129 commented Mar 7, 2024

justinmclean commented Mar 14, 2024

justinmclean commented Apr 12, 2024

zivali commented Apr 14, 2024

mchades commented Apr 14, 2024

zivali commented Apr 15, 2024

mchades Apr 15, 2024

mchades Apr 15, 2024

zivali Apr 15, 2024

mchades Apr 24, 2024

mchades commented Apr 29, 2024

zivali commented Jun 17, 2024

mchades Jun 18, 2024

zivali Jun 18, 2024

mchades Jun 18, 2024

mchades left a comment

[#2179] Improvement: Improve security when creating and dropping schemas and tables #2335

[#2179] Improvement: Improve security when creating and dropping schemas and tables #2335

Conversation

zivali commented Feb 24, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

yuqi1129 commented Feb 26, 2024

zivali commented Feb 27, 2024 • edited Loading

simarjeetss commented Mar 3, 2024

justinmclean commented Mar 6, 2024

simarjeetss commented Mar 7, 2024

yuqi1129 commented Mar 7, 2024

justinmclean commented Mar 14, 2024

justinmclean commented Apr 12, 2024

zivali commented Apr 14, 2024

mchades commented Apr 14, 2024

zivali commented Apr 15, 2024

mchades Apr 15, 2024

Choose a reason for hiding this comment

mchades Apr 15, 2024

Choose a reason for hiding this comment

zivali Apr 15, 2024

Choose a reason for hiding this comment

mchades Apr 24, 2024

Choose a reason for hiding this comment

mchades commented Apr 29, 2024

zivali commented Jun 17, 2024

mchades Jun 18, 2024

Choose a reason for hiding this comment

zivali Jun 18, 2024

Choose a reason for hiding this comment

mchades Jun 18, 2024

Choose a reason for hiding this comment

mchades left a comment

Choose a reason for hiding this comment

zivali commented Feb 24, 2024 •

edited

Loading

zivali commented Feb 27, 2024 •

edited

Loading