Problem/Motivation
Context: Wikipedia.
Proposed resolution
Use hash_hmac()
instead of hash()
for salted hashes.
Remaining tasks
User interface changes
API changes
Beta phase evaluation
Issue category | Bug, because usage of hash() is not correct. |
---|---|
Issue priority | Normal, because it is a security improvement to make getting the hash salt out of a known hash prefix easier. |
Prioritized changes | The main goal of this issue is security. |
Disruption | Not disruptive for core/contributed and custom modules/themes because it is internal to DrupalKernel only and both changed had just been introduced. |
Comment | File | Size | Author |
---|---|---|---|
#6 | 2474043-use-hmac-6.diff | 2.78 KB | znerol |
#6 | interdiff.txt | 1.02 KB | znerol |
#4 | 2474043-use-hmac-4.diff | 2.78 KB | znerol |
#4 | interdiff.txt | 1.57 KB | znerol |
#1 | 2474043-use-hmac.diff | 2.35 KB | znerol |
Comments
Comment #1
znerol CreditAttribution: znerol commentedI left two instances unchanged:
PhpassHashedPassword.php
implements a proven algorithm based a gazillion rounds of hashing. That mechanism is considered to be secure.update.module / _update_manager_unique_identifier()
because that would change how sites are registered on d.o. Also the resulting hash is truncated to 8 characters which helps preventing that thehash_salt
can be attacked.Comment #3
alexpottI think it is worth making a helper method here. To create an apc key with a namespace. Something like:
Comment #4
znerol CreditAttribution: znerol commentedComment #6
znerol CreditAttribution: znerol commentedComment #7
Fabianx CreditAttribution: Fabianx for Acquia commentedRTBC, looks great.
I shortly pondered the usage of hash_hmac() but then it looked like it was used for something else and with a prefix security was still okay ...
Thanks for fixing!
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud at Centarro commented^ Why are we even doing that? There is no security issue here, we could use straight the salt for what I care.
^ Using the hash salt sounds wrong here. It's either the private key, or the hash salt, it should really not be both at the same time (I realize we do the same thing in
drupal_get_token()
, but it is equally wrong).As the name implied, the hash salt was never meant to be super secret. It was just meant as a way to ensure that having a dump of the database by itself is not sufficient to attack a live site.
Comment #9
Fabianx CreditAttribution: Fabianx for Acquia commented#8: Can you help how this should work? What is secure, what is not?
And is there any documentation explaining this in some detail?
Comment #10
pwolanin CreditAttribution: pwolanin at Acquia commentedProbably needs a re-roll, and better issue summary
Comment #12
jhedstromPatch no longer applies, also this is tagged for an IS update.
Comment #20
larowlanI reviewed this as part of Bug Smash Initiative triaging.
#2474909: Allow Simpletest to use the same APC user cache prefix so that tests can share the classmap and other cache objects made the required changes for APC caching.
But we're still using the same
hash()
instead ofhash_hmac()
in the PermissionsHashGenerator.Question is, is this still worth doing?
@znerol thoughts?
If not, I think this can be closed as a duplicate of #2474909: Allow Simpletest to use the same APC user cache prefix so that tests can share the classmap and other cache objects
Comment #24
Kristen PolAs part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" two years ago.
Since @larowlan requested feedback in #20 yet did not receive any, I'm marking this issue "Closed (duplicate)" per that comment. Feel free to reopen this issue and provide more information if warranted.
Thanks!
Comment #25
larowlanLeaving this one open if we can, keen to hear thoughts from znerol
Comment #27
smustgrave CreditAttribution: smustgrave at Mobomo commentedRe closing since there hasn't been a follow up in a year.