defuse / php-encryption

Simple Encryption in PHP.
MIT License
3.79k stars 306 forks source link

Does “KeyProtectedByPassword” weaken password hashing otherwise done with “bcrypt”? #426

Closed ocram closed 5 years ago

ocram commented 6 years ago

In a common use case, PHP’s password_hash function is employed to hash the user’s login password, e.g. with the current default options of “bcrypt” as the algorithm and 10 as the cost or work factor.

Now, as per the second scenario of your excellent tutorial in docs/Tutorial.md, arbitrary messages and content shall be encrypted using that same login password – using this library here.

To do that, as described in your tutorial, a random key is generated and protected with the user’s login password upon registration:

$myProtectedKey = KeyProtectedByPassword::createRandomPasswordProtectedKey($myPassword);
$myProtectedKeyAscii = $myProtectedKey->saveToAsciiSafeString();
// store $myProtectedKeyAscii

Upon every login, the key is loaded again and unlocked using the password:

// retrieve $myProtectedKeyAscii
$myProtectedKey = KeyProtectedByPassword::loadFromAsciiSafeString($myProtectedKeyAscii);
$myKey = $myProtectedKey->unlockKey($myPassword);
// use $myKey

Internally in this library, it seems, the KeyProtectedByPassword is generated and the password is protected as follows:

Crypto::encryptWithPassword(
    Key::createNewRandomKey()->saveToAsciiSafeString(),
    \hash('sha256', $password, true),
    true
);

Now if an attacker got access to the protected keys (i.e. $myProtectedKeyAscii) and also to the separately hashed passwords, both stored in the database, what would be the faster way to crack the passwords and expose the user’s original login passwords – targeting the protected keys (or even messages encrypted with those) or the hashed passwords?

In other words, does KeyProtectedByPassword introduce another target for the attacker which is easier to attack and yield the user’s login password than attacking the “bcrypt” password hashes?

As we know, “bcrypt” has explicitly been designed for the purpose of protecting passwords. But if the attacker could instead attempt to call KeyProtectedByPassword#unlockKey with varying guesses for the password until the key “opens”, this would circumvent the password hashing done separately and reduce the target from bcrypt(password) to sha256(password) (?).

Regarding context separation, you mentioned slides which say:

Reusing a secret key for different purposes can have catastrophic implications.

http://archive.hack.lu/2017/hacklu-crypto-api.pdf

Further, you mention the following:

WARNING: Because of the way KeyProtectedByPassword is implemented, knowing SHA256($password) is enough to decrypt a KeyProtectedByPassword. To be secure, your application MUST NOT EVER compute SHA256($password) and use or store it for any reason. You must also make sure that other libraries your application is using don't compute it either.

docs/Tutorial.md and docs/classes/KeyProtectedByPassword.md

So is all this a valid concern?

Or do the IVs (or something else) prevent KeyProtectedByPassword from being an easier target for password cracking?

paragonie-scott commented 6 years ago

In other words, does KeyProtectedByPassword introduce another target for the attacker which is easier to attack and yield the user’s login password than attacking the “bcrypt” password hashes?

As we know, “bcrypt” has explicitly been designed for the purpose of protecting passwords. But if the attacker could instead attempt to call KeyProtectedByPassword#unlockKey with varying guesses for the password until the key “opens”, this would circumvent the password hashing done separately and reduce the target from bcrypt(password) to sha256(password) (?).

Regarding context separation, you mentioned slides which say:

Reusing a secret key for different purposes can have catastrophic implications. – http://archive.hack.lu/2017/hacklu-crypto-api.pdf

Further, you mention the following:

WARNING: Because of the way KeyProtectedByPassword is implemented, knowing SHA256($password) is enough to decrypt a KeyProtectedByPassword. To be secure, your application MUST NOT EVER compute SHA256($password) and use or store it for any reason. You must also make sure that other libraries your application is using don't compute it either. – docs/Tutorial.md and docs/classes/KeyProtectedByPassword.md

So is all this a valid concern?

Or do the IVs (or something else) prevent KeyProtectedByPassword from being an easier target for password cracking?

So here's the issue: In order to side-step DoS attacks, the password is prehashed with SHA256, since encryptWithPassword() used PBKDF2 to expand the password into a key. You don't want to disclose the SHA256 hash of your plaintext password anywhere since the SHA256 hash itself can be used with decryptWithPassword(). That's the limited scope of the relevant cross-protocol attacks.

If you, separately, have a bcrypt hash of the password, this isn't a concern. It's totally orthogonal, since the SHA256 hash of the password then goes through 100k rounds of PBKDF2-SHA256, which won't afford attackers a significantly more viable target than bcrypt.

That being said, if you can, please don't let your users reuse passwords.

ocram commented 6 years ago

Thanks, @paragonie-scott!

That being said, if you can, please don't let your users reuse passwords.

Well, they just have a single password, as with most services. It’s just that – other than the login where “bcrypt” is used – now another use case for that single password is added, which is the encryption of the key, which is a suggestion from docs/Tutorial.md. Or are you referring to something else? Re-use across services? Re-use after password change?

If you, separately, have a bcrypt hash of the password, this isn't a concern. It's totally orthogonal, since the SHA256 hash of the password then goes through 100k rounds of PBKDF2-SHA256

Does that mean, though, that the described use case changes (perhaps reduces) the cost of password cracking from bcrypt(password) to min(bcrypt(password), pbkdf2-sha256-100000(password)), assuming each of these expressions describes the costs?

paragonie-scott commented 6 years ago

Does that mean, though, that the described use case changes (perhaps reduces) the cost of password cracking from bcrypt(password) to min(bcrypt(password), pbkdf2-sha256-100000(password)), assuming each of these expressions describes the costs?

I think that's a fair assessment, assuming that the same password is used in both places.

ocram commented 6 years ago

Thanks!

Yes, the same password is used, which is what the use case is all about, as per the tutorial.

Anyway, min(bcrypt(password), pbkdf2-sha256-100000-rounds(password)) really sounds acceptable. I just wanted to make sure the cost is not just min(bcrypt(password), pbkdf2-sha256-1-round(password)), which would render the second scenario from the tutorial practically useless, as it would break the security of a separate password hashing.