Skip to content

Conversation

PatOnTheBack
Copy link

This addresses issue #369.

@mymindstorm
Copy link
Member

mymindstorm commented Jul 30, 2019

Thanks for the PR! I'm 99% sure this will break all existing backups and encryption. This needs to migrate old data.

This has been on my to-do list for a while. Instead of just moving to SHA256, it might be a better idea to move to a full blown password hashing algorithm via libsodium or some other crypto library that's being actively maintained.

I'm on vacation right now, I'll need a while to get around to this (and finishing up 5.6).

@Sneezry
Copy link
Member

Sneezry commented Jul 31, 2019

We use MD5 as an identity of an entry, but not for data protection. Will this issue affect us? What's your idea @mymindstorm

@PatOnTheBack
Copy link
Author

I suppose it is more of a theoretical threat. Someone who is manipulating the code in the browser extension could create a fake entry that has the same MD5 hash as the real entry.
The infosec industry is gradually moving away from MD5 because of the vulnerabilities discovered in it.
SHA256 will run more slowly than MD5, so if speed is important, MD5 might still be a better choice.

@PatOnTheBack
Copy link
Author

@mymindstorm I agree that using a password hashing algorithm is best.
libsodium only supports Scrypt and Argon2.
This NPM package for bcrypt seems to be currently maintained, so bcrypt is another option.

@mymindstorm
Copy link
Member

mymindstorm commented Aug 7, 2019

Someone who is manipulating the code in the browser extension could create a fake entry that has the same MD5 hash as the real entry.

I don't really see the point of moving solely due to the collision vulns. The most damage exploiting that (in our context) could do would just replace an entry w/ bad data. My understanding of the benefit of a password hashing algorithm is that they are intentionally slow as to make brute forcing infeasible.

For me

https://github.com/antelle/argon2-browser

https://www.npmjs.com/package/@types/argon2-browser

@Sneezry
Copy link
Member

Sneezry commented Aug 18, 2019

I'm closing this PR because we have no plan to change the hash currently to avoid breaking change. Please feel free to raise a new thread if you have any other idea. Thanks for your contribution.

@Sneezry Sneezry closed this Aug 18, 2019
@mymindstorm mymindstorm mentioned this pull request Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants