Posted on 8 Comments

One-way Password Crypting Flaws

I was talking with a client and the topic of password crypting came up. From my background as a C coder, I have a few criteria to regard a mechanism to be safe. In this case we’ll just discuss things from the perspective of secure storage, and validation in an application.

  1. use a digital fingerprint algorithm, not a hash or CRC. A hash is by nature lossy (generates evenly distributed duplicates) and a CRC is intended to identify bit errors in transmitted data, not compare potentially different data.
  2. Store/use all of the fingerprint, not just part (otherwise it’s lossy again).
  3. SHA1 and its siblings are not ideal for this purpose, but ok. MD5 and that family of “message digests” has been proven flawed long ago, they can be “freaked” to create a desired outcome. Thus, it is possible to manufacture a source string that generates an MD5 of course.
  4. Add a salt of reasonable length (extra string added to password), otherwise dictionary attacks are way to easy. In addition, not using a salt means that two users who have the same password end up with the same encrypted password which is another case of “too much info” for people. Salt should of course be different for each user. Iterate.
  5. Even if someone were to capture your user/pwd table, they should not be able to decode the passwords within a reasonable amount of time. Flaws in any of the above issues can make such attacks easy.

The below code, used in a variety of ecommerce packages (osCommerce prior to v2.3.0, ZenCartCRE Loaded / Loaded Commerce, and other derivatives and descendants of oscommerce) on the surface appears to do something quite smart. Note that this code does not use an external salt (such as the username or other separate field) but instead generates it and adds it to the encrypted password. This enables it to be used in applications where no username or other login constant other than the password is available, although I’d consider that quite rare.

function validateAdminPassword($plain, $encrypted) {
  if (!$plain && !$encrypted) {
    return false;
  }

  $stack = explode(':', $encrypted);
  if (sizeof($stack) != 2)
    return false;

  if (md5($stack[1] . $plain) == $stack[0]) {
    return true;
  }

  return false;
}

function encryptAdminPassword($plain) {
  $password = '';

  for ($i=0; $i<10; $i++) {
    $password .= rand();
  }

  // arjen comment: so the 2 is what you need to increase,
  // as well as the length of the relevant database column.
  $salt = substr(md5($password), 0, 2);

  $password = md5($salt . $plain) . ':' . $salt;

  return $password;
}

This code is flawed. Apart from being confusing (using the $password variable name when calculating the salt) the main problem is that the salt ends up too short. The code generates 10 pseudo-random characters (PHP tends to initialise the random generator from time, so it can be somewhat predictable which is a potential attack vector – for instance when the creation time of the user record is also stored) but then it’s run through MD5() after which only the two first characters of the resulting message digest are used for the actual salt. Since the MD5 comes out as hex digits, the range of each of the two characters is [0-9a-f] and so the total number of possibilities for the salt string is 256. That’s not a lot!

The effort involved in pre-calculating the MD5s (including all salt permutations) is not that high, it’s merely 256 times the size of the dictionary used. Wouldn’t take that much disk space. Since this code is used by lots of sites, the potential for a successful attack is rather high in that sense also. Combined with the lack of iteration, this just makes an attack all too easy.

Finally, if the user table were captured from a site with a large number of users, the chance of finding colliding encrypted passwords is quite a bit higher than it should be. But the above mentioned approach already has sufficient potential for a damaging security breach.

If this code is active on your site, a quick patch would be to increase the length of the salt by changing the substr() call, and make it do iterations. Obviously you’ll also need to similarly increase the length of the password storage column in your database. You then get old and new crypted passwords in your table and you can work out which version by checking the length of the crypted password string. On login you can replace old for new for each user as you’ll have their plain password at that point (since they just filled it in and sent it to your app). That way you can create a clean transition. I grant you it’s not perfect but it’s at least improving an otherwise very insecure situation.

If, rather than pragmatically fixing up an existing environment that you didn’t write, you want to do all this properly, read Password Storage Cheat Sheet from OWASP (the Open Web Application Security Platform). It lists a range of considerations (and reasoning) beyond my basic pragmatic list. If you’re going to code from scratch, please do it right.

Edit: 20120717: added maximum affected osCommerce version thanks to Harald Ponce de Leon. osCommerce as of v2.3.0 uses phpass like WordPress

Posted on 8 Comments

8 thoughts on “One-way Password Crypting Flaws

  1. – latest oscommerce uses different code
    – zencart still uses it
    – creloaded don’t offer a download

    google for “if (sizeof($stack) != 2) return false;” … this code is used by a LOT of people

    1. thanks for the update Brett!

  2. It appears this code was in CRELoaded when it was given PCI compliance. sigh.

  3. a more telling websearch is “$salt = substr(md5($password), 0, 2)”.

    I found a review of the same code by Ryan Uber here http://www.ryanuber.com/oscommerce-password-hashing-function-ghetto-maybe.html that doesn’t quite emphasise the limited character set of the salt combined with its length but highlights other faults.

  4. Don’t forget OWASP “Rule 3: Iterate the hash” – this is called stretching, and has been a standard component of good password hashing for decades, omitting this is serious issue, since even properly used salts are not effective enough against modern CPUs and rainbow tables. Stretching builds in a fixed CPU/resource cost that is acceptable for validation checks, but increases time for an attacker by orders of magnitude. I also suggest looking at phpass (http://www.openwall.com/phpass/) which is written by an expert in password hashing and has had review by many others.

  5. Looks good but I know for a fact there is many flaws in the sha1 and md5 even with salts. My old forum I use to host had a few people that were breaking security of many sites using high end security with sha1+salt without dictionary attacks. I think most ethical hackers or people interested in computer security that don’t go into the great depths of blackhat hacking every day for many hours, that the surface level security you think that should be fixed is enough, but when a ex hacker or hacker (not a script kiddie) look’s at something like this blog article they would just laugh and think to themselves (It would seem we are still ahead). But it was a good read 🙂

    1. Thanks for your response, Alisha. Much appreciated.
      I am a pragmatist. Fact is many sites use these packages, which are completely insecure in their current state.
      While, as I already mentioned in the article, I fully agree that fundamental architectural changes are appropriate, I also appreciate the fact that that is not within the realm of capabilities of the people deploying the packages.
      So, by making them aware of the flaws they can be more selective when choosing a package, and with minor modifications they can improve the security of an existing site a little bit.
      It’s by no means ideal, but it’s way better than what is out there now.
      In addition, because I leave the issues of how much longer the salt should be and how many iterations to run open, different people will implement this differently. While again that does not improve overall security as such, it will result in creating different implementations. That means that a stupid generic attack will no longer work on any of those deployments, and that too is a win.

  6. I just don’t believe that there is or will be a secure state. The amount of script kiddies out there trying to break site security via sql injection, local file inclusions, remote file inclusions in php/asp/html/aspx sites. Script kiddies use generic attacks but they join hacker forums and gain the knowledge or tools to do more advanced attacks so the ethical security coders/pentesters keep getting mad cause these kids keep finding ways around the wall.

    All I can say is you can slow hackers but not stop them.

Comments are closed.