1
\$\begingroup\$

I'm building this website with PDO and Pattern MVC. I'm currently working on the login page and would like to know if this structure is correct for MVC pattern. I'd also like your opinions and advice on this.

For my login view I have:

<form action="test.php" name="loginform" method="post">
    <input type="text" name="login">
    <input type="password" name="password">
    <input type="submit" name="login_submit" value="Se connecter">
</form>

For my login controller I have:

if(isset($_POST['login_submit'])){
    if(!empty($_POST['login']) && !empty($_POST['password'])){
        $auth = new auth();
        $auth->setLogin($_POST['login']);
        $auth->setPassword(md5(sha1($_POST['password'])));
        if(($row = $auth->login('u_login,u_email,u_id_level', 'users', 'u_login', 'u_password'))){                 
            $_SESSION['back_office'] = array(
                'login' => $row->u_login,
                'email' => $row->u_email,
                'level' => $row->u_id_level
            );
        }else{
            message::showError('Compte non reconnu');
        }
    }else{
        message::showError('Veuillez remplir tous les champs');
    }
}

For my login model I have:

class auth{
    protected $rowUser;
    protected $login;
    protected $password;
    protected $email;

    public function setLogin($login){
        $this->login = $login;
    }
    public function setPassword($password){
        $this->password = $password;
    }
    /**
     * @return array $row
     */
    public function login($fields, $table, $col_login, $col_password){
        $query = Db::getInstance()->prepare('SELECT '.$fields.' FROM '.$table.' WHERE '.$col_login.' = :login AND '.$col_password.' = :password');
        $query->bindValue(':login',$this->login,PDO::PARAM_STR);
        $query->bindValue(':password',$this->password,PDO::PARAM_STR);
        $query->execute();
        if($query->rowCount() > 0){
            $row = $query->fetch();
            return $row;
        }else{
            return false;
        }
        $query->closeCursor();
    }    
}
\$\endgroup\$
-1
\$\begingroup\$

Just a minor note: I'd reverse some conditions and use guard clauses to make the code flatten.

if(empty($_POST['login']) || empty($_POST['password'])){
    message::showError('Veuillez remplir tous les champs');
    return;
}

$auth = new auth();
$auth->setLogin($_POST['login']);
$auth->setPassword(md5(sha1($_POST['password'])));
if(!($row = $auth->login('u_login,u_email,u_id_level', 'users', 'u_login', 'u_password'))){                 
    message::showError('Compte non reconnu');
    return;
}
$_SESSION['back_office'] = array(
    'login' => $row->u_login,
    'email' => $row->u_email,
    'level' => $row->u_id_level
);

(You might need to extract this out to a function.)

References:

  • Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code by Martin Fowler;
  • Flattening Arrow Code
\$\endgroup\$
  • \$\begingroup\$ don't use return; inside the if statement. This makes it really hard to debug code. For instance the message class could decide that the code should continue executing. But it can't because of the return. Now the application logic is partly handling the error. This is bad design. Let the message class decide what to do and end with die(); \$\endgroup\$ – Pinoniq Jul 22 '13 at 15:10
  • \$\begingroup\$ Also, you application logic should know how the password is encrypted. This should happen inside the auth class. Because know if the encryption is changed, you will have to change all occurences to the setPassword call. Also the setPAssword method name says that you should be setting the password. But in face you are setting a PasswordHash. This makes it really crap to debug in a few month time. More info: thc.org/root/phun/unmaintain.html \$\endgroup\$ – Pinoniq Jul 22 '13 at 15:12
  • \$\begingroup\$ @Pinoniq: Have you noticed the "You might need to extract this out to a function." note? I agree with the second comment (I haven't noticed that) but I guess it should be a comment on the original code. \$\endgroup\$ – palacsint Jul 22 '13 at 16:28
1
\$\begingroup\$

A part of code in auth::login() of method you should move to other class, for example 'db'. Because you class auth has too much responsibility. For example if you want to have dynamic define fields in class auth:

Db class:

class db {
    public function getItem($table, $fields = '*', $conditions = array()) {
        // you code ...
        return $row;
    }
}

Your model:

class auth {
    private $table = 'users';
    private $fields = 'u_login, u_email, u_id_level';
    private $fieldLogin = 'u_login';
    private $fieldPassword = 'u_password';

    public function setTable($value) { $this->table = $value; }
    public function setFields($value) { $this->fields = $value; }
    public function setFieldLogin($value) { $this->fieldLogin = $value; }
    public function setFieldPassword($value) { $this->fieldPassword = $value; }

    public function login() {
        $db = new db();
        return $db->getItem($this->table, $this->fields, array(
            $this->fieldLogin => $this->login,
            $this->fieldPassword => $this->password
        ));
    }   
}

This code is more elastic and simpler will be change it.

\$\endgroup\$
  • \$\begingroup\$ Ok but I ut what in my controller? \$\endgroup\$ – user2584229 Jul 21 '13 at 13:55
  • \$\begingroup\$ You should also pass in the DB instance in the constructor. Read on dependency injection to know why \$\endgroup\$ – Pinoniq Jul 22 '13 at 14:43
0
\$\begingroup\$

This is just about the security:

md5(sha1()); is the worst security you can do.

Let's just say that I am a hack0r that happens to come accross a database dump of your user table. I can now start cracking your user's passwords.

Because I am such a 1337 hack0r I have multiple ways of going at your code.

There is the bruteforce way: simply trying all possibilities. But I don't think I need to explain that trying all possibilities is not that fast...

Then I can use dictionary atack, simply try all most used passwords and combinations of those passwords.

This greatly reduces the amount of time needed to crack a users password and is only held back by the time it takes to generate a hash. You use 2 hashes so it will take some time. Do that times the amount of passwords and maybe my grandson will have all passwords cracked.

BUT, don't underestimate my hack1ng skillz, for I am the mast3r. I use a rainbow table. Since you are using the exace same algorithm for all users I can create a database table with all calculated hashes in it. Then I perform a simple SELECT password FROM rainbowtable WHERE hash = :theHachedHash; BOOM!

But now the time creating a rainbow table is the one thing that is holding me back. Because there are still a lot of possibilities. But there is where you helped me out. You crate a md5 hash of an sha1 hash. Oh man, thank you! I now know that I only need to check for md5 hashes of 20-string long strings to get the sha1 string. And then I only need a rainbow table for password to sha1 (google will help me there). So by hashing you password twice you have succesfully helped me crack your passwords by using 2 rainbow tables. 1 for md5 -> 20string long string (that one will take a couple of minutes to create on a cheap laptop) and one of the many rainbow tables for sha1 you can find in google.

But that is not is, you have not only choosen to fall for the trap of hashing a password twice, you are actually using hashing algorithms that are weak and have been cracked many years ago. So to say: your password hashing algorithm is a laugh and will only work against script kidd0s, but they won't even get to the database in the first place...

So, as we saw above, with a technique called Rainbowtables I can easily hack your users password. You could upgrade the security by using a stronger hashing algorithm that takes more time to create. But as computers get stronger, you would always have to upgrade your algorithm.

But don't cry! there is a solution. It's called SALT. Instead of somply passing a password into a hashing algorithm, we first append a unique random string to the password. Then we hash it in a strong algorithm. Now the atacker will have to create a rainbow table for every user. Even is the users have the same passwords. passwordRandomString1 has a different hash then passwordRandomString2

But now we have two things to secure, the unique SALT has to be absolutely random, so for instance time() will not suffice since it time based and that is not random. We could use the built in random generator of php. But this one uses time() to generate random numbers. So it is good enough for application logic, but not for security.

But lucky for us, some really smart people did all the work for us. We have some very nice libraries for all languages. pkbf2 being one of them with a native implementation. Or even better crypt using the blowfish algorithm.

\$\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.