[Dev] SQL - Security: use prepared statements and validate SQL settings

[ljacqu]

Maybe a story for @DNx5?

1. Ensure that we only use prepared statements for all SQL statements we run with parameters.

2) Validate the settings for the column and table names (e.g. mySQLColumnId) to be also sure that they won't break the SQL statements (probably a regexp such as [a-zA-Z0-9_-]+ or similar) Skipped for this issue

3. Once this is done, we can remove the checks for SQL keywords in the password. For instance, this is done in ChangePasswordCommand and will no longer be necessary:

if (playerPassLowerCase.contains("delete") || playerPassLowerCase.contains("where")
            || playerPassLowerCase.contains("insert") || playerPassLowerCase.contains("modify")
            || playerPassLowerCase.contains("from") || playerPassLowerCase.contains("select")
            || playerPassLowerCase.contains(";") || playerPassLowerCase.contains("null")

[sgdc3]

👍

[DNx5]

I'm only use preparedStatement if the input type is String.
we can remove that sql keywords check for mysql, but idk for other datasources. I will do it if I have time.

[sgdc3]

@ljacqu bump

[ljacqu]

sgdc3 commented @ljacqu bump
sgdc3 added this to the 5.2 Release milestone

I don't have time to do this personally within the release of 5.2, but if there's any taker for it, it would certainly be great to have it done in 5.2. I think 1) has already been done by DNx5, 2) is fairly short (just gotta find a good, central place to do it!) and then 3) is basically just some searching and deleting. I've even annotated a lot of places with a "TODO #308" for the third point

[DNx5]

i think there is no need to validate the database configuration, because it will fail to connect if something wrong. people should put the right value there.

[ljacqu]

It's more about preventing some sort of SQL injection. I know it's a long shot since the names come from the config file, i.e. a file only an admin has access to, but it wouldn't hurt.

[DNx5]

I'm too lazy to think about that, because it is not only for MySQL 😆

[ljacqu]

OK 😆
Is 3) possible to be done?

[DNx5]

we can remove it, if 1) is done.

[sgdc3]

5.3 beta

[ljacqu]

@Xephi "awaiting answer"?

[Xephi]

I would put please-verify :') but i've finish my check, also i think it's ok now

If you're ok by the way i've fixed 1), we can close this

[ljacqu]

1. looks good; nice finds :)
2. was decided by DNx5 that we skip (for this issue anyway; I will create another)
3. is still open

[ljacqu]

Reopened – there are three other places annotated with TODO #308 and there are probably more places where this check is present.

[Xephi]

Reclose - Sorry :')

[ljacqu]

Looks good – thanks! I'm very glad that these checks are no longer part of the codebase. :)