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".

[ghost]

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.

[ghost]

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.

[ghost]

@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.

[xiongchiamiov]

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

Brute force protection is one of the primary reasons to use a key stretching algorithm like bcrypt. If you can only process, say, 10 password attempts a second instead of several thousand, it's gonna take a lot longer to make progress.

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

You shouldn't copy and paste bits of cryptographic code. Just include the whole thing as a dependency and then use the library in your code.

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

Care to expand on that reason?

[xiongchiamiov]

You may also find this story from Ars interesting. You're not using md5 (good choice!), so it's not quite as bad as in the case they show, but sha1 is only about twice as slow as md5.

[ghost]

Hashcat and John The Ripper don't have OpenCart's hashing method yet so there's at least that. I'm sure once they do they'll be able to handle hundreds of millions if not billions of attacks per second. SHA1 was never meant to be computationally difficult.

[Ayrx]

Yeah, because writing a password cracker for a known algorithm is incredibly difficult right...?

Very disappointing response to some very real security issues by opencart.

[ghost]

@Ayrx Who said it was?

Edit: As far as updating existing crackers with new hash methods. Writing a password cracker that can run billions of operations a second is definitely not trivial.

[danielkerr]

@xiongchiamiov

Ayrx is basing this problem on a user already having access to a shops db! there is no limit for the number of tries!

@Ayrx

you dont need to write a password cracker for a known algorithm, if you have a copy of somebodys db all you need to do is pay for amazons cloud hosting service and have the system brute force crack the passwords. it will do 18.6 billion passwords a minute!

i could add 1024 character hash and put the password in between. its not going to to stop a brute force attack!

[ghost]

@danielkerr

You don't need an Amazon EC2 instance these days. GPU based attacks are literally 10,000 times faster than the old CPU ones. With an off-the-shelf video card you can make billions of guesses per second on a hash. Plus new mask-based attacks are scary effective at finding complex passwords.

The only saving grace (for the moment) is that the most popular GPU-based hash cracking programs don't allow for dynamic hashing algorithms. The hashing methods are all built in and OpenCart isn't one of them. That won't last forever though.

That's why using password_hash/bcrypt is important. It takes away the advantage GPU cracking currently has. You go from billions of attacks per second to hundreds. It doesn't make a bad password invincible but it goes a huge way in helping the situation. Hackers go from being able to easily find 80%+ of all passwords to only the simplest ones. That way if you do find yourself in the horrible situation of a breach you've bought yourself time to notify your users to update their passwords.

[jlem]

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!"

Lol. Bcrypt is supported in 5.3.... why do you think it's not?

http://www.php.net/manual/en/function.crypt.php

crypt($2y$......) is Bcrypt

PHP 5.5 and the password_compat libraries are just thin wrappers for crypt($2y/2a$) to make them a bit more idiot-proof, but you're not an idiot (right?) so you should have no problem implementing crypt($2y$) (5.3.7 or higher) or crypt($2a$) (versions lower than 5.3.7) without much trouble!

[DS-Matt]

@OpencartHelp makes some very good points!

[jeremykendall]

If you'd like an extremely easy way to upgrade from your current solution to password_hash, might I recommend Password Validator? It's a quick upgrade, validates passwords against password_hash, and can transparently upgrade from your current hashing implementation to the password_hash implementation.

[theshadow]

Shouldn't you be using bcrypt instead?

[mattattui]

Writing a password cracker that can run billions of operations a second is definitely not trivial.

Er …doesn't need to be billions per second, because rand() is piss poor, but even if it wasn't, spend $50 on EC2.

Look it's easy: if you don't know what you're doing, use bcrypt (in PHP 5.3 if you insist, even though it's end of lifed and no longer getting security fixes), and if you do… use bcrypt. Muppet.

[theshadow]

• This isn't cryptographically secure, don't use rand() especially don't use rand() without a seed. If you must use it then use mt_rand instead, but even then, don't do that. Use BCrypt or some other standardized library.
• You're not binding your SQL variables, even if your escape method is beyond expert and can prevent injection attacks you're still compiling that statement on every call losing out on the DB query cache
• You're assuming a mySQL DB
• Why are you even bothering to cast to an int and then wrap in a string? You're forcing the database to have to cast the value to the field type.

[jlem]

Why are you even bothering to cast to an int and then wrap in a string? You're forcing the database to have to cast the value to the field type.

It's a poor man's way to strip out any potential injection nasties. Casting to an integer MAKES it an integer, so any bogus string data gets blown away.

[ghost]

@inanimatt

Er …doesn't need to be billions per second, because rand() is piss poor, but even if it wasn't, spend $50 on EC2.

I'm not quite following what you're talking about. The password salt maybe? If so, those don't need to be cryptographically secure. That's not their purpose. Salts just need to be suitably unique and complex. That way rainbow tables are no longer a useful attack and duplicate passwords are obscured.

What I was talking about is brute force attacking a password. In that scenario a malicious user has already breached your site and obtained a copy of your database with the hashed passwords and salts. They then run it against a program like Hashcat or John The Ripper.

[mattattui]

Right, it’s 2014: sha1 rainbow tables aren’t difficult or time-consuming to generate any more. This 100 GPU rig cracks 63 billion SHA1 hashes per second, so yay, you do 3 iterations. That’s still 21 billion/sec. Or close to 250,000,000/sec per GPU. If you have the salt, and you assume that the password’s one of the top 10,000 that most people apparently use, your hashes aren’t going to take very long to defeat.

That’s why we use bcrypt, which has a tuneable computational cost to prevent exactly this kind of thing. You don’t have to generate billions of hashes either, most people apparently use one of 10,000 passwords. If you’re just going fishing through a stolen database you’ll probably go a surprisingly long way with a couple of GPUs and a few hours.

On 4 Apr 2014, at 21:56, rph notifications@github.com wrote:

@inanimatt

Er …doesn't need to be billions per second, because rand() is piss poor, but even if it wasn't, spend $50 on EC2.

I'm not quite following what you're talking about. The password salt maybe? If so, those don't need to be cryptographically secure. That's not their purpose. Salts just need to be suitably unique and complex. That way rainbow tables are no longer a useful attack and duplicate passwords are obscured.

What I was talking about is brute force attacking a password. In that scenario a malicious user has already breached your site and obtained a copy of your database with the hashed passwords and salts. They then run it against a program like Hashcat or John The Ripper.

—
Reply to this email directly or view it on GitHub.

[adamculp]

With all the time you folks spent talking about this you could have simply done a pull request after fixing the issue.

Lose the ego and keep people safe.

[ghost]

@inanimatt

sha1 rainbow tables aren’t difficult or time-consuming to generate any more.

Do you mean brute forcing is much faster now? Rainbow tables aren't applicable to this situation since there are salts. Plus they would definitely be more time and resource consuming compared to GPU based brute force attacks.

For anyone interested in the subject I would highly recommend Bob Weiss' lecture Size Does Matter: Password Tools and Data. It's the best talk I've seen on the subject. Also download oclHashcat and give it a try. I had a chance to use it recently for some security upgrades and was amazed at the results on some spare hardware (91% MD5 recovery on a 3100+ hash list in the 48 hour period run time; 900M to 1400M h/s).

[ghost]

@adamculp

With all the time you folks spent talking about this you could have simply done a pull request after fixing the issue.

You could always lead by example.

I have some code for OpenCart 1.5.1 that does this but since Daniel hasn't indicated he'll accept a pull request there isn't much point to upgrading it for v2.0 and submitting it. I haven't done anything with Community Edition because I don't want to break forward compatibility.

[adamculp]

Would love to, but I'm not as up to speed as I'd like to be for this. :( However, there are many on this thread who seem more than knowledgeable enough to handle it correctly.

[mattattui]

Yes, I mean that brute forcing is much, much, much faster now, as oclHashcat demonstrates. Salting stops two things: 1. googling the hash, 2. using a pre-computed rainbow table. The point is that for weak hashing methods like this one, brute-forcing is no longer a big enough challenge to be a deterrent. To defeat brute-forcing, you need a hashing algorithm that is intentionally slow, like bcrypt.

On 5 Apr 2014, at 01:20, rph notifications@github.com wrote:

@inanimatt

sha1 rainbow tables aren’t difficult or time-consuming to generate any more.

Do you mean brute forcing is much faster now? Rainbow tables aren't applicable to this situation since there are salts. Plus they would definitely be more time and resource consuming compared to GPU based brute force attacks.

For anyone interested in the subject I would highly recommend Bob Weiss' lecture Size Does Matter: Password Tools and Data. It's the best talk I've seen on the subject. Also download oclHashcat and give it a try. I had a chance to use it recently for some security upgrades and was amazed at the results on some spare hardware (91% MD5 recovery on a 3100+ hash list in the 48 hour period run time).

—
Reply to this email directly or view it on GitHub.

[ghost]

@adamculp

Here's part of my old (obsolete) code for 1.5.1 to give you some ideas.

<?php
class Password {
    public function hash($password) {
        $hash = password_hash($password, PASSWORD_DEFAULT);

        if ($hash === false) {
            trigger_error('Unable to hash password!');
            return md5($password);
        } else {
            return $hash;
        }
    }

    public function isValid($password, $hash) {
        if (password_verify($password, $hash) || md5($password) === $hash) {
            return true;
        } else {
            return false;
        }
    }

    public function needsUpdate($hash) {
        if (password_needs_rehash($hash, PASSWORD_DEFAULT)) {
            return true;
        } else {
            return false;
        }
    }
}
?>

[tigerhawkvok]

This may help you, and should be easy to implement:

https://github.com/tigerhawkvok/php-stronghash

Please let me know if you (or anyone else on this thread) see any issues.

[danielkerr]

i think crypt should be ok, anyone have issues with this?

[mattattui]

@danielkerr crypt() is fine, but password_hash() with password-compat makes more sense - a single simple, future-proof interface and it also does the boring legwork of generating a secure salt for you. Once Opencart's requirements increase to 5.5+ (it'll happen one day), you can ditch the compatibility library without changing any code.

[ghost]

password_compat also allows you to use password_hash() in PHP 5.3.7 which is the minimum version you want to be using for crypt() implementing Blowfish hashing anyway.

Edit: Here's some of the 1.5.1.3 code I did: https://gist.github.com/opencarthelp/10011152#file-customer-php-L48-L58

[tml]

crypt() will use Blowfish, as long as you format your salt correctly; PHP ships with its own Blowfish implementation (largely taken from OpenWall) as of 5.3.0 - a few of us worked very hard to make that happen before 5.3.0 was released so that people would be able to have a viable option.

[theshadow]

@jlem So why not just bind variables and not muck around with having to handle it at all?

[jlem]

@theshadow Only the open cart creator can fill us in on why he's not using prepared statements to safely bind parameters. Though I'm not surprised he's not using prepared statements if he's also triple sha1ing his passwords when there are so many better options available to him.

[ghost]

@tml If you're using crypt() you should be at least using PHP 5.3.8 because of some rather serious bugs.

[tml]

@OpencartHelp 👍 Although, I would say if you're only running 5.3.8 you're at considerable risk to other issues that are likely more serious than the crypt() issue; as we noted in the security bulletin written at the time, "for passwords without characters with the 8th bit set, there's no issue" with crypt()'s blowfish implementation pre-5.3.8; but I agree with the general principle that you should always run the most recent release of PHP that can possibly work in your environment.

[ghost]

@tml I was also thinking of the nasty CRYPT_MD5 bug in 5.3.7. In a perfect world users would at least be using a recent version of 5.4 but I've seen hosts that are still using PHP 5.2.

[tml]

@OpencartHelp Well, yes - hopefully no one is using either 5.3.7 nor CRYPT_MD5 :)

[fitorec]

I checked the login function in system/library/user.php:37 ,and occurred to me an idea of ​​how to simulate the generation of passwords:

/**
 *
 **/
function genPassword($passwordTxtPlain, $salt) {
    return sha1($salt . sha1($salt . sha1($passwordTxtPlain)));
}

$test = array(
    'salt' => '03ad53208',
    'password' => 'opencart_pass',
    'expected' => '19181f51af2af6bc0392067d57ced726749d2bcb'
);

$password = genPassword($test['password'], $test['salt']);

if($password == $test['expected']) {
    echo "It's great!, The function is correct";
}

Maybe we could use the internally at the core of opencart, what do you think?

[kholia]

Apologies for resurrecting an old topic but here are some hash cracking numbers,

https://github.com/magnumripper/JohnTheRipper/wiki/Cracking-OpenCart-hashes-with-JtR