8
\$\begingroup\$

I have some small applications that I want to secure. I've been using the following setup that I think is fairly safe, but I've never been able to set my mind at ease that it really is. Could you give me some reviews on the security of this? It doesn't need super-security like credit card data, but I suppose secure is secure.

Cookie-based Sessions. User table is:

  • Username field (cleartext)
  • Random/unique salt field (created with mt_rand() at signup
  • Password field (SHA256 hash)
  • (among other stuff)

Login method takes username, looks for the DB row, gets the salt, adds it to the end of the posted password, calcs a SHA256 hash for that string, and compares that to the password field in the DB.

auth.php include at beginning of app

<?php
session_start();
if(!isset($_GET['logout']) && isset($_SESSION['user']) && $_SESSION['ipadd'] == $_SERVER['REMOTE_ADDR']) {
    // currently logged in
        // setup data 
    require_once('lib/functions.php');
    $db = new ezSQL_sqlite('./','main.db');
    $user = new user($db,$_SESSION['user']);
    return;

}


// build login form

$head = '<html><head><title>Please login</title>
<style type="text/css">h2 { margin-top: 75px; margin-left: 100px; }</style>
</head><body>';

$form = '<form method="post" action="'.$_SERVER['PHP_SELF'].'" id="userLogin">
    <table><tr>
    <td>UserName:</td><td><input type="text" name="username" value="'.$_POST['username'].'"></td>
    </tr><tr>
    <td>Password:</td><td><input type="password" name="pass" /></td>
    </tr><tr>
    <td>Remember me on this computer</td>
    <td><input type="checkbox" name="remember" value="1" /></td>
    </tr><tr>
    <td>&nbsp;</td>
    <td><input type="hidden" name="loggingin" value="true" />
    <input type="submit" value="login" />
    </td></tr></table>
    </form></body></html>';

$msg[1] = '<h2>You\'ve logged out.</h2>';
$msg[3] = '<h2>That username and password didn\'t match anything on record.</h2>';
$msg[4] = '<h2>You must login to use this application</h2>';


// used logout button
if($_GET['logout'] == 'true') {
    setcookie('SaveMe','',time()-3600);
    session_unset();
    die($head.$msg[1].$form);


// trying to login from form or returning with 'save me' cookie
} elseif ($_POST['loggingin'] == 'true' || isset($_COOKIE['SaveMe'])) {

    require_once('lib/functions.php');
    $db = new ezSQL_sqlite('./','main.db');
$loginName = (isset($_POST['username'])) ? $_POST['username'] : $_COOKIE['worshipSaveMe'];

    // ADDED: escaping of posted/cookie data;
$loginName = mysql_real_escape_string($loginName);


        // try to create new user object, error on fail
    try {
        $user = new user($db,$_POST['username']);
    } catch (Exception $e) {
        die($head.'<h2>'.$e->getMessage().'</h2>'.$form);
    }


        // try to login with user object, die on fail
    if( ! $user->login($_POST['pass'])) 
        die($head.$msg[3].$form);
    else {
                // if remember me box was checked
        if($_POST['remember'] == 1)
            setcookie('worshipSaveMe',$_POST['username'],time()+60*60*24*365); 
        return;
    }

// no post data, no save me cookie, just got here
} else { 
    die($head.$msg[4].$form);
}

And the relevant part of the user class:

class user
{
    private $username;
    protected $ID;
    private $created;
    private $salt;
    private $password;
    private $db;

    function __construct ( ezSQL_sqlite $db, $postedUsername ) {

        if( !$user = $db->get_row("SELECT * FROM users WHERE uname = '$postedUsername';"))
            throw new Exception ('That username didn\'t match anything on record.');
        else {
            $this->db = $db;
            $this->username = $postedUsername;
            $this->ID = $user->user_id;
            $this->created = date('Y-m-d',$user->createdDate);
            $this->salt = $user->salt;
            $this->password = $user->pword;
        }

    }

    /** check password for match
     * @param user input from a posted form
     * @return boolean 
     */
    private function verifyPassword($postedPass) {

        $pHash = hash('sha256',$postedPass . $this->salt);

        if($this->password != $pHash)
            return FALSE;
        else
            return TRUE;
    }

    /** This relies on cookie based sessions, and so 
     * must be called before any output to browser.
     * @return bool
     */
    public function login($postedPassword) {
        if( ! $this->verifyPassword($postedPassword) ) {
            return FALSE;
        } else {
            if( isset( $_COOKIE["PHPSESSID"] )) { //be sure session was initialized
                $_SESSION['user'] = $this->username;
                $_SESSION['ipadd'] = $_SERVER['REMOTE_ADDR'];
                return TRUE;
            } else {
                die('Session must be initialized before calling login method.');
            }

        }
    }
\$\endgroup\$
3
  • \$\begingroup\$ This should be relevant: How To Safely Store A Password (bcrypt) \$\endgroup\$
    – andrew-e
    Feb 16, 2011 at 19:35
  • \$\begingroup\$ Ah, interesting. So can I just sub out my hash('sha256',$password) function with crypt($password,'$2a$12$1234567890123456789012$') (obviously with a different salt string). \$\endgroup\$
    – JakeParis
    Feb 17, 2011 at 15:02
  • \$\begingroup\$ crypt() might fall back to MD5 or something else if there is no support for blowfish, or if the salt isn't right. us.php.net/crypt To be sure, you could try phpass with PHP 5.3.0+ or the Suhosin patch: openwall.com/phpass \$\endgroup\$
    – andrew-e
    Feb 17, 2011 at 16:26

5 Answers 5

6
\$\begingroup\$

I see you check for sql injection of the loginname:

$loginName = mysql_real_escape_string($loginName);

Do you filter bad content for the submitted password?

Now that I look at it, you are sending the POSTed login name straight to the SQL, aren't you?

This:

new user($db,$_POST['username']);

Should be this:

new user($db,$loginName); 

If I'm reading this correctly - you need to sanitize the password also!

This is generally very important. You are also using MySQL functions to sanitize SQLite data. That's probably okay, but I wouldn't bet the farm on it!

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Be sure that you have communicated the locale to the MySQL function - otherwise you could still be SQL injected. \$\endgroup\$
    – Demi
    Dec 15, 2013 at 10:36
5
\$\begingroup\$

Don't ever pass data directly into a query. You are just opening yourself to SQL Injection. Always use bind variables when you can.

\$\endgroup\$
1
\$\begingroup\$

Assuming that you also have user authenticated application behavior check against the open session before it executes, your security process appears reasonably solid. I would personally feel a little uncomfortable storing my salt in the user's cookie - though excluding it in your setup may prove problematic if each user has a unique string. From your description, it sounds like you are already retrieving the user's salt from the db during authentication and not from the cookie. Lastly, your variable $postedUsername kinda has me worried. You are escaping or removing unsafe (for MySQL) characters before your query, aren't you?

\$\endgroup\$
2
  • \$\begingroup\$ The salt is not in the cookie, it's in the db. The only thing stored in the cookie is the username. But as far as the unsafe characters, I guess there's a weak point. Is using mysql_real_escape_string () good enough for this purpose? I've added that to the code. \$\endgroup\$
    – JakeParis
    Feb 17, 2011 at 14:53
  • \$\begingroup\$ Right on. Beside keeping your database input clean (escaping characters) you script looks secure. Depending on how much time you have, mysql_real_escape_string() should do the trick. I know it's not solid either, but I also often like to implement some javascript controls on the input fields to keep apostrophes and quotation marks from being entered altogether (along with some other characters). It's not technically any more secure - but I figure while I'm at it... \$\endgroup\$
    – 65Fbef05
    Feb 17, 2011 at 23:49
1
\$\begingroup\$

You're using a decent hash, with a unique salt per user. So it's pretty solid. You might consider adding some password stretching, but it is by no means necessary.

Step 1: Hash the password and salt.
Step 2: Add the salt to the hash, and rehash
Step 3: Repeat step 2 x number of times.
\$\endgroup\$
1
  • \$\begingroup\$ One should not do these steps manually but use a predefined password hashing instead, e.g. bcrypt. \$\endgroup\$ Dec 1, 2016 at 3:47
0
\$\begingroup\$

Your code is crap.

It contains the two most popular security holes, cross-site scripting and SQL injection. Read about them, learn them by heart and then try again.

You may have heard somewhere that you should use mysql_real_escape_string, but you didn't get the context right. This function must only be used for MySQL databases, not for SQLite.

Also, your password hashing function is too fast. Use bcrypt or PHP's CRYPT_SHA512 instead. With all your other security holes it's easy for a hacker to extract the password database, and after that, cracking the passwords will be really fast.

\$\endgroup\$

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.