How can I improve this code to not be hacked easily?

Question

I am a victim of sql injection, they hacked into my mysql database and they started deleting data and also they changed passwords. I thought using PDO am safe. This is my login page for PHP. I need help where did I go wrong here which led to the attacks. I would like to be advised on how to improve this code to avoid similar attacks to happen again.

<?php session_start(); ?>
<?php
require_once('dbconnect/pdo.inc.php');
$username = (isset($_POST['username'])) ? trim($_POST['username']) : '';
$password = (isset($_POST['password'])) ? trim($_POST['password']) : '';
$pas = md5($password);
$redirect = (isset($_REQUEST['redirect'])) ? $_REQUEST['redirect'] :
        'index.php';
$result = array();
$result['error'] = FALSE;
$result['message'] = "";
//if(isset($_POST['submit'])){

if (empty($password)) {
    $result['error'] = true;
    $result['message'] = "enter password";
//json encode and echo reusult.
    $res = json_encode($result);
    echo $res;
    exit();
}
if (empty($username)) {
    $result['error'] = true;
    $result['message'] = "enter username";
//json encode and echo reusult.
 $res = json_encode($result);
    echo $res;
    exit();
}
$query = ("SELECT username FROM users WHERE username=:username
 AND password =:password");
$query_login = $con->prepare($query);
$query_login->execute(array(
    ':username' => $username,
    ':password' => $pas));
$results = $query_login->rowCount();
if ($results > 0){
    $_SESSION['username'] = $username;
    $_SESSION['logged'] = 1;
    $result['error'] = false;
    $result['message'] = 'Successfully logedin';
        header('Location:index.php');
    $res = json_encode($result);

    echo $res;
}
else{
//set these explicitly just to make sure 
    $result['error'] = true;
    $result['message'] = 'User name invalid';
    header('Location:login.php');
 $res = json_encode($result);
    echo $res;
    exit();
}
//  }
?>

//This is the way I connect to the database

<?php
function connected_Db(){

    $dsn  = 'mysql:host=localhost;dbname=usaDB;charset=utf8';
    $opt  = array(
        PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC
    );
    #echo "Yes we are connected";
    return new PDO($dsn,'brrmingham','m#67yhfdelkomngf_()likf4', $opt);

}
$con = connected_Db();
if($con){
//echo "connected ";
}
else {
//echo "Connection faid ";
exit();
}
?>

For starters don't use a fast algorithm like `md5()` see [Safe Password Hashing](https://www.php.net/manual/en/faq.passwords.php) ... rewrite the sql into `SELECT password FROM users WHERE username=:username` and use `password_verify()` when password is also in the `WHERE` clause it comes prone to [timing attacks](https://en.wikipedia.org/wiki/Timing_attack) again as databases are designed to return the data as quick as possible especially when password is also included in the index. — Raymond Nijland, Aug 08 '19 at 20:15
This code looks pretty safe, maybe a different query? Also, maybe they hacked the server itself through webserver vulnerability or something else. — AbraCadaver, Aug 08 '19 at 20:15
This particular code only contains a single query, and you're using a prepared statement correctly to protect against SQL injection. Are you 100% sure this is the query they used? — rickdenhaan, Aug 08 '19 at 20:15
I'm not sure how authentication works in PHP. But in C# when you log in a token is created. Here I see only set two variables `$_SESSION['username'] = $username; $_SESSION['logged'] = 1;` so you can change those variable with a fiddle. — Juan Carlos Oropeza, Aug 08 '19 at 20:19
*"I thought using PDO am safe"* PDO is only really safe when you define a charset `SET NAMES utf8` as query or `charset=utf8` when connecting and **dont** use client side prepares -> `PDO::setAttribute(PDO::ATTR_EMULATE_PREPARES, false)` ... — Raymond Nijland, Aug 08 '19 at 20:21
Also using `$_REQUEST` is also not really safe as the values can come from $_GET, $_POST and $_COOKIE you get the point i hope? .. Also a `header("Location: ...") is missing a `exit()` after — Raymond Nijland, Aug 08 '19 at 20:26
you haven't shared enough information to say for sure. it looks like you are maybe using PDO for your query, but we don't know unless we see how $con gets defined. Also, you default ERROR to FALSE -- if there is more than one account with the same username and password that would let a person get through. We also don't know if you enforce any password security when creating accounts, etc. You also fail to check if the query succeeded. You should probably require that they enter some non-empty password. — S. Imp, Aug 08 '19 at 20:26
I'd also point out that SQL injection may happen at some other point than your login page. What makes you think it was this page in particular that contained the vulnerability? — S. Imp, Aug 08 '19 at 20:28
On top of everything here, never `trim()` a password. A space is a perfectly valid character for a password. — ceejayoz, Aug 08 '19 at 20:28
[Are PDO prepared statements sufficient to prevent SQL injection?](https://stackoverflow.com/q/134099/1839439) — Dharman, Aug 08 '19 at 20:43
I really appreciate all your views guys and am trying to do more and more investigations to see how did they break in . — Humphrey, Aug 09 '19 at 06:37
@RaymondNijland I edited my question and added the way I connect to the databse — Humphrey, Aug 09 '19 at 07:08
If your updated question now contains your actual database username and password, **change the password immediately**. You just made it easier to get hacked. — rickdenhaan, Aug 09 '19 at 14:38

How can I improve this code to not be hacked easily?

0 Answers0