3
\$\begingroup\$

I'm building a checkout process where I am quite frequently making SQL connections based on user input so this is quite important. I want to know if it's well-protected from any SQL injection or other forms of SQL attack.

I'm currently trying to implement a prepared statement approach with some sanitizing with htmlspecialchars(),preg_match(), etc. Is this a safe function? Any help identifying what to add or change to make this function more secure would be appreciated.

Note: $arg is from user input

//Queries will generally be of this structure->"SELECT * FROM Users WHERE Id ?"

function fetchAssocPreparedStatements($query , $arg , $type) {

$servername = "xxx";
$username = "xxx";
$password = "xxx";
$dbname = "xxx";

$conn = mysqli_connect($servername, $username, $password, $dbname);
if ($conn->connect_error) {
    exit("An error occurred");
} 

     $arg = trim($arg); //try to sanatize
     $arg = stripslashes($arg);
     $arg = htmlspecialchars($arg);
     $arg = preg_replace("/[^a-z0-9\-]+/i", "", $arg); //now using preg_replace

     $stmt = mysqli_stmt_init($conn); //prepare and execute statement
     mysqli_stmt_prepare($stmt, $query);
     mysqli_stmt_bind_param($stmt, $type, $arg);
     mysqli_stmt_execute($stmt);


     $meta = $stmt->result_metadata(); //create assoc array with data
         while ($field = $meta->fetch_field()) { 
            $var = $field->name; 
            $$var = null; 
                $parameters[$field->name] = &$$var; 
         } 

     call_user_func_array(array($stmt, "bind_result"), $parameters);

     $copy = create_function('$a', 'return $a;');
     $results = array();

        while ($stmt->fetch()) {
            $results[] = array_map($copy, $parameters);
        }


     return $results; //returns results and closes the connections
     mysqli_stmt_close($stmt);
     mysqli_close($conn);
}
\$\endgroup\$
3
  • \$\begingroup\$ Titles should only state the code's purpose. Specific requests just stay in the post body. \$\endgroup\$
    – Jamal
    Commented Apr 17, 2016 at 5:08
  • \$\begingroup\$ What the code is for is irrelevant, its more about whether or not the code itself is secure. I did not ask this question for any guidance on a checkout process, i asked this question for guidance on how to make my PHP sqli code more secure against sql injection (as my previous title stated)... i mean seriously, how is "Checkout process" a good representation of my question? (especially since the code provided is only querying a database and saving info, nothing really checkout related just standard stuff) \$\endgroup\$ Commented Apr 17, 2016 at 5:14
  • 2
    \$\begingroup\$ Right, and like I said, the purpose stays in the title. We're not focusing on reviewing what's in the title; that's to tell us what the code is for. Your specific questions will still be read and likely and addressed. \$\endgroup\$
    – Jamal
    Commented Apr 17, 2016 at 5:17

1 Answer 1

2
\$\begingroup\$

Since you are using bind to build your query all of these lines in your code really are not needed.

 $arg = trim($arg); //try to sanatize
 $arg = stripslashes($arg);
 $arg = htmlspecialchars($arg);
 $arg = preg_replace("/[^a-z0-9\-]+/i", "", $arg); //now using preg_replace

From the looks of what you posted you already know what you expect $arg to contain based on the fact that you are passing the bind type to the function ($type). You may use $type to check to make sure that the value of $arg is of the type you are expecting.

\$\endgroup\$
1
  • \$\begingroup\$ yes this is sort of an old post and i removed all the sanitation since they are unnecessary with prepared statements (I Think). Thanks anyways! \$\endgroup\$ Commented Jul 18, 2016 at 22:10

Not the answer you're looking for? Browse other questions tagged or ask your own question.