3
\$\begingroup\$

vir.php

<?php 

require_once '../db.php';

session_start();

if($_SERVER['REQUEST_METHOD'] == 'POST'){ 

$username = trim($_POST['username']);

try{

  $Query = "SELECT * FROM users WHERE username = :username";
  $statement = $conn->prepare($Query);
  $statement->bindValue(':username', $username);
  $statement->execute();
  $user = $statement->fetch(PDO::FETCH_ASSOC);    
  $RowCount = $statement->rowCount();

} catch (PDOerrorInfo $e){

    die('QuerySCD Error '.$e->getMessage());

  }

  if( $RowCount == 0 ){
   // User doesn't exist
    $_SESSION['message'] = "error!";
    header("location: error-login.php");

  } else{ // User exists

      if( password_verify($_POST['password'], $user['password'])){

        $_SESSION['username']  = $user['username'];
        $_SESSION['active']    = $user['active'];

        $_SESSION['logged_in'] = true;

        header("location: riscar.php");

      } else {

          $_SESSION['message'] = "error!";
          header("location: error-login.php");

        }      
    }  
}

$conn = NULL;

?>

<!DOCTYPE html>
<html lang="pt_BR">
<head>
    <title>Login</title>
    <meta charset="UTF-8">
</head>

<body>

    <form action="vir.php" method="post" autocomplete="off">

        <input type="username" required autocomplete="off" name="username">
        <span data-placeholder="User"></span>

        <input type="password" required autocomplete="off" name="password">
        <span  data-placeholder="Password"></span>

        <button name="login">
            Logar
        </button>

    </form>

</body>
</html>

ir.php

<?php
/* Log out process, unsets and destroys session variables */
session_start();
session_unset();
session_destroy(); 
?>

<!DOCTYPE html>
<html lang="pt_BR">
<head>
    <title>Login</title>
    <meta charset="UTF-8">
</head>

<body>

    <p><?= 'You have been logged out!'; ?></p>

</body>
</html>

error-login.php

<?php
/* Displays all error messages */
session_start();
?>

<!DOCTYPE html>
<html lang="pt_BR">
<head>
    <title>Login</title>
    <meta charset="UTF-8">
</head>

<body>

    <h4>
        <?php 
        if( isset($_SESSION['message']) AND !empty($_SESSION['message']) ): 
            echo $_SESSION['message'];    
        else:
            header( "location: vir.php" );
        endif;
        ?>
    </h4> 

</body>
</html>

Only me will use this login sytem and only me will have the link to this login system, i will use this to insert records on my database.

I don't know if someone could find the link to this login, to i used PDO prepared statement, so prevent against SQL Injection.

How safe is my code?

Improve this question
\$\endgroup\$
3
  • \$\begingroup\$ What happens if the pdo statement fails (e.g. because the server went away)? As i see it, this would result in an undefined variable error for $RowCount. \$\endgroup\$
    – Tobias F.
    Sep 26, 2018 at 11:13
  • \$\begingroup\$ @TobiasF. Most likely it will result in the error message telling you that the server went away \$\endgroup\$
    – Your Common Sense
    Sep 26, 2018 at 12:03
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Mast ♦
    Sep 27, 2018 at 7:06

2 Answers 2

Reset to default
1
\$\begingroup\$

Your code is safe, but your attitude is not.

I don't know if someone could find the link to this login, so I used PDO prepared statement

This is the wrong reason to use a prepared statement. Such an attitude will lead to a disaster, sooner or later. You should never mind the data source when protecting from SQL Injection. It's the destination that matters. Would this variable be used in the SQL query? Then add it via prepared statement. No exceptions.

die('QuerySCD Error '.$e->getMessage());

This is a textbook security flaw. By echoing the error message out you may reveal too much of your system internals that can be used by the attacker.

There should never be a code like this. First, a die with the error message is rather useless, as an uncaught exception will kill you script providing the error message all the same. But what is more important, it will direct it to where it belongs: on a live site it most be a log file, not a browser. Read more on the proper error reporting in my article.

Regarding the code itself, I've got a canonical example for the case.

There is also a possible breach in riscar.php, so you better show us its code as well.

Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ As you said on the article: "just leave error messages alone". So i could just delete the code that is on the catch (PDOerrorInfo $e){and leave it blank? Please check my edit on the end of the question. And i will post the riscar.php soon. \$\endgroup\$
    – mind set
    Sep 26, 2018 at 16:50
  • \$\begingroup\$ yes, a try..catch that does nothing but die with error is useless and harmful, so delete it for sure. Regarding your .htaccess, I you don';t have to remove this line but rather just change the value to 0 and add another line with error_log=1 \$\endgroup\$
    – Your Common Sense
    Sep 27, 2018 at 8:36
  • \$\begingroup\$ Alright, and i posted my code from the file "riscar.php" on anohter question, link: codereview.stackexchange.com/questions/204473 \$\endgroup\$
    – mind set
    Sep 27, 2018 at 17:17
1
\$\begingroup\$

How safe is my code?

I would suggest if you protect the login form from

  • CSRF (Cross site request forgery) Attacks by generating a unique code for each session and assigning it to a hidden field in the form, and check every post request whether it matches the one from the session or not.

<input type = "hidden" name="token" value="' . $_SESSION['csrf_token'] . '">

Example: ignore naming convention

   class token {

     public static function generate_csrf_token() {
           if (!isset($_SESSION['csrf_token'])) {
              $_SESSION['csrf_token'] = bin2hex(random_bytes(32));
           }
        }

    public static function check_csrf_token($token) {
        return $token === $_SESSION['csrf_token'];
    }
 }

Then you want to check when the form is submitted and compare both token.

  • Timing attacks, disable logins requests under a second, mostly bots and robots.

        //Gets the value when the page loads and set it in a session
    $start = microtime(true);

    if (!isset($_SESSION['microtime'])) {
     $_SESSION['microtime'] = $start;
    }

    and after the login button is clicked, check and perform the same and then compare the time

    if (isset($_POST['submit'])) {
   $finish = microtime(true);
   $seconds_passed = $finish - $_SESSION['microtime'];
   if ($seconds_passed < 1) {

?>

    • Limit requests from an ip, so for example say, 3 requests maximally allowed per a second, to protect from lots of attack.

    • Putting captcha and login attempts will be useful.

    • Session Hijacking multiple things:

Have an identification method that is making each user unique (perhaps hashing user agent and ip) and check every request if they match to see if the current user is the same. And have a session requests tracker that would re-assign a new session id after 25 requests for example. 

 if (!isset($_SESSION['session_clicked_times'])) {
            $_SESSION['session_clicked_times'] = 1;
        } else {
            if (($_SESSION['session_clicked_times']) >= 25) {
                $_SESSION['session_clicked_times'] = 0;
                session_regenerate_id(true);
            }
            $_SESSION['session_clicked_times'] += 1;
        }

Also generate a new id once logged in, and so on.

This code:

 if( $RowCount == 0 ){
   // error   
  } else{ // User exists

      if(password_verify($_POST['password'], $user['password'])){

     //login

      } else {
         //error
        }      
    }  
}

to just this (since they both handle the same thing in your case)

 if( $RowCount == 0 || !password_verify($_POST['password'], $user['password']) ){
       // error   
      } else { 
       //login
    }
Improve this answer
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Don't check hashes or CSRF tokens with ===, use hash_equals(). \$\endgroup\$
    – esote
    Sep 26, 2018 at 13:01
  • 1
    \$\begingroup\$ By the way, $RowCount is superficial as well. In PHP, $user always could be used instead of the row count \$\endgroup\$
    – Your Common Sense
    Sep 26, 2018 at 13:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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