I have created a simple function to post data into a mysql table using a function and an array. The array is built from $_POST items on a form. What I would like to know is: Are there potential security holes that I am not seeing?
Here is the function:
public function add_sql_data($table,$array){
$tot = count($array);
$c=0;
foreach($array as $k => $v){
$fields = $fields.$k;
$values = $values."'".$v."'";
$c++;
if($c < $tot){
$fields = $fields.",";
$values = $values.",";
}
}
$sql = "INSERT INTO ".$table."(".$fields.") values(".$values.")";
if (mysql_query($sql)){
return "succesfull";
}else{
return "error";
}
}
I have tried to use the small amount of PHP I know to create a SQL injection, but it seams to me that the array is actually stopping any harmful syntax to run. Thanks!!
In the sql in the question - there is no sanitization or escaping. Consider what will happen with individual form inputs like this:
In the first, there's going to a syntax error of some sort, and also in the second.
The 3rd is just a hint at the tip of an iceberg of the sort of injection you are making possible. Here, we're injecting the value of another table into this insert statement, if that field is visible to the end user they can do things like this to explore your database and gain access to information they shouldn't be able to see. Just google sql injection and you'll find an endless list of examples.
You really, really should be using PDO and preferably prepared statements instead of the older mysql_* functions.
An example of something similar to what you're doing that is safe from injection is:
$dbh = new PDO('mysql:host=localhost;dbname=test', $user, $pass);
$stmt = $dbh->prepare("INSERT INTO table (name, value) VALUES (?, ?)");
$stmt->bindParam(1, $name);
$stmt->bindParam(2, $value);
// insert one row
$name = 'one';
$value = 1;
$stmt->execute();
In this way, it doesn't matter what $name or $value are - they will be escaped appropriately.
It is not safe. Don't directly insert data from $_POST into MySQL. Please read on SQL injection.
You can try your code like this..
public function add_sql_data($table,$array){
$tot = count($array);
$c=0;
foreach($array as $k => $v){
$fields = $fields.$k;
$values = $values."'".$v."'";
$c++;
if($c < $tot){
$fields = $fields.",";
$values = $values.",";
}
}
/* To protect MySQL injection you can do like this */
$values= stripslashes($values);
$values = mysql_real_escape_string($values);
$sql = "INSERT INTO ".$table."(".$fields.") values(".$values.")";
if (mysql_query($sql)){
return "succesfull";
}else{
return "error";
}
}
I see 3 problems:
By inserting data this way (function, with all the data in a array), you are very only a few steps away from using parameter binding, witch solves te SQL injection problem. For Cross Site Scripting use have to use a validation mechanism.