search

foin137 / werwolfonline.eu

www.werwolfonline.eu
http://www.werwolfonline.eu/info
GNU General Public License v3.0
20 stars 11 forks source link

Vulnerability for sql injection #4

Closed TobiasXy closed 4 years ago

TobiasXy commented 4 years ago

Edit: Originally wrote this text in german, but it's probably better to keep the issues in english.

Hey, since we want to try out your project later and I noticed you open sourced it, I wanted to have a quick look at the code.

What I noticed: The database queries are not protected against sql injection, which could get as bad as people dropping your database tables or deleting/changing/viewing records within the database. I just wanted to note that down here, but if I get time in the next days I could fix it myself too.

foin137 commented 4 years ago

Thank you very much for your comment!

This is indeed a problem that should be fixed soon. Are you already working on it? If not, I will try and fix it myself in the next days.

TobiasXy commented 4 years ago

No, I don't work on it yet. Can't promise anything, but I think I could get to it next week some time. But if you do it that's fine too obviously.

foin137 commented 4 years ago

I tried to fix it using prepared sql statements for string inputs and (int) conversions for integer inputs. It would probably be better to have all queries with prepared statements, but this should also work (as far as I know)

It would be helpful if you could comment on that, do you think that solves the issue?

TobiasXy commented 4 years ago

Yes, prepared statements are not vulnerable to sql injection, as long as all the parameters are passed to bind_param and are not inserted into the string directly (as it was before).

For the other queries where you have a variable table name, it is not really possible to use prepared statements unfortunately. Casting the values to int beforehand also makes sure sql injection doesn't work I think, but it is much harder to track if you actually casted the value everywhere it is used. Having separate tables (and therefore variable table names) is pretty unusual too, but that is a separate point.

foin137 commented 4 years ago

Ok thank you!

There are probably a few other things as well I would now do differently or that could be done in a better way.

But anyway, this issue can then be closed .