Hello,

I just read https://www.drupal.org/writing-secure-code/hash-functions.

It recommands to use hash('sha256', $data) instead of sha1() which protected node uses.

Converting the function calls will not be difficult, but impossible to handle the convertion for existing datas ?

Should we keep it for a 7.x-2.x branch with the refactoring ?

CommentFileSizeAuthor
#6 protected_node-use_sha256-2326905-6.patch10.55 KBgrimreaper
#4 protected_node-use_sha256-2326905-4.patch9.95 KBgrimreaper
#2 protected_node_n2326905-2.patch7.74 KBdamienmckenna

Comments

izus’s picture

izus commented

i don't think there is a one shot and general way to do this.
http://stackoverflow.com/questions/6971316/want-to-convert-a-website-pas...

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
StatusFileSize
new protected_node_n2326905-2.patch7.74 KB

Maybe include support for both, check sha-256 first and if it fails check sha-1?

This patch needs further work, but it's a step towards it.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Active » Needs work
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
StatusFileSize
new protected_node-use_sha256-2326905-4.patch9.95 KB

Hello,

Thanks for the patch. there was just a variable which name has changed.

But Warning: the automated tests failed and point that in the database schema, the field that store the password hashed has a length of 40 (which is good for sha1 but not for sha256).

I wonder if the use of db_change_field is a good thing because of I understand the function, it will delete the current passwords.

So, I think we need to use a second field in the database for the sha256 passwords.

izus’s picture

izus commented

hi,
for the length warning part we will need to change it directly in hook_schema so that it's ok for both types.

we then need a hook_update_n to update the schema for sites that already installed the module and here db_change_field may be helpful :)

++

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs work » Needs review
StatusFileSize
new protected_node-use_sha256-2326905-6.patch10.55 KB

Hi,

Ok thanks for the response. I test db_change_field, the existing passwords ware not erased.

izus queued 6: protected_node-use_sha256-2326905-6.patch for re-testing.

  • izus committed f8fb52a on 7.x-1.x authored by DamienMcKenna 
    Issue #2326905 by Grimreaper, DamienMcKenna, izus: Use sha-256 instead...
izus’s picture

izus commented
Status: Needs review » Fixed

Oki
it seems good enough to be merged
Thank you all for your contributions
++

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.