I've been asked to handle a security issue for a site which was set up by another programmer. As of yet, I haven't seen any of the code, so I'm going off of assumptions at this point and I want to cover my bases. The group hosting the site ran a security check and found that they had code vulnerable to SQL injection.
Example: www.example.com/code.php?pid=2&ID=35 (GET parameter ID is vulnerable to SQL Injection)
Now, because I'm a novice, I've explained that I can likely resolve the issue with the host, but their site would still need to be looked over by someone who has a deeper knowledge of security.
So, to take care of potential SQL Injections (and without seeing the code), I would use mysql_real_escape_string:
$query = sprintf("SELECT * FROM table WHERE pid='%s' AND ID='%s'",
mysql_real_escape_string($pid),
mysql_real_escape_string($id));
Additionally, I would consider mysqli_real_escape_string and prepared statements, but I don't know how they're configured. But would mysql_real_escape_string take care of potential SQL Injection?
Skip the old mysql_*
stuff if you can and use PDO.
$pdo = new PDO('mysql:host=localhost;dbname=whatever', $username, $password);
$statement = $pdo->prepare('SELECT * FROM table WHERE pid=:pid AND ID=:id');
$statement->bindParam(':pid', $_GET['pid']);
$statement->bindParam(':id', $_GET['id']);
$results = $statement->execute();
var_dump($results->fetchAll());
That function should be fine - your variables are inside single quotes in the SQL statement, and any single, or double quotes will be escaped.
This means that none of the variables can "break out" of the statement.
Yes, mysql_real_escape_string() will escape any potentially hazardous characters. If you know that the arguments are numeric, it would not hurt to verify this aswell using is_numeric()
You should also look at mysql::prepare -- This will ensure that only 1 statement is executed, and prevent additional SQL vulnerabilities.
If ID and PID are integer fields, why not casting them to int.
That way, you are sure to have a number, an no SQL injection :
$pid = (int) $pid;
$id = (int) $id;
That should be fine, but i'd always recommend using prepared statements.