4
\$\begingroup\$

I've been doing a lot of searching on PHP, logins, forms, cookies, sessions, etc. And so, I've tried to gather all the info that I got from all over the place. But, I didn't find a place with all the info, and tried to do the best secure login I could do.

This is what I have. If this ends up being good, others can use this. Otherwise, just ignore it. But I'd like to have opinions.

First, every page makes sure you use HTTPS:

if($_SERVER["HTTPS"] != "on")
{
    header("Location: https://" . $_SERVER["HTTP_HOST"] . $_SERVER["REQUEST_URI"]);
    exit();
}

Send username and pass using POST over the https should be safe enough?

Using XMLHttpRequest to check password on Check.php:

function validate() {


var un = document.form1.myusername.value;
var pw = document.form1.mypassword.value;


  httpObject = getHTTPObject();

  if (httpObject != null)
  {
    var params = "username="+un+"&password="+pw;

    httpObject.open("POST","Check.php", true);
    httpObject.setRequestHeader("Content-type","application/x-www-form-urlencoded");
    httpObject.setRequestHeader("Content-length",params.length);
    httpObject.setRequestHeader("Connection","close");
    httpObject.onreadystatechange = setValid;
    httpObject.send(params);
  }

}

On Check.php, upon confirming password (which was saved on server encrypted), I set up cookie and session creating unique IDs and session secrets:

if($u==$username && $p==$password)
    {
        $valid=1;

        //Setting up session and cookie
        //if cookie stored secret does not meet session secret, session has been hijacked, the same for id
        //session is stored on server while cookie is stored on client

        session_start();
        $sessionTime = time();
        $unique_id = uniqid(sha1($_SERVER['HTTP_USER_AGENT'] . $username), true); //create a unique ID
        $sessionSecret = sha1($sessionTime . $unique_id);   // Create a unique secret based on time and ID

        $_SESSION[$unique_id]['time']    = $sessionTime;
        $_SESSION[$unique_id]['user']    = $us;
        $_SESSION[$unique_id]['secret']    = $sessionSecret;

        // if user changes cookie Secret, it wont match any secret of any other session stored in server
        setcookie("LoginCookie_s", $sessionSecret);
        setcookie("LoginCookie_i", $unique_id);

        // inform user that login was successfull
        echo $valid;
        return;
    }

Back in login: if the return value is 1, I can proceed to main page. On the main page and every other pages I validate cookie and session:

<?php
 // Validate session and cookie
//if cookie stored secret does not meet session secret, session has been hijacked, the same for id
//session is stored on server while cookie is stored on client

//is cookie set?
if(!isset($_COOKIE["LoginCookie_s"]) || !isset($_COOKIE["LoginCookie_i"]))
{
    //this guy did not pass by login!
    header("location:Login.php");
    exit();
}

session_start();

// was session started on login?
if(!isset( $_SESSION[$_COOKIE["LoginCookie_i"]] ))
{
    //this guy did not pass by login!
    header("location:Login.php");
    exit();
}

// even if a user forged an id matching another user's id he would need to change cookie secret
// if user changes cookie id or secret, it wont match the session secret of that other user id
if( $_COOKIE["LoginCookie_s"] != $_SESSION[$_COOKIE["LoginCookie_i"]]['secret'] )
{
    //cookie and\or session has been hijacked
    header("location:Login.php");
    exit();
}

//all is fine, proceed loading the rest of the page...
 ?>

Am I missing something?

\$\endgroup\$
  • \$\begingroup\$ You might want to read this article: phpsec.org/projects/guide/4.html and place a session_destroy / session_regenerate_id in your Check.php code. \$\endgroup\$ – jeff Sep 8 '13 at 19:19
  • \$\begingroup\$ Why are you reinventing the wheel? Use OpenId. \$\endgroup\$ – jmoreno Sep 8 '13 at 19:36
  • 3
    \$\begingroup\$ By encrypted password you mean hashed, right? And by hashed, you would mean either scrypt or bcrypt right? :) \$\endgroup\$ – Jack Sep 10 '13 at 8:34
3
\$\begingroup\$

Some comments:

if($_SERVER["HTTPS"] != "on")

See: https://stackoverflow.com/a/2886224/59087

Avoid calling exit() from more than one location.

The following code is repeated many times:

//this guy did not pass by login!
header("location:Login.php");
exit();

You could write some functions to avoid the duplication:

function redirectLogin() {
  redirect( "Login.php" );
}

function redirect( $page ) {
  header( "Location: $page" );
  exit();
}

This removes the duplication and ensures that you have only one place in the code to change if you wanted to rename Login.php, for example, to login.php, which is a bit more user-friendly (on Unix systems).

\$\endgroup\$

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.