Rambox hashes passwords using the MD5 hash function. #545

Open
EdOverflow opened this Issue Jan 2, 2017 · 1 comment

Projects

None yet

2 participants

@EdOverflow
EdOverflow commented Jan 2, 2017 edited

Summary

Rambox hashes passwords using the MD5 hash function, which is not a secure method of password storage. MD5 is too fast and you should therefore use a slow algorithm as suggested in the "How can this be fixed?" section.

Why does this vulnerability exist?

Rambox has a createHash(alogrithm) function which creates and returns an object which you then generate hash digests with (digest('hex')). The problem here is that you have chosen md5 for the algorithm parameter, which shouldn't be used to store passwords. Even if you do add a salt to the password, this form of password storage remains insecure.

The issue is located in several files, for example, in main.js MD5 is implemented as follows:

ipcMain.on('validateMasterPassword', function(event, pass) {
	if ( config.get('master_password') === require('crypto').createHash('md5').update(pass).digest('hex') ) {
		createWindow();
		mainMasterPasswordWindow.close();
		event.returnValue = true;
	}
	event.returnValue = false;
});

Link: https://github.com/saenzramiro/rambox/blob/master/electron/main.js#L271-L278

On a side note, this line is vulnerable to timing attacks, because you are performing a byte-by-byte comparison of two strings which terminates as soon as two characters do not match:

if ( config.get('master_password') === require('crypto').createHash('md5').update(pass).digest('hex') 

MD5 is used here too: https://github.com/saenzramiro/rambox/blob/master/app/view/preferences/PreferencesController.js#L19

The MD5 hashing function implementation can be found here: https://github.com/saenzramiro/rambox/blob/master/app/util/MD5.js

What is MD5?

If you would like to learn more about the MD5 hash function, I wrote a chapter on it in Laurens Van Houtven's "Crypto 101" book: https://github.com/crypto101/book/blob/master/Crypto101.org#md5 😊

Why shouldn't MD5 be used for hashing passwords?

Although researchers have demonstrated MD5 collisions, this is not the issue with storing passwords. In order to securely hash passwords one must use a slow hashing function, which MD5 isn't. By using a fast hash function like MD5 you make password cracking easier.

On a side note, HMAC-MD5 is still a secure form of message authentication; however, it probably shouldn’t be implemented in new cryptosystems.

How can this be fixed?

For better security, use a slow algorithm like PBKDF2 or bcrypt.

You can use the crypto.pbkdf2 function as follows:

crypto.pbkdf2(password, salt, iterations, keylen, digest, callback)

Link: https://nodejs.org/api/crypto.html#crypto_crypto_pbkdf2_password_salt_iterations_keylen_digest_callback

If you would rather use bcrypt there is a package here: https://www.npmjs.com/package/bcrypt

A final thing, hashing is not the same thing as encryption, so you might want to rename the function to hash and you misspelled encrypt (Currently it is encypt).

If you require any further information, feel free to contact me.

@saenzramiro saenzramiro added this to the 1.0.0 milestone Jan 3, 2017
@saenzramiro
Owner

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment