php login form - security basics

Question

again a simple but not very obvious question from me. This time is login in PHP and the session.

AS far as I understand the security side of it:

checking all the variables before sending to MySQL
using $_POST for hiding info
and logging the sessions

Well I have some code 'learned/made' but if you could spot out things that I am missing with a little explanation I would very much appreciate and also could be useful for many beginners like me. (I have read so many questions about it but most of the time the answer started - Despite the fact the code is disastrous in many ways, the specific answer to...)

So here there are:

index.php

<html>
<head>
<title>Website</title>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
</head>

<body>
<ul><li><a href="index.php">HOME</a></li><li><a href="menu1.php">menu1</a></li><li><a href="logout.php">logout</a></li></ul>
</body>
</html>

session.php:

<?php 
session_start(); 
if (!isset($_SESSION["txtUserId"])) { 
require "login.php"; 
exit; 
}

login.php

require_once('db_connect.php');

$errorMessage = '';
if (isset($_POST['txtUserId']) && isset($_POST['txtPassword'])) {
 // check if the user id and password combination is correct
 $random = '$%hgy5djk3tgbG^bhk';;
 $logname=htmlspecialchars($_POST['txtUserId']);
 $pass=sha1(($_POST['txtPassword']).$random)

 $sql = "SELECT user, pass FROM users WHERE username= :login";
 $stmt = $db->prepare($sql);

  $stmt->bindvalue( ':login', $logname);
  $stmt->execute();
 if $stmt['pass']==$pass {

   // set the session
   $_SESSION['basic_is_logged_in'] = true;
   header('Location: main.php');
  exit;
 }
else {
 $errorMessage = 'Sorry, wrong user id / password';
 require "login.php"; 
 } 
}
?>
 <html>
 <head>
 <title>Login ...</title>
 <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
 </head>

 <body>
 <?php
 if ($errorMessage != '') {
  ?>
<p align="center"><strong><font color="#990000"><?php echo $errorMessage; ?></font></strong></p>
<?php
}
?>
<form method="post" name="frmLogin" id="frmLogin">
<table width="400" border="1" align="center" cellpadding="2" cellspacing="2">
<tr>
<td width="150">User Id</td>
<td><input name="txtUserId" type="text" id="txtUserId"></td>
</tr>
<tr>
<td width="150">Password</td>
<td><input name="txtPassword" type="password" id="txtPassword"></td>
</tr>
<tr>
<td width="150">&nbsp;</td>
<td><input type="submit" name="btnLogin" value="Login"></td>
</tr>  
</table>
</form>
</body>
</html>

db_connect.php:

<?php
$hostname = "localhost";
$username = "name";
$password = "pass";
 try {
    $pdo = new PDO("mysql:host=$hostname; dbname=dbnamehere", $username, $password);
//echo "Connected to database"; // check for connection
    }
catch(PDOException $e)
    {
    echo $e->getMessage();
    }
?>

Only thing I would suggest is to put the error message in a session variable and redirect back to login.php instead of using require "login.php". — Rusty Fausak
– Rusty Fausak, Commented Sep 22, 2011 at 21:39

JM4 · Accepted Answer · 2011-09-22 22:18:45Z

2

For starters, your code will run an infinite and broken loop if a login error occurs. You require a file from within itself which would not work. I am impressed you took the time however to properly understand some security basics and have implemented PDO into your DB calls and properly escaping your user input with prepare statements.

edit

I am assuming your login page actually exists on index.php. As opposed to 'echoing' statements from your login.php script, I would allow the entire script to process, capturing any errors in a known $_SESSION variable, perhaps:

$_SESSION['login_valid_error'] = "some text".

At the end of the script,

if(isset($_SESSION['login_valid_error']) && !empty($_SESSION['login_valid_error'])) {
    header(...) //take the person to logged in page
} else {
   header('Location: index.php');
}

then back on index.php, just above your form create an error notice section (with some css) which if page is loaded and the variable exists, echo it:

<?php
if(isset($_SESSION['login_valid_error'])) { 
  echo '<div class="error">'. $_SESSION['login_valid_error'] .'</div>'; 
  unset($_SESSION['login_valid_error']);
}
?>

making sure to unset the variable once the page is loaded (any new attempts on the page will collect new errors.

I would also consider using something like phpass link is here. It may seem a bit overwhelming at first but download the code and look at the samples. It is actually very easy to integrate and will allow you to skip worry about salting/hashing your user input for things like passwords.

edited Sep 22, 2011 at 22:18

answered Sep 22, 2011 at 21:52

JM4

6,80819 gold badges80 silver badges130 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

TryHarder Over a year ago

any help with it would be appreciated if you could show me some hint

JM4 Over a year ago

@AndrasSebestyen - new edit has been posted above. This is merely a suggestion but you can see how it would help in recognizing errors and displaying to the user. Note: not all errors should be displayed to the user in the first place.

JM4 Over a year ago

@AndrasSebestyen - did you decide you didn't like either of these answers or what did you end up doing?

Ted · Accepted Answer · 2011-09-22 22:14:36Z

1

The $_POST variable only hides the data from the user, but it doesn't add any security other than obscurity.

Security considerations should include (at least) a unique token in your form (all forms!) to prevent cross site scripting, validating the token, encrypting the request to the login form (at least), encrypting and salting (unique, unstored salt) your user passwords, as well as validating and sanitizing all user entered-data.

I recommend using a well supported MVC framework, which will handle much of the security for you.

Here are some videos of exactly what you are trying to do, using the CodeIgniter MVC framework:

And the Hello World and Blog examples:

http://codeigniter.com/tutorials/

edited Sep 22, 2011 at 22:14

answered Sep 22, 2011 at 21:51

Ted

2,2492 gold badges19 silver badges28 bronze badges

4 Comments

TryHarder Over a year ago

thanks for your comment, unfortunately I have only a shared host for the site and could not setup a test site, unless I do a subdomain and I could not setup a MVC (lack of knowledge)

Ted Over a year ago

You can install an MVC framework on shared hosting (it is just php code) and it takes no special knowledge. Just install one and watch the tutorials. I worry you might be getting in over your head and your users' data might get compromised. But all I can do is include the following warning: If you don't use a framework, you have to pay more attention to security and that is hard and complex, even for experienced developers. Please, for the sake of your users, pay attention to all of the security considerations I listed.

TryHarder Over a year ago

thanks very much for your answer and do understand your concern, however I haven't find a beginner tutorial/example how to install MVC on shared host under a subdomain. If you could advice me with a link or in an other post I would be really greatful :)

Ted Over a year ago

Here are two great videos for beginners on CodeIgniter: codeigniter.com/tutorials

Collectives™ on Stack Overflow

php login form - security basics

2 Answers 2

edit

3 Comments

4 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

edit

3 Comments

4 Comments

Your Answer

Sign up or log in

Post as a guest

Related