Security problems

[VanNostrand]

.config_commonfunctions is vulnerable to SQL injections: All functions with SQL statements are problematic. This means, the database can easily be manipulated/read/vandalized by unauthorized people and the authentication.php allows any blackhat to login as admin this way.

One code example can be found in add_user: mysql_query("INSERT INTO " [...] VALUES ('" . trim($username) . "','" . trim($encryptedpassword) . "',1,'" . $registrationdate . "')");

Never insert data from html forms or the GET/POST environment in general into SQL statements. See http://bobby-tables.com/ and http://php.net/manual/en/security.database.sql-injection.php for more information.

Also, as the user inputs are not checked or escaped, other security problems can be possible, like executing commands etc.

Note: mysql_query will be deprecated in php 5.5.0, so it can be changed to MySQLi or PDO_MYSQL (they use prepared statements with bound variables) while fixing the security problem :-)

[woogerboy21]

So your suggesting is that I capture the input and check the input before adding the captured variable into the SQL statement?

Also I knew about the mysqli.

Thanks, Woodrow Collins

On May 18, 2013, at 4:48 AM, "VanNostrand" notifications@github.com wrote:

.config_commonfunctions is vulnerable to SQL injections: All functions with SQL statements are problematic. This means, the database can easily be manipulated/read/vandalized by unauthorized people and the authentication.php allows any blackhat to login as admin this way.

One code example can be found in add_user: mysql_query("INSERT INTO " [...] VALUES ('" . trim($username) . "','" . trim($encryptedpassword) . "',1,'" . $registrationdate . "')");

Never insert data from html forms or the GET/POST environment in general into SQL statements. See http://bobby-tables.com/ and http://php.net/manual/en/security.database.sql-injection.php for more information.

Also, as the user inputs are not checked or escaped, other security problems can be possible, like executing commands etc.

Note: mysql_query will be deprecated in php 5.5.0, so it can be changed to MySQLi or PDO_MYSQL while fixing the security problem :-)

— Reply to this email directly or view it on GitHub.

[woogerboy21]

Hey, by chance is the mysqli require checking of input or have they worked that security functionality in? I can't say I have looked into using it yet at all other than I knew the mysql_query was decpecated.

[VanNostrand]

yes, the input must not contain unescaped characters, eg. the following username inside " " should be forbidden: "'; select * from users; -- " This would be an sql injection that selects everything from a table, it could be any sql command and php then creates something like this: INSERT INTO [...] VALUES ''; select * from users; -- ','pw',1,'date') aka rubbish, then the Blackhat's command, then an SQL comment dealing with the rest of the line.

But: do not escape those characters manually and do not use php's escape_string() functions. Instead, using prepare+bind+execute statements should solve the problem good enough: http://www.php.net/manual/en/mysqli.quickstart.prepared-statements.php For this, the php variables in the SQL commands have to be replaced by the string "(?)", then the bind_param() escapes and binds the php variables to this statement and execute then executes it. This should be safe, though it does not hurt to check most variables if they have the correct datatype first (e.g. with ctype_alnum(), which checks if the variable contains letters or digits only; if it contains special characters it should return false; or whatever you need from http://www.php.net/manual/en/ref.ctype.php). For passwords, special characters should be allowed of course, but as you insert hashes (or better: blowfish encrypted strings) into the password field anyway, you could hash/encrypt the password variable first and then you can insert the hash without harm).

I saw a seminar video about SQL injections and the conclusion was: this is one of the oldest security problems in IT and no library/framework/language is able to deal with it properly, but they try - and everything is better than creating SQL statements with concatenated variables :-D

So after this I guess the code can be considered safe enough for most cases. I don't know the details of php and it's database interfaces, the documentation should contain all needed information.

[woogerboy21]

Since I can't seem to find out how to contact you outside of github on here. Shoot me an email when you have a moment.

[VanNostrand]

we wrote about the cockatrice release, you already have my mailaddress (mark) :)

[woogerboy21]

Updated the few functions un-moderated users have access to in order to prevent random people from just trying to inject code. I will still have to go back and update the remaining functions when I update to the mysqli functionality. It's dirty but should work for now.

[VanNostrand]

using mysql_real_escape_string is not safe btw, you need to use variable binding.

[woogerboy21]

Until the code is changed over to mysqli mysql_escape_string is as good as it gets.

Welcome to why people moved to using mysqli :)

On May 18, 2013, at 6:39 PM, "VanNostrand" notifications@github.com wrote:

using mysql_real_escape_string is not safe btw, you need to use variable binding.

— Reply to this email directly or view it on GitHub.

[woogerboy21]

And correct me if I am wrong but the only advantage I see to variable bindings is the type checks. I'm also not convinced doing things like preparing MySQL statements on such a small application will improve performance at all. Maybe in one area possibly but since the query's are not repetitively used before sessions are closed and reopened all it is doing is causing overhead.

[VanNostrand]

mysql_escape_string can be circumvented by sending a hexadecimal or otherwise encoded string instead of clear text sql fragments. There is nothing to escape, but the database interprets and executes it. Example: http://ferruh.mavituna.com/sql-injection-cheatsheet-oku/#UsingIntegers Parameterized statements ensure that a (?) placeholder can only store a value of the given type and not an arbitrary SQL fragment. If the php variable contained an SQL injection then it would simply be treated as a strange (and probably invalid) parameter value. And if it is hex, then it is stored as hex. So binding escapes the data and enforces that the content is treated as one value. There are crazy things that can be done, the cheat sheet is a very nice overview.

To improve the performance, you create a database transaction after establishing the connection: BEGIN WORK; sql stuff here COMMIT WORK; Then all commands are executed in one rush, otherwise the database creates a transaction for every single command, which can easily create a huge overhead. This is important for inserting large quantities of data, e.g. when you import data. I had a database once where the initialisation with data took one hour and after creating a single transaction it finished in 15 minutes :) Depending on what data you read and write you need to set a specific isolation level for the transaction to prevent certain read/write anomalies, see http://en.wikipedia.org/wiki/Isolation_%28database_systems%29

[woogerboy21]

Once again you can only parameterize your mysql statements using mysqli or pdo. The older php 4 compatible mysql_ functionality does not allow for this. I could take one more step and walk through my code verifying passed in content is a certain type (such as making sure its a string) and possibly do something like type casting or throw an error prior to constructing the query. But I would much rather spend my time updating the code to use mysqli and correcting the problem rather than band aiding it.

In regards to improving performance. I already follow the method you described above: BEGIN WORK; sql stuff here COMMIT WORK;

I was simply pointing out the fact that this small application doesn't insert large amounts of data at any one time. In fact the most it ever may insert is updating a single row containing possibly 5 or 6 columns. I will probably do mysqli prepare's for good practice when the code is converted to use those calls but having such a small amount of db contact for a given transaction will just produce unneeded preparation overhead when making a single db call.

Are you thinking there should be some additional functionality in the app that would require massive amounts of data input's?

[shapeshiftyr]

@woogerboy21 what is a good way to contact you? I found some stuff with your server while I was setting up a cockatrice server for reddit.com/r/spikes that you should know about.

[woogerboy21]

woogerboy21@gmail.com

Thanks.

On Jul 29, 2014, at 4:09 PM, "Keaton Mantz" notifications@github.com wrote:

@woogerboy21 what is a good way to contact you? I found some stuff with your server while I was setting up a cockatrice server for reddit.com/r/spikes that you should know about.

— Reply to this email directly or view it on GitHub.