Jump to content



Photo

Passwords, Unique Salts, Session Hijack And User Auth Strenght

is this secure

  • Please log in to reply
8 replies to this topic

#1 Antonio Conte

Antonio Conte

    Advanced Member

  • Members
  • PipPipPip
  • 1,070 posts
  • LocationOslo, Norway

Posted 2 May 2012 - 9:31 AM

Hey, everyone

Coding a small application with user authentication. Never done this before as I've not been secure enough regarding my abilities to create something secure.

Regarding password security:
My passwords are saved using double hashing. First, the users's email is hashed with the sha256 algorithm together with a site-wide salt using the function below. I then use the same hashing function to hash the user's password together with the unique salt. That means that password will differ for each user even if they use the same password.

Don't mind the functionality of $mysqli->query(), etc. I use my own class.

public function login( $email, $password )
{        

    // Get DB connection
    $mysqli = new Database();
    $mysqli->escape($email);
    $user = $mysqli->query("SELECT * FROM bet_users WHERE email = '$email' LIMIT 1"); // Get user from DB
            
    // Make sure we found a user ( An array )
    if ( is_array($user) )
    {
        // Get special hash
        $hash = $this->hash($email, HASH_SALT); // Site wide salt
        
        // Check hashed passwords
        if ( $user['password'] == $this->hash($password, $hash) ) // Hash $password with unique salt
        {
            
            // Prevent session hijack
            Util::validate_session($email);
            
            // Set user details
            $this->set_user_details($user);
            
            // User is logged in
            return true;
        }
        // Wrong password
        return false;
    }

}

private function hash( $input, $salt )
{        
    // Initialize an incremental hashing context
    $hashed = hash_init('sha256', HASH_HMAC, $salt);
        
    // Set active hashing context
    hash_update($hashed, $input);
        
    // Return hashed password
    return hash_final($hashed);
}

Regarding login checks:
I only add $_SESSION['admin'] to the session array if the queried user has admin status in the DB. My checks looks like this and uses the session hijacking check below. I use these function like Util::is_logged_in() and Util::is_admin().

public static function is_logged_in()
{
    return isset($_SESSION['user_id']) && self::validate_session($_SESSION['email']);
}

public static function is_admin()
{
    return self::is_logged_in() && isset($_SESSION['admin']) && $_SESSION['admin'] == true;
}

Here's the function to prevent session hijack:

public static function validate_session( $email = null )
{

    // Set hashed http user agent
    $agent = md5($_SERVER['HTTP_USER_AGENT'].$email);

    // Check for instance
    if ( isset($_SESSION['initiated']) == false || isset($_SESSION['HTTP_USER_AGENT']) == false )
    {
        // Create new id
        session_regenerate_id(TRUE);
        $_SESSION = array();
        $_SESSION['initiated'] = true;
            
        // Set hashed http user agent
        $_SESSION['HTTP_USER_AGENT'] = $agent;
    }

    if ( isset($_SESSION['initiated']) && isset($_SESSION['HTTP_USER_AGENT']) )
    {
        // Validate the agent and initiated
        if ( ($_SESSION['HTTP_USER_AGENT'] == $agent) && $_SESSION['initiated'] )
        {
            return true;
        }
        else
        {
            // Destroy session
            session_destroy();

            return false;
        }
    }

    return false;

}

How would you say the security is here? Is the security good? Any improvements I can make? Thanks for any answers. :)
  • 1

#2 Paul Swanson

Paul Swanson

    Excellent Advisor

  • Members
  • PipPipPip
  • 163 posts
  • LocationPortland, OR, USA

Posted 2 May 2012 - 10:39 AM

Not a comment on the security, but I question whether using an email address to hash a password is a good idea. What if the user wants to change their email address? I would use something that won't change, like the timestamp from when they registered, or the username.
  • 1

#3 Edward

Edward

    Advanced Member

  • Members
  • PipPipPip
  • 1,134 posts

Posted 2 May 2012 - 11:16 AM

Have you ever heard the saying "Why put 10 locks on the door, when the keys are under the doormat?"
  • 1

#4 Antonio Conte

Antonio Conte

    Advanced Member

  • Members
  • PipPipPip
  • 1,070 posts
  • LocationOslo, Norway

Posted 2 May 2012 - 11:34 AM

Yes. If you are able to spot obvious security flaws, please point them out. :)

I can't really see any though. The method set_user_details(), not shown here, simply assign the db keys to the corresponding session key. It's private and not callable outside the class.

Btw: no need to quote my long post.

Edit: good idea regarding the email. The usernames are changeable, that's why I went with email. Maybe I'll just keep usernames unchanchable and use those instead. They too are unique, which is some of the point.

Good input here. Hoping for more.
  • 2

#5 Edward

Edward

    Advanced Member

  • Members
  • PipPipPip
  • 1,134 posts

Posted 2 May 2012 - 11:44 AM

I have a friend that works for a big company, he was the one that told me the thing about the locks and the keys under the doormat. He told me that there are many way to inject into scripts and also take session data etc. It means nothing much is safe, and i also spoke to him about the design patterns, he quoted that now people are anti pattern because of their use of global's. Security wise its probably best to do as much as is required proportion related to the amount of users that are actually going to be visiting the site. I mean just think about if we had a shed in our back garden with some tools in it, there is no point putting a big diamond log on it, a small one would probably do the job. I am not trying to be a smart ass but its just i have a similar project, so its best to keep things small to start with, that's all i am saying. However if your project is already big then you will need to do all you can, you need to get an advanced book on website security and encyrption.
  • 0

#6 Larry

Larry

    Administrator/Writer

  • Administrators
  • 3,962 posts
  • LocationState College, PA (USA)

Posted 2 May 2012 - 1:21 PM

Yes, Paul, good point about the email address for salt. I agree that using an unchangeable username is a better idea. Edward, pithy statements and metaphors aside, do you see any actual "key under the doormat" situations with his original code? The fact that nothing is truly safe isn't really relevant to this particular question, is it? I appreciate the desire to help, but do you have construct feedback here?
  • 1

#7 Edward

Edward

    Advanced Member

  • Members
  • PipPipPip
  • 1,134 posts

Posted 2 May 2012 - 9:38 PM

Sorry your right unless i have a full explanation, i shouldn't of bothered saying anything at all, these guys are already better than me.
  • 0

#8 Antonio Conte

Antonio Conte

    Advanced Member

  • Members
  • PipPipPip
  • 1,070 posts
  • LocationOslo, Norway

Posted 4 May 2012 - 10:18 AM

After some thoughts, do I really need the double hashing? I used the site wide hash the last time I wrote a similar class, but I felt the need for unique hashing too.

I'm thinking about conflicts here. I have often heard that double hashing, for example md5( md5('password') ), is discouraged, but how does that compare when you use the first hash as salt instead? My code is in practice doing something very similar.

// My hashing method
$hashed_password = $this->hash( $password, $this->hash($username, HASH_SALT) );

// Double md5
$hashed_password = md5( md5($password) );

Even thought the sha256 is a lot stronger than the md5 algorithm, I kind of feel I might end up weakening the hash instead of strengthening it.

My thought is to write a function that will create a new salt based on both the site wide salt and the username. A quick unpolished version from the top of my head to illustrate this:


// Create a hash based on salt.
$this->hash( $password, $this->create_new_salt( HASH_SALT, $username) );

private function create_new_salt( $salt, $input)
{

	// Get salt length
	$salt_length = strlen($salt);
	$input_length = strlen($input);

	return substring($salt, - (int)($salt_length/4)) . chr($input[0]-3) . chr($input[1]-2). chr($input[2]-1). substring($salt, 0, (int)($salt_length/4));

}

Not a very good function yet, but I feel it has potential. By switching some the input from the site salt and username, the salt will be very hard to guess. Notice that I discuss this for the sake of it, not because I think it will be unbreakable. I also know that production code will need something more robust than this... As I said. From the top of my head for the sake of discussion.

The point here being that you need the functionality of the black box to verity passwords. How it works it not very important, but the end result will be a harder to guess salt for the hash function.

Interested in your thoughts on this. :)

Edit: Random_salt was a bad name for the function. Not random, just a new construct of the old.
  • 0

#9 Edward

Edward

    Advanced Member

  • Members
  • PipPipPip
  • 1,134 posts

Posted 4 May 2012 - 10:42 AM

You could generate the salts from the email rather than the username, haha, before the '@' Semantic. You could also concatenate the username and email to create the salts. Some servers they can protect against brute force attacks on passwords, its pretty much impossible to get through with that.
  • 0