Security issue

[Nuranto]

Hi,

I think MASTER_PASSWORD should be always stored hashed. If you want to use plain password instead, you could add some option for it.

But as it is, you cannot use hashed password. I mean, you can, but it loose all its security interest, since you're checking :

$isCorrect = ($plain === $masterPass) || $this->hasher->check($plain, $masterPass);

So you will be logged in if you enter the hashed password...

Should be something like this :

$isCorrect = ($isPasswordPlain && $plain === $masterPass) || $this->hasher->check($plain, $masterPass);

(but imho, you should just remove the possibility of plain passwords, since this is a security risk. Maybe you can add an artisan hash:make command instead.)

[Nuranto]

(sorry just read about the security issue policy.... However, that's a low risk here, which suppose that the hacker already broke in... But feel free to delete the issue.)

[imanghafoori1]

I like the way you think about it. I will investigate it.

But I think finding the real password is much much easier to the attacker rather that to try to use the hashed version. so we are not making anything easier for the brute force attacker. plus, the speed of attack is not any faster, since we are also checking for the hash each time.

[Nuranto]

@imanghafoori1 I'm not talking about brute force here. The goal of hashing is not linked to brute force either. Using hashed password prevent us to store (in file or database, whatever) in plain text. If via some other security breach, an hacker obtain read access to your file system or database, if the password is plain, he can use it to login to any account. If the password is hashed, he can not (unless he cracks it, but that's another debate ^^). Well he should not... But currently, he can, that's why I created this issue.

[imanghafoori1]

hmm... you mean it is better to force to use a hashed version of passwords ?

Plain passwords are meant to be used in local development only for ease of use. Not production. We assumed that the developer knows not to use plain text for real production servers.

[Nuranto]

Yes I think it is better to force to use a hashed version of password. But the main issue is not there.

The issue is that even if you use a hashed password in production, it will be as if you were using a plain one. Because of this line :

$isCorrect = ($plain === $masterPass) || $this->hasher->check($plain, $masterPass);

Ok, let's have your doc's example :

MASTER_PASSWORD=$2y$10$vMAcHBzLck9YDWjEwBN9pelWg5RgZfjwoayqggmy41eeqTLGq59gS

So if you login with mySecretMasterPass you will be logged in because of $this->hasher->check($plain, $masterPass) returning true. Ok, great. But if you login with $2y$10$vMAcHBzLck9YDWjEwBN9pelWg5RgZfjwoayqggmy41eeqTLGq59gS you will also be logged in because of $plain === $masterPass returning true, and that's a security issue.

[imanghafoori1]

Got it. I fixed for now by checking master pass length not to be 60.

in future versions I will provide more precise way to detect whether the master pass is hashed or plain.

thank you for remaining the issue.

[Nuranto]

thanks !