-1
\$\begingroup\$

One of my friends told me that my validate.php file has some problems with SQL injections. Please feel free to commit it on GitHub.

 <?php
 session_start();
 error_reporting(0);
 include("../config.php");
 $conn = new mysqli($hostname, $username, $password, $db_name);
 $query = mysqli_query($conn,"SELECT * FROM `Administrators` WHERE username = '"   . $_POST["usernamep"] . "' AND password='".  md5($_POST["passwordp"]) . "'");
if(mysqli_num_rows($query) > 0){
$_SESSION["username"] = $_POST["usernamep"];
header("Location: dashboard.php");
}else{
header("Location: index.php?alert");
}
?>

?>

Can you please try and improve my code to prevent SQL injections?

\$\endgroup\$
  • 5
    \$\begingroup\$ What happens if I enter Bobby'; DROP TABLE Administrators;-- as my user name? (hint: don't try it) \$\endgroup\$ – Phrancis Jan 19 '16 at 16:58
  • \$\begingroup\$ Great idea let me fix it \$\endgroup\$ – Thomas Wilbur Jan 19 '16 at 16:58
  • \$\begingroup\$ The password can mess your SQL up as well. Check the answers on this SO post out: stackoverflow.com/questions/332365/… \$\endgroup\$ – Hosch250 Jan 19 '16 at 17:00
  • \$\begingroup\$ github.com/poKe-Websites/poKe-Multimedia-editor/commit/… \$\endgroup\$ – Thomas Wilbur Jan 19 '16 at 17:00
  • \$\begingroup\$ @Phrancis probably nothing, mysqli_query doesn't support multiple queries as far as I know. Still, an attacker can bypass the login check, read out database information, and depending on settings write to files (meaning that they gain code execution). \$\endgroup\$ – tim Jan 19 '16 at 17:03
5
\$\begingroup\$

It's not just your validate.php file, it's a problem accross your code. See eg here.

  • always use prepared statements when putting variables in queries. It is not ok to just put them into queries, and addslashes is really not the proper solution either.
  • don't use md5. It's too fast, it's broken, and it hasn't been a proper approach to hashing since at least 15 years. Use bcrypt instead.
  • always die after a redirect. Clients do not have to follow them, meaning that any code that comes after a redirect can be executed. This may or may not be a problem right now, but it's just good practice.
\$\endgroup\$
1
\$\begingroup\$

As suggested by tim, you should be using prepared statements whenever you need to use user supplied input as part of a query, but at the very least, you should wrap any user supplied input in a mysql_real_escape_string() function.

Passing any type of raw user input to a database is asking for trouble. I would also suggest making use of the PHP PDO driver for databases, rather than they MySQLi driver, as when you change your code to properly use prepared statements rather than directly passing user input, you'll find that the PDO way of parameter binding is much more expressive than the MySQLi way.

\$\endgroup\$
  • \$\begingroup\$ Thx Can You help with that \$\endgroup\$ – Thomas Wilbur Jan 26 '16 at 0:58
  • \$\begingroup\$ This is an excellent resource that should help you get started with PDO and using prepared statements to execute your queries. \$\endgroup\$ – jlswint Jan 28 '16 at 21:20
  • \$\begingroup\$ Great idea why not you help github.com/poKe-Websites/poKe-CMS \$\endgroup\$ – Thomas Wilbur Jan 29 '16 at 16:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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