A critical problem with the database, and also the state of this plugin

[A248]

A long time ago marcelo-mason wrote the original PreciousStones. It came with some degree of durability, and in its lifetime after the original author left, has been helped along by various maintainers and contributors, all in their spare time working for the benefit of users to enjoy a humble, usable protection plugin.

Unfortunately, in the present time, the situation is less optimistic. PreciousStones's database handling is in dire need of a major code overhaul. As it stands, the StorageManager is frankly a mess. The code is vulnerable to SQL injections, a major problem for a plugin whose interface is almost entirely user-facing.

It is not clear exactly how to solve this. There is no easy solution or quick fix. The StorageManager and DBCore implementations are so convoluted that it is very hard for a newcomer to understand what is going on.

This problem is compounded by another reality. The truth is, @xtomyserrax , it does not seem to me that you understand how the plugin's database works. Don't worry, I don't understand PreciousStones myself! It is a complex plugin. The issue though, is that this means I have no idea who is, or might become one who is, someone acquainted with the plugin, understanding its features, knowledgeable in SQL and Java, and willing to sink several hours of effort into fixing problems plaguing PreciousStones's backend.

If @NathanWolf may be more equipped to take this task on, this would be the perfect time for Nathan to speak up.

Otherwise, if you think you know enough of PreciousStones's other features, but do not know enough about SQL to rework the database, I may be of some help in this regard by lending knowledge. However, you would still need to learn the basics of SQL and JDBC.

Finally, I would like to note. Protection against SQL injection is not an enhancement, but a bug and a gaping security hazard. As far as I can tell PreciousStones does not sanitise any of its inputs. This is no small issue, and if you are doubtful, I encourage you to research some of the consequences of SQL injection.

[xtomyserrax]

I couldnt agree more with the things you say. Sadly, PS was left by the original author so as far as I know, Nathan did what he could to just keep the plugin working. In my case, Im helping with some commits to fix some issues and keep compability with newer minecraft versions. I think I was more than clear that I have none knowledge about databases (Ill probably learn about them this quarter) and well also Im not familiar with a lot of things here, so I just do what Im able to and fix some stuff so the server I play gets a better version of PS.

So well, I talked with Nathan in the past but I dont think the has the time to work on big things for PS. I guess he will answer if he is able to. In my point of view you are more than welcome to help us, moreover because you are a experienced and nice guy. Thanks!

[NathanWolf]

I agree with everything said here, including that I do not have enough time myself to really fix this plugin.

To be totally honest, if I were to invest that much time it would probably be to create my own replacement plugin, rather than fix this one. I really only care about the protection parts, and I think I could probably re-write that part from scratch, including the ability to import fields as protected regions from an existing PS db.

That said, my personal (as a server owner) solution to this problem is that I give no command permissions to players. That is a general rule for me, really.

I know that doesn't work for everyone though, and much of PS (such as allowing other players in a field) requires player-facing commands.

So... yeah I guess all I can offer is to take a look at the db layer at some point when I find some time. It seems like it shouldn't be tremendously hard to shift towards using prepared statements, at least for any operations with query parameters that could originate as player input.

[NathanWolf]

Ok so looking through the code a bit, while it is a bit ugly I only see a few places where there is a real risk of injection.

For my purposes, allowing players is the only thing I think I let players do, and fortunately that input is semi-validated, in that it must be a real player name that is used. (We will ignore the possibility of little Johnny Drop-Tables having an MC account).

Other very bad (potentially) commands that I personally do not let players use would be renaming a field, and I think changing welcome/leave messages, though I can't find exactly where in the code that happens.

Probably the best fix here would be to change the underlying layer to use prepared statements and require all parameters to be passed in as such, rather than pre-joined into a SQL string. This would be quite a bit of work, though- mainly it's a lot of busywork updating all of the calls to update() and insert() though.

[A248]

PreciousStones does use Helper.escapeQuotes inside the StorageManager#updateField method. However, I'm sure that isn't sufficient to stop a malicious, determined, and knowledgeable user. There's this SO question on it: https://stackoverflow.com/questions/139199/can-i-protect-against-sql-injection-by-escaping-single-quote-and-surrounding-use

Probably the best fix here would be to change the underlying layer to use prepared statements and require all parameters to be passed in as such, rather than pre-joined into a SQL string.

Definitely. The issue is that there is so much messy code in StorageManager, of which a large portion must be changed to use PreparedStatements, and the rest, if not changed, understood, since fixing bugs that may arise is tough when you don't understand the bigger picture.

[NathanWolf]

Yeah IMO the first rule of SQL safety is never try to sanitize inputs yourself. There are just too many edge-cases, and we hope the drivers handle it all properly. Unfortunately it's a lot harder to re-write all the code to use prepared statements then to just try to escape everything :(