3
\$\begingroup\$

I'm trying to learn PHP/MySQL and the likes, so I've been reading tutorials for PHP login systems. My current iteration is based heavily on one from this website and contains the accepted answer for random salts here. This is my first thing I've done in MySQL, and my first attempt at PHP besides a tiny Tic-Tac-Toe game.

config.php:

<?php
    //set off all error for security purposes
    error_reporting(E_ALL);


    //define some contstant
    define( "DB_DSN", "mysql:host=localhost;dbname=gryp" );
    define( "DB_USERNAME", "root" );
    define( "DB_PASSWORD", "" );
    define( "CLS_PATH", "class" );

    //include the classes
    include_once( CLS_PATH . "/user.php" );

    ?>

user.php:

<?php

     class Users {
         public $username = null;
         public $password = null;
         public $salt = null;

         public function __construct( $data = array() ) {
             if( isset( $data['username'] ) ) $this->username = mysql_real_escape_string( htmlspecialchars( strip_tags( $data['username'] ) ) );
             if( isset( $data['password'] ) ) $this->password = mysql_real_escape_string( htmlspecialchars( strip_tags( $data['password'] ) ) );

         }

         public function storeFormValues( $params ) {
            //store the parameters
            $this->__construct( $params ); 
         }

         public function userLogin() {
             $success = false;
             try{
                $con = new PDO( DB_DSN, DB_USERNAME, DB_PASSWORD ); 
                $con->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

                $sql = "SELECT * FROM users WHERE username = :username LIMIT 1";
                $fetch = $con->prepare( $sql );
                $fetch->bindValue( "username", $this->username, PDO::PARAM_STR );
                $fetch->execute();
                $row = $fetch->fetch(PDO::FETCH_ASSOC);

                if($row){
                    $this->salt=$row['salt'];
                    if ( hash("sha256", $this->password . $this->salt) == $row['password'])
                    {
                        $success = true;
                        $sql = "UPDATE users SET lastlogin=NOW() WHERE username=:username";
                        $fetch = $con->prepare($sql);
                        $fetch->bindValue( "username", $this->username, PDO::PARAM_STR );
                        $fetch->execute();
                    }
                }

                $con = null;
                return $success;
             }catch (PDOException $e) {
                 echo $e->getMessage();
                 return $success;
             }
         }

         public function register() {
            $correct = false;
                try {
                    $con = new PDO( DB_DSN, DB_USERNAME, DB_PASSWORD );
                    $con->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

                    $this->salt = $this->unique_md5();

                    $sql = "INSERT INTO users(username, password,salt,registerdate) VALUES(:username, :password,:salt,NOW())";

                    $fetch = $con->prepare( $sql );


            $fetch->bindValue( "username", $this->username, PDO::PARAM_STR );
                $fetch->bindValue( "password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR );
                $fetch->bindValue( "salt", $this->salt, PDO::PARAM_STR );
                $fetch->execute();
                return "Registration Successful <br/> <a href='index.php'>Login Now</a>";
            }catch( PDOException $e ) {
                return $e->getMessage();
            }
     }

    public function unique_md5() {
        mt_srand(microtime(true)*100000 + memory_get_usage(true));
        return md5(uniqid(mt_rand(), true));
    }

 }

?>

Table structure:

# name        type        collation       null    default extra       
1 userID      int(11)                 No  None    AUTO_INCREMENT  
2 username    varchar(50) latin1_swedish_ci   No  None        
3 password    varbinary(250)              No  None        
4 salt        varbinary(32)               No  None        
5 registerdate    datetime                No  None        
6 lastlogin   datetime                No  None

I think that my input is sanitized, that I'm safe from SQL injections, and that I'm safe from XSS attacks. But before I move on with what I'm doing and learn more, I figure that it's better to assume my code is insecure and ask for help, than to assume it is secure and find out it isn't.

I feel that

$this->salt=$stmt->fetchColumn(3); 

shouldn't be what I'm doing. Also that I have 3 queries for login, which seems wasteful. But that was the least I could do it in.

Is my code going in the right direction? What can I do better?

\$\endgroup\$
  • \$\begingroup\$ The indentation doesn't look too good, make sure to convert tabs to spaces when copy/pasting if it looks correct in your editor. \$\endgroup\$ – elclanrs Oct 9 '13 at 6:21
  • \$\begingroup\$ Use the build in PHP Functions for password hashing. php.net/manual/en/function.password-hash.php or if you don't have PHP 5.5 use the backport github.com/ircmaxell/password_compat \$\endgroup\$ – AlucardTheRipper Oct 9 '13 at 10:42
  • \$\begingroup\$ I would select only user only by username and then check password in php code. You can find some discussion about this topic at stackoverflow.com/questions/5978539/… \$\endgroup\$ – DominikM Oct 9 '13 at 11:36
  • \$\begingroup\$ elclanrs fixed a few spaces in editor, but copy/pasting here messed a bit more of it up. AlucardTheRipper I'm pretty sure mine work adequately, but I'll look into using the built in functions over built in hash. @DominikM did just that. Looks cleaner and I think its faster. Thank you. \$\endgroup\$ – Lutcikaur Oct 9 '13 at 23:17
1
\$\begingroup\$

Depending on the size of your application, it could be a good idea to abstract away the user information to a DAO (data access object). This object would contain the methods needed for handling user information.

For instance like the following:

class UserDAO {

  private $dbh;

//$dbh is the dbhandle you get from new PDO(…)
function __construct( $dbh ) {
  $this->dbh = $dbh;
}

function save( $user ) {
  if ( !isset( $user->id ) || $user->id == 0 ) {
    $this->insert( $user );
  } else {
    $this->update( $user );
  }
}

function getByUsername( $username ) {
  $stmt = $this->dbh->prepare( "SELECT * FROM user WHERE username = ?" );
  if ( !$stmt ) {
    echo 'Error in fetch query';
    die();
  }
  $stmt->setFetchMode( PDO::FETCH_CLASS, "User" );
  $stmt->execute( array( $username ) );
  $user = $stmt->fetch();
  return $user
}

private function insert( $user ) {
  $stmt = $this->dbh->prepare( "INSERT INTO
                        user (username, email, password)
                        VALUES (:username, :email, :password)" );
  if ( !$stmt ) {
    echo 'Error in saving query.';
    die();
  }
  $stmt->bindParam( ':username', $user->username );
  $stmt->bindParam( ':email', $user->email );
  $stmt->bindParam( ':password', $user->password );
  if ( !$stmt->execute() ) {
    echo 'Error saving user data. ';
    echo $stmt->errorCode();
    print_r( $stmt->errorInfo() );
    die();
  }
  $user->id = $this->dbh->lastInsertId();
}

private function update( $user ) {
  $stmt = $this->dbh->prepare( "UPDATE user
      SET username = :username,
          email = :email,
          password = :password
      WHERE
          id = :id" );
  if ( !$stmt ) {
    echo 'Error in update query';
    die();
  }
  $stmt->bindParam( ':username', $user->username );
  $stmt->bindParam( ':email', $user->email );
  $stmt->bindParam( ':password', $user->password );
  $stmt->bindParam( ':id', $user->id );
  if ( !$stmt->execute() ) {
    echo 'Error updating user';
    echo $stmt->errorCode();
    print_r( $stmt->errorInfo() );
    die();
  }

The the login code on your php page would just be:

    $dao = DAOFactory::getDAO("user");
    if ($user = $dao->getByUsername($this->username)) {
      if (Utils::checkPassword($password, $user->password)) {
        $_SESSION['username'] = $user->username;
        $user->last_login = date("Y-m-d H:i:s");
        $dao->save($user);
        header('Location: info.php');
        return;
      }
    }
    $this->errors[] = "Invalid username or password";
\$\endgroup\$
  • \$\begingroup\$ You mix PHP4 and PHP5 code. var was deprecated in PHP5 and __constructor was introduced in PHP5. Decide if you are writing PHP4 or PHP5 code and stick to that. \$\endgroup\$ – Alexandru Guzinschi Oct 17 '13 at 18:29
  • \$\begingroup\$ As of PHP 5.1.3 it is no longer deprecated. If you declare a property using var instead of one of public, protected, or private, then PHP 5 will treat the property as if it had been declared as public. But you are correct in saying that it has a PHP4 smell to it. Source \$\endgroup\$ – Andreas Finne Nov 4 '13 at 12:20
  • \$\begingroup\$ Supporting var declaration can be stopped or changed at any time and the property declared it will be public. So it is better to consider var deprecated and let it die in peace. \$\endgroup\$ – Alexandru Guzinschi Nov 11 '13 at 17:11
  • \$\begingroup\$ Edited code to change var to private as pointed out by @AlexandruG. \$\endgroup\$ – Andreas Finne Nov 13 '13 at 13:10
  • \$\begingroup\$ It is the implementer's decision what to do when an error occurs, not the library itself. So you should not interrupt the process when something has gone haywire, but throw an exception or return something, so that the implementer can decide what to do next. ( see exit from your example ) \$\endgroup\$ – Alexandru Guzinschi Dec 23 '13 at 12:21
0
\$\begingroup\$

For hashing passwords, I use the code below. Note, I need to support running on older versions of PHP, so that is why I have the different checks for available features.

static function hashPassword($pwd) {
  $salt = self::secure_rand(16);
  $salt = str_replace('+', '.', base64_encode($salt));
  if (CRYPT_BLOWFISH == 1) {
    $salt = substr($salt, 0, 22);
    $cost = "07";
    $hash = crypt($pwd, '$2a$' . $cost . '$' . $salt);
    return $hash;
  } else {
    $salt = substr($salt, 0, 8);
    $hash = crypt($pwd, '$1$' . $salt . '$');
    return $hash;
  }
}
private static function secure_rand($length) {
  if(function_exists('openssl_random_pseudo_bytes')) {
    $rnd = openssl_random_pseudo_bytes($length, $strong);
    if ($strong === TRUE)
      return $rnd;
  }
  $sha =''; $rnd ='';
  for ($i=0; $i<$length; $i++) {
    $sha = hash('md5',$sha.mt_rand());
    $char = mt_rand(0,62);
    $rnd .= chr(hexdec($sha[$char].$sha[$char+1]));
  }
  return $rnd;
}

The valid password is checked by fetching the user by username from database, and then comparing the entered password with the encrypted password using the crypt function.

static function checkPassword($pwd, $hash) {
  return $hash == crypt($pwd, $hash);
}

I have these methods in a Utils class.

\$\endgroup\$
0
\$\begingroup\$

For logging in and registering it does what it should. You may want to consider beyond the act of logging in and registering though.

  1. If this user object is something that will be used after someone has logged in to do other things, the password and salt are probably not going to be something you need to keep track of beyond the login request.
  2. Keeping item 1 in mind, I would restructure this to use a static method to do the login and registration since those are things that are typically self contained. So instead of passing the $data array into the construct you would do Users::userLogin($data)
  3. I'm not a huge fan of calling the construct from other methods. When you resort to that I think it is a sign that you need to make a change. The construct is intended to run once, if you need to use it a second time you should move the logic into it's own method. If you kept your code "as is", I would swap the construct for storeFromValues like this:

public function __construct( $data = array() ) {
   $this->storeFormValues( $data ); 
 }

 public function storeFormValues( $params ) {
 //store the parameters
     if( isset( $params['username'] ) ) $this->username = mysql_real_escape_string( htmlspecialchars( strip_tags( $params['username'] ) ) );
     if( isset( $params['password'] ) ) $this->password = mysql_real_escape_string( htmlspecialchars( strip_tags( $params['password'] ) ) );
  }
\$\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.