3
\$\begingroup\$

I wrote the code to log in php to the admin panel. Everything works as it should. But I am not sure if it is well written code and if it is safe. I care about security. I have read a lot about it but I am still not sure. Please, help me.

<form method="post">
   <input type="email" name="email" placeholder="Email">
   <input type="password" name="password" placeholder="Password">
   <button type="submit" name="submit">Submit</button>
</form>

LOGIN PAGE:

session_start();
session_regenerate_id();
if(isset($_POST['submit'])){
   unset($_POST['submit']);
      if(in_array('', $_POST)){
         //Errors for empty input
      } else {
         if(filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)){
            if(Check if the user exists in the database){
               if(password_verify($_POST['password'], Password field in db)){
                  $_SESSION['loggedin'] = 1;
                  header('Location: welcome');
                  exit();
               } else {
                  //Wrong login credentials
               }
            } else {
               //Wrong login credentials
            }
         } else {
            //Wrong email
         }
      }
   }
}

PAGE AFTER LOGIN:

session_start();
session_regenerate_id();
if(empty($_SESSION['loggedin'])){
   header('Location: login');
   exit();
}
\$\endgroup\$
1
  • \$\begingroup\$ I don't think there is a lot to review here. What you presented is the general flow of validation but it does not allow us to judge the overall security. For instance, the code for password_verify is not present. How can we judge that it will always work as intended ? What is relevant for the purpose of security is the database layer and whether your code is free of SQL injections. \$\endgroup\$ – Anonymous Apr 8 at 18:16
2
\$\begingroup\$

The devil is in the details, and you are omitting a lot of details from your code that usually go wrong. That said, I at least saw the following issues in your code:

session_regenerate_id regenerates the... id... of the session. I am not sure why you do this, but it is not necessary and will likely cause issues where people are logged out unexpectedly on unstable networks where a page might not load for any reason. Your logged in session is also valid indefinitely. If an attacker can get their hands on a session identifier, they can keep their session alive indefinitely. You might want to add an expires_on timestamp to your session that is checked to limit the lifetime of any session.

While not wrong perse, unset($_POST['submit']); does not add anything either. It does not do anything for you security-wise at least.

Your check in_array('', $_POST) is a bit convoluted. It uses that in_array checks values, even if you pass it an array with key-value pairs instead of one with numeric indexes. It is probably the reason why you unset the submit button, but at least in the documentation it is only used with numeric indexed arrays. You might want to check if password and email are set instead with empty($_POST['password']) || empty($_POST['email']). This also guards against warnings when password is not set. A second quirk of in_array is that your check will return true with the following array due to type conversion: [ 'remember_me' => 0 ].

I am not sure why you use filter_var($_POST['email'], FILTER_VALIDATE_EMAIL), since the email is already in the database and you should be using prepared queries that are resistent against sql injection through any user input. FILTER_VALIDATE_EMAIL lets through a lot more than you expect, and if you are relying on that function to prevent sql injection, you might be vulnerable. Since you didn't share that part of the code I cannot comment on if your code is vulnerable in that regard.

The second argument of password_verify($_POST['password'], Password field in db) is a hash, not a database field name. If you meant that you retrieve it from the database, it is fine.

You do not store who is logged in and instead only set if someone is logged in $_SESSION['loggedin'] = 1;. If you do not care who is logged in, the email part of the login is redundant and you should just remove it. It does not really add any security.

All else-es in your code should be combined into one message. You do not want to give a would-be attacked any information on what part they should improve to get into your website. Just give a generic message "your email or password is incorrect".


That said, the functions that you do use do not return anything else than the expected booleans as far as I know. You correctly exit after sending your location header. I also don't see a way to set being logged in outside sending a username and password through the login page.

\$\endgroup\$
4
  • \$\begingroup\$ "A second quirk of in_array is that your check will return true with the following array due to type conversion: [ 'remember_me' => 0 ]." Brilliant, I love these weakly typed languages; you can test forever, nothing turns up and then an iteration later you add a variable that has value 0 some time in the future and bang, then the bug turns up. \$\endgroup\$ – Maarten Bodewes Apr 10 at 13:14
  • \$\begingroup\$ It is fixable by passing true as the third argument \$\endgroup\$ – Sumurai8 Apr 10 at 14:11
  • 1
    \$\begingroup\$ Optional boolean parameters to the rescue :|. Well, I suppose it is better than the weak typing problem. I like your fix better; parameters may well be 0 or empty, just check the ones you need to check. \$\endgroup\$ – Maarten Bodewes Apr 10 at 14:27
  • \$\begingroup\$ @Sumurai8 Thank you very much for your thoughtful reply! \$\endgroup\$ – test Apr 22 at 17:26

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.