A while back, one of the guys here at work set up an open source collaboration web app to exchange some files with a customer, being an inquisitive chap I thought I'd take a wander through the source code to see how it fared from a security stand point. My first port of call for these sort of things is usually to have a look at how they're doing authentication and in this case they're doing (arguably) the right thing or, at least better than a number of other web apps out there and storing an individually salted sha1 hash of the password, they also offer the option of LDAP integration, so far so good.
The code for validating password complexity when setting passwords turned out to be a bit more interesting, the user’s password is stored with the other user details in the user table in MySQL, but there is also a password table, and the passwords in it don't look like sha1 hashes... A little more digging through the code reveals this table to be a password history table, but this doesn't explain why the passwords aren't just stored as hashes, there are also a couple of functions with ominous names called within the password reset code (cp_encrypt and cp_decrypt) that take the password as a parameter... eeek! Also, testing confirms that not just the history, but also the current password is stored in this table.
1: function cp_encrypt($password, $time){
2: //appending padding characters
3: $newPass = rand(0,9) . rand(0,9);
4: $c = 1;
5: while ($c < 15 && (int)substr($newPass,$c-1,1) + 1 != (int)substr($newPass,$c,1)){
6: $newPass .= rand(0,9);
7: $c++;
8: }
9: $newPass .= $password;
10:
11: //applying XOR
12: $newSeed = md5(SEED . $time);
13: $passLength = strlen($newPass);
14: while (strlen($newSeed) < $passLength) $newSeed.= $newSeed;
15: $result = (substr($newPass,0,$passLength) ^ substr($newSeed,0,$passLength));
16:
17: return base64_encode($result);
18: }
1: function cp_decrypt($password, $time){
2: $b64decoded = base64_decode($password);
3:
4: //applying XOR
5: $newSeed = md5(SEED . $time);
6: $passLength = strlen($b64decoded);
7: while (strlen($newSeed) < $passLength) $newSeed.= $newSeed;
8: $original_password = (substr($b64decoded,0,$passLength) ^ substr($newSeed,0,$passLength));
9:
10: //removing padding
11: $c = 1;
12: while($c < 15 && (int)substr($original_password,$c-1,1) + 1 != (int)substr($original_password,$c,1)){
13: $c++;
14: }
15: return substr($original_password,$c+1);
16: }
Looking into the encrypt and decrypt functions, there are a couple of things of note; firstly their very existence tells us that the passwords are being stored in a reversible form in the database, essentially defeating the purpose of storing the password as a salted hash. This means that an administrator on the system can decrypt and use any user's password; it also means that should the server be compromised an attacker could also trivially decrypt the passwords and use these accounts. While salted SHA1 hashes are also crackable in short order with the right hardware they do raise the bar somewhat. To mitigate the SHA1 attack it would be relatively straight forward in this case to swap out the SHA1 function being used and substitute it with a slow password hashing algorithm such as scrypt or bcrypt.
A review of the encryption algorithm being used is also pretty interesting; the developer has chosen to use XOR encryption, the password is padded to 16 characters and XOR’d with an MD5 of a pseudo-random string generated at install and the current date. In the case of short strings like passwords, this is actually a reasonably effective encryption algorithm as the password is unlikely to be any longer than 16 characters (in the event it is the key material is repeated which introduces a weakness). However in this context the encryption being used is really academic, the passwords shouldn’t be stored in a reversible format, though when you look at the apparent reasoning behind it there is an interesting security trade-off.
The impetus behind storing the passwords encrypted appears to be so that the application can do a comparison of the new password with previous passwords to ensure that not only is it different from previous passwords but also that it isn’t too similar to any previous passwords. The motivation here is noble but, in my opinion, somewhat misguided, a better approach would probably be to prompt the user based on the entropy of the new password while also checking that it doesn’t match any of the previous X passwords. While this doesn’t solve the problem of having users choose passwords and incrementing a suffix it means that passwords aren’t being stored reversibly, even removing the current password from the history table doesn’t mitigate the problem as users still have a tendency to choose passwords based on a pattern, regardless of the checks that you enforce. Password selection guidance is still maybe best done as part of a security education program.
1:25 PM
Justin
, Posted in
