PDA

View Full Version : Is this good for preventing mysql injections?



motormichael12
08-05-2008, 10:46 PM
I am working on a game that needs to be safe from injections, and I was wondering if this was sufficient for protection:


function clean_field($i)
{
if (substr_count_array($i, array("#", "--"))) {
log_hack($i);
}
if (!preg_match('/^[a-zA-Z0-9]+$/', $i)) {
//$errors = "Invalid characters: $i";
$problem = TRUE;
}


if($problem){
return FALSE;
}
else
{
return TRUE;
}
}

function log_hack($data)
{
$timestamp = date('d/m/Y H:i:s');
$ip = $_SERVER['REMOTE_ADDR'];
$handle = fopen("hack_attempts.php", 'a+');
fwrite($handle, "$timestamp|| $data|| $ip\n");
fclose($handle);

}

function substr_count_array( $haystack, $needle ) {
$count = 0;
foreach ($needle as $substring) {
$count += substr_count( $haystack, $substring);
}
return $count;
}

Any suggestions to make it better are welcome.

blm126
08-05-2008, 11:38 PM
The function substr_count_array isn't needed. Your regular expression handles that. However, that is the wrong way to go about it. All you need to do is make sure that all data is run through mysql_real_escape_string or similar before it is placed in SQL query.

motormichael12
08-06-2008, 12:42 AM
Well the substr_count_array is to get mysql comments (# or --) so that if anything looks like it could be a hack it is saved in a log.

However I also only want alphanumeric from this function, so would this work for that since I don't want to escape but rather return the error?

blm126
08-06-2008, 02:55 AM
No, it is opening another huge security hole. Consider what would happen if "#<?php unlink('index.php'); ?>" was passed to clean_field(). This is an even bigger security hole than SQL injection.

motormichael12
08-06-2008, 03:54 AM
How would you recommend finding anything that has # or -- and logging it then?

Twey
08-06-2008, 04:43 AM
Injection into what? Apply htmlentities() if it's going on a PHP or HTML page, mysql_real_escape_string() if it's going into a database, &c. (although there are better alternatives, such as PDO's prepared statements or an ORM like Propel (http://propel.phpdb.org/)).

blm126
08-06-2008, 05:03 PM
I don't get why you would want to log it. Just prevent the injection(which, as Twey said, depends on where the data is going). The log want show you any useful information. It's the hacks that your filters(and therefore your log) don't catch that you need to worry about.

motormichael12
08-06-2008, 05:46 PM
Well the log isn't to prevent anything but just to see if it is a possible attempted injection, this way I can see when people might be trying.

The preg_match is because I only want alphanumerics in data that passes through.

After it checks those, I want to use the mysql_real_escape_string before saving fields and textareas to the database, as not all data will go through clean_field.

Is that a bad idea?

blm126
08-06-2008, 06:05 PM
Not really in theory. However, the code you have posted here has a huge security hole. Basically, if PHP code gets passed to clean_field, it will be written to your log file(which is a PHP file). This would cause complete access to the server.

motormichael12
08-06-2008, 06:11 PM
I realize what you mean by that being a securtity hole. If someone put in what you said earlier, when I load the page it will execute the PHP.

What if I saved it as a text file, instead of hack_attempts.php make it hack_attempts.txt?

The file isn't going to be placed in an HTML file, but rahter I will get an alert when there is a new hack attempt and then go to the file. If it is text, it won't execute anything.

Would that work?

motormichael12
08-08-2008, 02:54 AM
I messed around with it and tested different ways, and my final solution was this:

function clean_field($i, $allowed='')
{
if (substr_count_array($i, array("#", "--"))) {
log_hack($i);
}
if (!preg_match('/^[a-zA-Z0-9\s' . $allowed . ']+$/', $i)) {
//$errors = "Invalid characters: $i";
$problem = TRUE;
}


if($problem){
return FALSE;
}
else
{
return TRUE;
}
}

and


$username = $_POST['username'];
$password = $_POST['password'];
$password_confirm = $_POST['password_confirm'];
$email = $_POST['email'];
$email_confirm = $_POST['email_confirm'];
$gender = $_POST['gender'];

$username_check = clean_field($username);
$password_check = clean_field($password);
$password_confirm_check = clean_field($password_confirm);
$email_check = clean_field($email, "@\.");
$email_confirm_check = clean_field($email_confirm, "@\.");

if(!$username_check && !empty($username))
{
$errors[] = "The username contains invalid characters.";
}
if(empty($username))
{
$errors[] = "You did not enter a username.";
}
if($username_check)
{
$taken = mysql_num_rows(mysql_query("SELECT * FROM `users` WHERE `username`='".$username."'"));
if($taken > 0 && $username_check)
{
$errors[] = "The username \"$username\" is already taken.";
}
}

if($password != $password_confirm && !empty($password))
{
$errors[] = "The passwords do not match.";
}
if(!$password_check && !empty($password))
{
$errors[] = "The passwords contain invalid characters.";
}
if(empty($password))
{
$errors[] = "You did not enter a password.";
}

if($email != $email_confirm && !empty($email))
{
$errors[] = "The email addresses do not match.";
}
if(!$email_check && !empty($email))
{
$errors[] = "The email addresses contain invalid characters.";
}
if(empty($email))
{
$errors[] = "You did not enter an email address.";
}

if(!in_array($gender, array(0,1,2)))
{
$errors[] = "Invalid Gender";
}

if(isset($errors))
{
for($i = 0; $i < count($errors); $i++)
{
echo $errors[$i] . "<br>";
}
}
else
{
$hash_pass = sha1($password);
$regtime = time();
$sql = "INSERT INTO `users` VALUES(NULL, 1, '{$username}', '{$hash_pass}', 0, 500, 0, 1, 10, 10, 10, 10, 0, 10, 10, 10, 10, 10, 0, 0, '{$email}', {$gender}, '', '', 0, {$regtime}, '', 0, '', 0, 0, '', '');";
mysql_query($sql);
}