Is this good for preventing mysql injections?

[motormichael12]

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]

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]

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]

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]

How would you recommend finding anything that has # or -- and logging it then?

[Twey]

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).

[blm126]

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]

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]

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]

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?