Opencart's password hashing is vulnerable

[Ayrx]

From admin/model/user/user.php:

    public function editPassword($user_id, $password) {
        $this->db->query("UPDATE `" . DB_PREFIX . "user` SET salt = '" . $this->db->escape($salt = substr(md5(uniqid(rand(), true)), 0, 9)) . "', password = '" . $this->db->escape(sha1($salt . sha1($salt . sha1($password)))) . "', code = '' WHERE user_id = '" . (int)$user_id . "'");
    }

There are several things wrong with this line of code.

1. rand() is not an appropriate way to generate salts. According to PHP.net, rand() is not cryptographically secure way to generate random values. Salts have to be globally unique and using a good CSPRNG is the best way to achieve this.
2. Three iterations of SHA1 is most definitely not a good password hashing algorithm. This will fall very quickly to GPU-wielding attackers.

Recommendations:

1. Please read How to securely hash passwords? for password hashing best practices.
2. Use bcrypt. In PHP 5.5 and above, this can easily be done using password_hash(). For older PHP versions, there is the excellent phpass library.
3. admin/model/user/user.php is not the only part of the code that suffers from this vulnerability. I noticed the same problem in system/library/customer.php.

Does opencart have a way to be contacted privately for security issues? I would prefer not to open an issue publicly in the future especially for more high-risk bugs.

[danielkerr]

you dont seem to have any experience for developing open source scripts. if you did you would have realised that the minimum requirements for opencart is php 5.3 so using bcrypt is out!

[Ayrx]

@danielkerr I explicitly linked to phpass for older PHP versions. Please read the bug report fully before jumping to conclusions that "I don't seem to have any experience".

Salts don't really need to be cryptographically secure. They just need to be unique so rainbow tables are no longer a viable option.

I use password_compat for all my stuff. It's a backport of password_hash() functionality to PHP 5.3.7 and later.

[Ayrx]

@OpencartHelp They need to be globally unique, not just unique to your application. A good CSPRNG is the best way of achieving this.

Regardless, this point is moot. Use bcrypt and forget the whole generating/storing salts yourself issue since it's taken care of by the library. And yes, password_compat is a good option as well.

They need to be globally unique, not just unique to your application.

On the off chance someone just happens to have some generated rainbow tables with the salt in them? Come on...

[cloud101]

He is right though. The password hashing scheme you are using is flawed and shouldn't be used. The bigger issue here rather than how you generate salts (and to be honnest just change to a true random and cryptographically secure algorithm, it's easy) is that you are not applying any or few iterations to your hashing algorithm. Furthermore as Aryx said, you are better off with using a standard password hashing algorithm rather than rolling your own. Maybe bcrypt and scrypt can be an issue, but PBKDF2 shouldn't be (not that PBKDF2 is using a HMAC and not just iterations).

I'd reopen this case, it's an issue which should be fixed.

@cloud101 Not sure if that was aimed at me or not but I'm not a core developer.

[danielkerr]

another point you miss here is that customers generally use crappy passwords. 123abc or username123. That's how about 1 million adobe accounts got hacked.

its easier to brute force the password tables than it would be to use rainbow stripe tables with salt in.

how will the machine be able to tell if its found the password if its in between a salt? wouldn't it have to be a password in a dictionary?

what method is phpbb and wordpress using? are they vulnerable to this sort of attack as well?

[Ayrx]

1. Adobe's passwords got cracked because they were encrypting them in ECB mode instead of hashing them.
2. Yes, bruteforcing them is the only viable tactic once you use a salt. That's precisely the point, you are only using three iterations of SHA1 which isn't nearly slow enough. Use a hash like pbkdf2, bcrypt or scrypt instead.
3. Wordpress uses the phpass library which I linked to.

[danielkerr]

i will re open this. it does not look hard to copy the code from phpass.

although phpass coding standards have are pretty messed up.

[Ayrx]

I'm glad to hear this. password_hash (and password_compat for older PHP versions) might be viable choices as well.

[danielkerr]

i'm closing this as its a waste of time.