Making a secure login in PHP

Question

I have made this PHP login page. As far as I know, this is secure enough to avoid attacks and injection.

This script:

Notifies me with an e-mail when someone logs in or fails to login
Only one user at a time
CAPTCHA
Secure session
Auth lasts for 20 minutes
Password is stored with MD5 hash
Max 3 login attempts, then it disables your IP for 1 hour

Can someone better than me tell me how safe this script is?

       <?php
   header("X-Frame-Options: DENY");
   sec_session_start();


   // controllo autenticazione........

   if(checkAuth()){
    $user = trim($_POST['user']);
    $pass = trim($_POST['pass']);
    $captcha_response = trim($_POST['g-recaptcha-response']);

    if(isset($user) && isset($pass) && isset($captcha_response) && $user != "" && $pass != "" && $captcha_response != "" &&            captchaControl($captcha_response)){
        //captcha valido
        $account = getAccount(); /* recupero dati account*/
        cleanExpiredAttemps();
        if(checkAttempsValidation()){
            //controllo dati
            if($user == $account->user && MD5($pass) == $account->pass){
                //dati validi
                //autentico
                autenticate();


                //send mail
                $mails = getMails();
                $arr = json_decode(file_get_contents("http://ip-api.com/json/".getIp()),true);
                $mailMsg = "Login success

                Date:  ". date('D, d M Y H:i:s')."
                Ip: ".getIp()."
                Citta (circa): ".$arr['city']."
                Provider: ".$arr['isp'];

                for($i = 0;$i < $mails->lenght;$i++){
                    mail($mails[$i],"Login effettuato",$mailMsg);
                } 

            }else{
                //dati non validi
                //login attemps +1
                addLoginAttemp();
                outputLoginForm();
            }
        }else{

            //send mail
                $mails = getMails();
                $arr = json_decode(file_get_contents("http://ip-api.com/json/".getIp()),true);
                $mailMsg = "Login wrong 3 times:

                Date:  ". date('D, d M Y H:i:s')."
                Ip: ".getIp()."
                City (circa): ".$arr['city']."
                Provider: ".$arr['isp'];

                for($i = 0;$i < $mails->lenght;$i++){
                    mail($mails[$i],"Login effettuato",$mailMsg);
                }
            echo "<p align='center'>Ti sei collegato troppe volte<br>sarai disconnesso per 1 ora<br></p>";
            outputLoginForm();
        }

    }else{
        //chiedi login
        outputLoginForm();
    }
   }

   //*************************************************************************************************************************           *****//    

   echo "autenticato";





   //*************************************************************************************************************************           *****//    

   function outputLoginForm(){
    die ("<meta name='viewport' content='width=450px', initial-scale=1.0'>
         <style>

            input{
                margin-bottom: 5px;
                width: 100%;
                height: 25px;

            }
            form{
                padding: 10px;
                border: 1px solid #dddddd;
                width: 300px;
                margin: 0 auto;
            }
            div{
                margin-bottom: 5px;
            }
            h2{
                margin: 0;
                margin-bottom: 3px;
                text-align: center;
            }
            </style>
            <script src='https://www.google.com/recaptcha/api.js'></script>
            <form action='#' method='post'>
            <h2>Login</h2>
            <input  name='user' placeholder='username'><br>
            <input type='password' name='pass' placeholder='password'><br>
            <input type='hidden' name='view' value='home'>
            <div class='g-recaptcha' data-sitekey='6LdybQcUAAAAADckezXhCvnYziDhLCwKwKrdVyFE'></div>
            <input type='submit'>
            </form>");
   }

   //*************************************************************************************************************************           *****//

   function cleanExpiredAttemps(){
    $mysqli = openDatabaseConn();
    if ($query = $mysqli->prepare("SELECT id,ip,last_attemp_time from login_attemps")) {
        $result = $query->execute(); /* execute query */
        $query->store_result();
        $query->bind_result($id,$ip,$last_attemp_time);
        if($result){
            if($query->num_rows > 0){

                //se ce ne uno o piu
                $query2 = $mysqli->prepare("DELETE from login_attemps WHERE id = ?");
                $query2->bind_param("i",$id_query2); 
                for($i = 0;$i < $query->num_rows; $i++){
                    $query->fetch();
                    if((time() - $last_attemp_time > 3600)){
                        $id_query2 = $id;
                        $query2->execute();
                    }
                }
                $query2->close();
            }
        }else{
            echo "Errore sconosciuto<br>";
                outputLoginForm();
        }

        $query->close();
    }else{
        echo "Errore sconosciuto<br>";
        outputLoginForm();
    }

    closeDatabaseConn($mysqli);
   }

   //*************************************************************************************************************************           *****//

   function checkAttempsValidation(){
    $mysqli = openDatabaseConn();

    if ($query = $mysqli->prepare("SELECT * from login_attemps WHERE ip = ?")) {
        $query->bind_param("s",getIp()); /*bind params*/
        $result = $query->execute(); /* execute query */
        $query->store_result();
        if($query->num_rows < 3 && $result){
            return true;
        }else{
            return false;
        }
        $query->close(); /* close statement */
    }
    closeDatabaseConn($mysqli);
   }

   //*************************************************************************************************************************           *****//

   function addLoginAttemp(){
    $mysqli = openDatabaseConn();

    if ($query = $mysqli->prepare("INSERT into login_attemps (ip,last_attemp_time) VALUES (?,?)")) {
        $query->bind_param("si",getIp(),time()); /*bind params*/
        $result = $query->execute(); /* execute query */
        $query->close(); /* close statement */
    }
    closeDatabaseConn($mysqli);
   }

   //*************************************************************************************************************************           *****//

   function captchaControl($response){
        $secret = "...";
        $remoteIp = getIp();
        $request = "https://www.google.com/recaptcha/api/siteverify?secret=$secret&response=$response&remoteip=           $remoteIp";

        $arr = json_decode($request);
        $result = $arr['success'];

        return $result;
   }

   //*************************************************************************************************************************           *****//

   class account{
    public $user;
    public $pass;

    public function account($user,$pass){
        $this->user = $user;
        $this->pass = $pass;

    }
   }

   //*************************************************************************************************************************           *****//

   function getAccount(){

    $mysqli = openDatabaseConn();

    if ($query = $mysqli->prepare("SELECT user,pass FROM login_account")) {

        $result = $query->execute(); /* execute query */
        if($result){
            $query->bind_result($db_user,$db_pass); /* bind result variables */
            $query->fetch();
            $account = new account($db_user,$db_pass);
            $query->close(); /* close statement */
        }else{
            outputLoginForm();
        }


    }

    closeDatabaseConn($mysqli);

    return $account;
   }

   //*************************************************************************************************************************           ****//

   function getMails(){

    $mysqli = openDatabaseConn();

    if ($query = $mysqli->prepare("SELECT mail FROM mail_account_to_notify")) {

        $query->execute(); /* execute query */
        $query->store_result();
        $query->bind_result($mail); /* bind result variables */
        for($i = 0; $i < $query->num_rows; $i++){
            $query->fetch();
            $mails[$i] = $mail;
        }

        $query->close(); /* close statement */
    }

    closeDatabaseConn($mysqli);

    return $mails;
   }

   //*************************************************************************************************************************           *****//

   function sec_session_start() {
           $session_name = 'sec_session_id'; // Imposta un nome di sessione
           $secure = false; // Imposta il parametro a true se vuoi usare il protocollo 'https'.
           $httponly = true; // Questo impedirà ad un javascript di essere in grado di accedere all'id di sessione.
           ini_set('session.use_only_cookies', 1); // Forza la sessione ad utilizzare solo i cookie.
           $cookieParams = session_get_cookie_params(); // Legge i parametri correnti relativi ai cookie.
           session_set_cookie_params($cookieParams["lifetime"], $cookieParams["path"], $cookieParams["domain"], $secure, $           httponly); 
           session_name($session_name); // Imposta il nome di sessione con quello prescelto all'inizio della funzione.
           session_start(); // Avvia la sessione php.
           session_regenerate_id(); // Rigenera la sessione e cancella quella creata in precedenza.
   }

   //*************************************************************************************************************************           *****//
   function autenticate(){
    //inserire in current login
    $mysqli = openDatabaseConn();

    if ($query = $mysqli->prepare("INSERT into current_login (ip,security_code,time) VALUES (?,?,?)")) {
        $security_code = generateSecurityCode();
        $query->bind_param("ssi",getIp(),$security_code,time()); /*bind params*/
        $result = $query->execute(); /* execute query */
        $query->close(); /* close statement */
    }
    closeDatabaseConn($mysqli);
    if($result){
        $_SESSION['security_code'] = $security_code;
    }else{
        //arresta tutto ,richiedi dati
        echo "Errore sconosciuto<br>";
                outputLoginForm();
    }


   }

   //*************************************************************************************************************************           *****//

   function checkAuth(){
    //controllo autenticazione
    $mysqli = openDatabaseConn();

    //check expiring
    if ($query = $mysqli->prepare("SELECT login_id,time from current_login")) {

        $result = $query->execute(); /* execute query */
        $query->store_result();
        if($result){
            if($query->num_rows > 1){
                echo "1";
                //se ce ne piu di uno
                $query2 = $mysqli->prepare("DELETE from current_login");
                $query2->execute();
                $query2->close();

                echo "<p align='center'>Errore<br></p>";
                outputLoginForm();
            }else if($query->num_rows == 1){
                echo "2";
                $query->bind_result($login_id,$time);
                $query->fetch();

                if((time()-$time) >= 1200){
                    echo "3";
                    //scaduto
                    $query3 = $mysqli->prepare("DELETE from current_login WHERE login_id = ?");
                    $query3->bind_param("i",$login_id);
                    $query3->execute();
                    $query3->close();
                }
            }
        }else{
            echo "Errore sconosciuto<br>";
                outputLoginForm();
        }
    }else{
        echo "Errore sconosciuto<br>";
        outputLoginForm();
    }

        $query->close();


    if ($query = $mysqli->prepare("SELECT ip,security_code,time from current_login")) {
        $result = $query->execute(); /* execute query */
        $query->store_result();
        if($result){
            if($query->num_rows == 0){
                echo "4";
                return true;
            }else if($query->num_rows == 1){
                echo "5";
                $query->bind_result($ip,$security_code,$time);
                $query->fetch();
                if($ip == getIp() && $security_code == $_SESSION['security_code'] && (time()-$time) < 1200){
                    echo "6";
                    return false;
                }else{
                    echo "7";
                    echo "<p align='center'>Utente gia' collegato<br></p>";
                    outputLoginForm();
                }
            }else if($query->num_rows > 1){
                echo "8";
                $query2 = $mysqli->prepare("DELETE from current_login");
                $query2->execute();
                $query2->close();
                return true;
            }else{
                echo "<p align='center'>Errore sconosciuto<br></p>";
                outputLoginForm();
            }
        }else{
            echo "<p align='center'>Errore sconosciuto<br></p>";
            outputLoginForm();
        }
        $query->close(); /* close statement */

    }

    closeDatabaseConn($mysqli);

   }

   //*************************************************************************************************************************           *****//

   function openDatabaseConn(){

    $mysqli = new mysqli("host", "user", "pass", "db");

    if (mysqli_connect_errno()) {
        echo "<p align='center'>Connect failed: ".mysqli_connect_error()."<br></p>";
        outputLoginForm(); ///////////////////////////////////////////////////////////////////
    }
    return $mysqli;
   }

   //*************************************************************************************************************************           *****//

   function closeDatabaseConn($conn){
    $conn->close(); /*close connection*/
   }

   //*************************************************************************************************************************           *****//

   function generateSecurityCode(){
    $string = "";
    $arr = array("a","b","c","d","e","f","g","h","i","l","m","n","o","p","q","r","s","t","u","v","w","x","y","z","k","j","A","B","C","D",           "E","F","G","H","I","L","M","N","O","P","Q","R","S","T","U","V","W","X","Y","Z","K","J","0","1","2","3","4","5","6","7           ","8","9");
    for($i = 0; $i < 32;$i++){
        $string .= $arr[rand(1,60)];
    }
    return $string;
   }

   //*************************************************************************************************************************           *****//

   function getIp(){
    $ipaddress = '';
       if (getenv('HTTP_CLIENT_IP'))
           $ipaddress = getenv('HTTP_CLIENT_IP');
       else if(getenv('HTTP_X_FORWARDED_FOR'))
           $ipaddress = getenv('HTTP_X_FORWARDED_FOR');
       else if(getenv('HTTP_X_FORWARDED'))
           $ipaddress = getenv('HTTP_X_FORWARDED');
       else if(getenv('HTTP_FORWARDED_FOR'))
           $ipaddress = getenv('HTTP_FORWARDED_FOR');
       else if(getenv('HTTP_FORWARDED'))
          $ipaddress = getenv('HTTP_FORWARDED');
       else if(getenv('REMOTE_ADDR'))
           $ipaddress = getenv('REMOTE_ADDR');
       else
           $ipaddress = '';

    return $ipaddress;
   }

   //*************************************************************************************************************************           *****//
   ?>

MD5 is no longer considered safe. Use the password_hash function in PHP instead: us3.php.net/manual/en/function.password-hash.php — Dave
– Dave, Commented Sep 30, 2016 at 14:20

Exhibitioner · Accepted Answer · 2016-10-03 20:11:56Z

The code seems much like spaghetti, there's a lot of functions which could be more efficient if written in other ways and all of the functions are jammed up together making it very difficult if you were to debug or wanted to change out things; as some of these have been mentioned in the other reply.

I'd recommend using an object-oriented approach for this type of system as it would mean the components wouldn't be as tightly coupled and you would easily be able to maintain it, with the current code you're looking for a hard time if something goes wrong. If you were to take the object-oriented route you would be able to create classes for each group, e.g. a database class which would fully handle all queries and such rather than repeating yourself (see the SOLID principle).

Also avoid using MD5 for password hashing as this is no longer/if it ever was considered a good practice, instead you could opt for the likes of bcrypt, scrypt, PBKDF2 and so on; bcrypt would be the easiest of which to implement in your case as there is a native function password_hash() in PHP >= 5.5.0. I noticed your generateSecurityCode() function, do not attempt to create your own "secure" code generator, instead use a well-tested CSPRNG function such as random_int() available in PHP7 >.

Avoid other little things such as unnecessary comments on functions/methods which are obvious as also has been stated as you should already understand what that specific method does, instead document functions rather than the code within them to explain what the purpose of the function is, e.g. create a user using the arguments: $email, $password etc. I would also try to avoid mixing PHP with HTML, using PHP to echo HTML isn't considered best practice and often can lead to bad habits; look into template engines. Instead of publicly displaying the database connection error, display a user friendly message and log the error to a text file outside of the web directory so that you can later diagnose the issue. outputLoginForm(), remove this function.

Mike Brant · Accepted Answer · 2016-10-03 19:06:55Z

I would be very concerned about any login/authentication/authorization model written in this sort of procedural fashion. You are in essence building in significant application fragility and tightly coupled dependencies.

With the high-level support that PHP has built-in for working in an object-oriented fashion over the last decade (and this continuing to be the primary focus for PHP development), there really are few reasons for someone to code in a procedural fashion for such a core piece of functionality in an application.

Also with the number of good libraries or framework components that deal with this specific functionality. I would strongly consider using something like this rather than rolling your own solution. The solutions are much more likely to address all the security edge cases, to be well-tested (ideally with unit test coverage) and will likely be much more re-usable/portable across your applications.

As for your other code, it is extremely tightly coupled. You really should be thinking about how to separate concerns like...

request validation (do you have a properly formed request to even work with?); - data input validation/sanitization;
setting up dependencies (i.e. database connections);
authentication;
authorization;
display;
sending email;
etc.;

...into their own classes or logic sections.

This code really is a bit of a mess to be honest. You one attempt to implement an object (account) is sort of ill-conceived in that it is really no different than just using a stdClass object. There are no meaningful methods on the class and no encapsulation of data.

Many of you functions have to instantiate their own dependencies (i.e. DB connection) rather than just have the dependency passed to them. So you end up setting up and tearing down DB connection way more than needed (you probably should only need to do this once.)

You have coding style issues around:

inconsistent indentation.
inconsistent whitespace around flow control structures (i.e. brackets)
lines lengths that are way too long (try to stick to 80 characters)
spurious comment separators to delineate different code sections (different sections of code - classes, related functions, etc. should really be split up into different files).
unneeded comments (Why do you need a /* close connection */ comment on a function/or method name that clearly should indicate what is happening.
mixed languages in comments
etc.

This all makes your code hard to read and hard to review.

First at all thank you for the answer. I know that my code is messy, probably i will fix it but this was only a test. And i'm studying php. — user118949
– user118949, Commented Sep 30, 2016 at 17:57
@Francesco If you really want to learn PHP, try to avoid learning resources which are heavily procedural with regards to samples. These are probably bad learning resources. Consider taking a look at a few of the popular frameworks. Step through their code around this functionality and see how they do it. — Mike Brant
– Mike Brant, Commented Sep 30, 2016 at 20:14

Stack Exchange Network

Making a secure login in PHP

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Making a secure login in PHP

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions