PHP Documentation Woes

This bit of php documentation has some interesting code:

 <?php  
 $passphrase = 'My secret';  
   
 /* Turn a human readable passphrase  
  * into a reproducable iv/key pair  
  */  
 $iv = substr(md5('iv'.$passphrase, true), 0, 8);  
 $key = substr(md5('pass1'.$passphrase, true) .   
         md5('pass2'.$passphrase, true), 0, 24);  
 $opts = array('iv'=>$iv, 'key'=>$key);  
   
 $fp = fopen('secret-file.enc', 'wb');  
 stream_filter_append($fp, 'mcrypt.tripledes', STREAM_FILTER_WRITE, $opts);  
 fwrite($fp, 'Secret secret secret data');  
 fclose($fp);  
 ?>  

3DES takes a 192-bit key (actually 168), so the first two keys are taken from the first MD5 and the third key is taken from the second MD5. I have a strong suspicion that that alignment introduces some kind of vulnerability, but I can't quite put my finger on it. If anyone knows, please leave a comment.

This tweet says it all:

@DefuseSec With documentation like that, it's no wonder we see so much crap code. :-\
— Adam Caudill (@adamcaudill) July 29, 2013

Lessons Learned:

• Don't use MD5 to derive keys from passwords. Use PBKDF2.
• Use a random, unique IV. Don't derive it from the password.
• Documentation will be used by many people. It's worth hiring a cryptographer to check it.

Password Hashing Fail

Here's a bit of password hashing fail.

I'm not sure if it will win the Password Hashing Competition, but it certainly is novel. I really like how, after it goes through all of that sha1/md5/rot13 "encryption", it saves the plaintext to the database. Oh, and if it weren't for a misplaced '}', it would accept any password in the database, not just the one for your account. It's also vulnerable to SQL injection (pointed out by an Anonymous commenter).

Hashing a password:

 public function encryptData($string)
 {  
   $stringORG = $string;  
   $string = str_rot13($string);  
   $string = md5($string);  
   $string = substr($string, 5, 15);  
   $string = sha1(SALT . $string);  
   $string = md5($string);  
   $string = sha1(SALT . $string);  
   $string = str_rot13($string);  
   $string = substr($string, 5, 15);  
   $string = md5(SALT . $string);  
   mysql_query("INSERT INTO `encryptions` (`encvalue`, `realvalue`) VALUES ('".$string."', '".$stringORG."');");  
   return $string;  
   }  

Testing a password:

public function tryEncryption($tried)
{  
  $query = mysql_query('SELECT * FROM encryptions');  
  while($row = mysql_fetch_assoc($query))  
  {  
    $encVal = $row['encvalue'];  
    $actVal = $row['realvalue'];  
  }  
    $actTried = $tried;  
    $tried = str_rot13($tried);  
    $tried = md5($tried);  
    $tried = substr($tried, 5, 15);  
    $tried = sha1(SALT . $tried);  
    $tried = md5($tried);  
    $tried = sha1(SALT . $tried);  
    $tried = str_rot13($tried);  
    $tried = substr($tried, 5, 15);  
    $tried = md5(SALT . $tried);  
    if($encVal==$tried)  
    {  
      if($actVal==$actTried)  
      {  
        return true;  
      }  
    }  
}  

Lessons Learned:

• When you encrypt something, make sure the plaintext is safely destroyed.
• Hash passwords properly.
• Write unit tests for your code.

PHP-Encrypt-Decrypt: Binary vs. Hex

This PHP script makes an interesting mistake. It attempts to derive a key from a password using md5, but it uses the hex-encoded MD5 output as the key, not the raw binary output. The hexadecimal string is passed to the mcrypt_encrypt() function, which interprets the 32-character hex string as a 256-bit key, even though it's really only 128 bits.

The author's intention was probably to use AES. In PHP, MCRYPT_RIJNDAEL_256 refers to the 256-bit block variant of Rijndael, not the 128-bit block version that was selected for AES. The key size of 128, 192, or 256 bits is determined by the length of the key parameter.

 $SECRET_KEY = 'YOUR SECRET KEY';  
 // ...  
 $encrypted_i = base64_encode(mcrypt_encrypt(MCRYPT_RIJNDAEL_256, md5($SECRET_KEY), $value, MCRYPT_MODE_CBC, md5(md5($SECRET_KEY))));  

Lessons Learned:

• Specify crypto parameters in the right format. Look at the output of the functions you use to make sure it's in the format you expect.
• Use key stretching (PBKDF2) to derive a key from a password, not MD5.
• Generate a random IV at the time of encryption. Do not derive it from the key.
• Use test vectors to ensure you are using the correct primitives.

PHP-Security-Library: $ciphertext->key

It's generally not a good idea to store the key in your ciphertext object. If you do, naive users are going to think it's ok to store the key with the ciphertext, and advanced users are going to serialize() it all by accident.

 use \PSL\Encrypter;  
 $confidentalText = 'I am using PSL';  
 $cipherText = Encrypter::encrypt(MCRYPT_RIJNDAEL_256, $confidentalText);  
 // $cipherText is an instance of \PSL\CipherText.  
 // To extract, do something like this:  
 $key = $cipherText->key;  
 $plainText = Encrypter::decrypt($cipherText, $key);  
 // $plainText is now same as $confidentalText.  

Lessons Learned:

• Do not store the key in the ciphertext.
• Design APIs so that it is as difficult as possible to use them incorrectly.

Password Generators: Math.random()

When you do a Google search for "password generator", you find a lot of JavaScript-based password generators. Unfortunately, all of the high-ranking ones use Math.random(), which is a pseudorandom number generator unfit for cryptographic use. In Firefox and other browsers, Math.random() is seeded with the time and provides at most 41 bits of entropy. The JavaScript-based generators don't support SSL/TLS connections, either.

In this one, the programmer attempted to generate an integer in the range [0, 25] by multiplying Math.random() by 25 then rounding to the nearest integer. This creates a bias away from 0 and 25. To get a 0 or 25, the unrounded value must fall in the range [0, 0.5) or [24.5, 25] respectively. The width of these ranges is 0.5. For all other results (1 through 24), the width of the range is 1.0. For example, to get a 6, the unrounded value can fall anywhere in [5.5, 6.5).

 for (var i = 0; i < lengthOfPassword; i++) {  
      capitalise = Math.round(Math.random() * 1);  
      if (capitalise === 0) {  
           StrongPasswordArray[i] = theLetters.charAt(Math.round(Math.random() * 25)).toUpperCase();  
      }  
      else {  
           StrongPasswordArray[i] = theLetters.charAt(Math.round(Math.random() * 25));  
      }  
 }  

Lessons Learned:

• Use a cryptographically secure random number generator when generating a key, password, or initialization vector.
• Don't use floating point numbers for cryptography.
• Test the output of your random number generator with tools like diehard.
• Enforce an SSL/TLS connection if client-side scripts deal with sensitive information.

osTicket: Fail Open, Fail Often

osTicket is (according to them) the "world's most popular open source customer support ticket system." They use one of the worst-designed encryption functions I've ever seen.

First of all, they're trying to use symmetric encryption to secure passwords. They should be using a slow hash (pbkdf2, bcrypt, scrypt) with salt. Edit: Actually, they have to be able to decrypt passwords, because they are used in POP3/IMAP connections. They have to be able to provide the plaintext passwords to a third party, not just verify them. Users' passwords are hashed.

Second, the encryption function fails open. If the mcrypt library is missing, encryption does nothing, and passwords are stored in the database in plain text. It does log a warning message, but who's going to see that?

I can almost feel the programmer's frustration, trying all possible combinations of trim(), IVs, and cipher modes to get the damn thing to decrypt properly. They tried to generate a random IV at encryption time, but did not realize they need to encode it with the ciphertext so it can be given to mcrypt_decrypt(). Instead, they use ECB mode, which ignores the IV. They even generate a random IV in the decryption function!

 function encrypt($text, $salt) {  
   
   //if mcrypt extension is not installed--simply return unencryted text and log a warning.  
   if(!function_exists('mcrypt_encrypt') || !function_exists('mcrypt_decrypt')) {  
     $msg='Cryptography extension mcrypt is not enabled or installed. IMAP/POP passwords are being stored as plain text in database.';  
     Sys::log(LOG_WARN,'mcrypt missing',$msg);  
     return $text;  
   }  
   
   return trim(base64_encode(mcrypt_encrypt(MCRYPT_RIJNDAEL_256,$salt, $text, MCRYPT_MODE_ECB,  
             mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB), MCRYPT_RAND))));  
 }  
   
 function decrypt($text, $salt) {  
   if(!function_exists('mcrypt_encrypt') || !function_exists('mcrypt_decrypt'))  
     return $text;  
   
   return trim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256, $salt, base64_decode($text), MCRYPT_MODE_ECB,  
           mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB), MCRYPT_RAND)));  
 }  

The "salt" (which is really the encryption key) is "randomly" generated at install time and stored in a source code file. If that happens to be missing, they use the md5 hash of the admin's email address.

 # This is to support old installations. with no secret salt.  
 if(!defined('SECRET_SALT')) define('SECRET_SALT',md5(TABLE_PREFIX.ADMIN_EMAIL));  

Here's how they generate the "salt." I shouldn't need to explain why this is wrong.

 function randCode($len=8) {  
   return substr(strtoupper(base_convert(microtime(),10,16)),0,$len);  
 }  

 $configFile= str_replace('%CONFIG-SIRI',Misc::randcode(32),$configFile);

Lessons Learned:

• Fail safe. If your encryption library isn't available, fail hard, don't just return the plaintext.
• Use a cryptographically secure random number generator to generate secrets.
• Protect passwords by hashing them.

Silent Circle: Downplaying Critical Vulnerabilities

Silent Circle, an encrypted communications mobile app, censored vulnerability reports and did not disclose the existence of severe security bugs to their users. This is a very bad practice because users are not aware that they are vulnerable, and cannot decide to stop using Silent Circle until the vulnerabilities are fixed.

Lessons Learned:

• Always inform your users of critical security bugs as soon as possible. Even if they can't fix it themselves, they can always chose not to use the software until it has been updated.

Cryptocat: RNG Bias, Decimal vs. Bytes

Cryptocat is a browser-based encrypted chat program. Cryptocat generates random numbers by generating random decimal digits, then converting them to a floating point number.

The way they were generating random decimal digits is biased. They did so by generating a random byte, discarding it if it was greater than 250, and otherwise dividing it by 10 to get an integer remainder between 0 and 9. They did this because 256 different values do not divide evenly into groups of 10, so there would be a bias. They didn't realize that between 0 and 250, there are actually 251 different integers, which still does not divide evenly into groups of 10.

  @@ -60,7 +60,7 @@ else {  
    var x, o = '';  
    while (o.length < 16) {  
     x = state.getBytes(1);  
 -   if (x[0] <= 250) {  
 +   if (x[0] < 250) {  
      o += x[0] % 10;  
     }  
    }   

They also confused bytes for decimal digits, and used too little entropy to generate their ECC keys.

 @@ -88,8 +88,8 @@ function checkSize(publicKey) {  
  // Generate private key (32 byte random number)  
  // Represented in decimal  
  multiParty.genPrivateKey = function() {  
 - rand = Cryptocat.randomString(32, 0, 0, 1);  
 - myPrivateKey = BigInt.str2bigInt(rand, 10);  
 + var rand = Cryptocat.randomString(64, 0, 0, 0, 1);  
 + myPrivateKey = BigInt.str2bigInt(rand, 16);  
   return myPrivateKey;  
  }  

NakedSecurity has an excellent blog post on the RNG flaw.

Lessons Learned

• Test the output of your random number generators with tools like diehard.
• Visually inspect the output of the functions you're using, to make sure it's in the format you expect.

SaltStack: RSA e = d = 1

An instance of textbook RSA has three parameters: e, d, and n, where e = d^-1 (mod phi(n)). Encryption of a message m is m^e, and decryption of a ciphertext c is c^d = (m^e)^d = m^ed = m¹ = m.

When you set e = 1, encrypting a message does not change it, since raising any number to the power of 1 does not change it. SaltStack was using 1 as their RSA public key (e), so encryption was doing nothing:

 @@ -47,7 +47,7 @@ def gen_keys(keydir, keyname, keysize, user=None):  
    priv = '{0}.pem'.format(base)  
    pub = '{0}.pub'.format(base)  
 -  gen = RSA.gen_key(keysize, 1, callback=lambda x, y, z: None)  
 +  gen = RSA.gen_key(keysize, 65537, callback=lambda x, y, z: None)  
    cumask = os.umask(191)  
    gen.save_key(priv, None)  
    os.umask(cumask)   

What really amazes me is that the library they're using, M2Crypto, lets you do this without any kind of error or warning.

Lessons Learned:

• Have a professional cryptographer review your parameter choices.
• Write unit tests to check that messages can't be decrypted with a different key (edit: This might not catch the problem, see the comments).
• Before using a cryptography library, it's a necessary to understand something about what it's doing behind the scenes.

Synergy: Integer Overflow, Key Reuse, IV Reuse

Synergy is a software that lets you share your mouse and keyboard between multiple computers. Until recently, it didn't have any support for encryption, which left users' keystrokes and mouse movements vulnerable to sniffing on the local network. Users worked around this limitation by tunneling Synergy's communications through a VPN or SSH. Lots of users complained, and the Synergy team decided to encrypt the connections. Instead of using a well-known protocol like TLS, they invented their own.

The most obvious problem is that they are re-using the same key and IV for encrypted communications in both directions. This is absolutely fatal for modes like CTR, OFB, GCM, and probably CFB, which are the ones Synergy supports. Update: A tool that exploits this vulnerability is available here.

Second, they derive the IV from the password, and because of an integer overflow bug, when the length of the password is congruent to its double mod 256, the key and the IV are the same. If the IV were the same size as the output of the hash function they're using (SHA256), you'd be able to recover the key for all passwords longer than 128 characters.

 if (!options.m_pass.empty()) {  
     createKey(m_key, options.m_pass, kKeyLength, static_cast<UInt8>(options.m_pass.length()));  
     byte iv[CRYPTO_IV_SIZE];  
     createKey(iv, options.m_pass, CRYPTO_IV_SIZE, static_cast<UInt8>(options.m_pass.length()) * 2);  
     setEncryptIv(iv);  
     setDecryptIv(iv);  
  }  

 void  
 CCryptoStream::createKey(byte* out, const CString& password, UInt8 keyLength, UInt8 hashCount)  
 {  
      assert(keyLength <= SHA256::DIGESTSIZE);  
      byte temp[SHA256::DIGESTSIZE];  
      byte* in = reinterpret_cast<byte*>(const_cast<char*>(password.c_str()));  
      SHA256().CalculateDigest(temp, in, password.length());  
      byte* tempKey = new byte[SHA256::DIGESTSIZE];  
      for (int i = 0; i < hashCount; ++i) {  
           memcpy(tempKey, temp, SHA256::DIGESTSIZE);  
           SHA256().CalculateDigest(temp, tempKey, SHA256::DIGESTSIZE);  
      }  
      delete[] tempKey;  
      memcpy(out, temp, keyLength);  
 }  

There is no attempt to detect or prevent replay attacks, and except for GCM mode, there is no authentication or data integrity guarantees (which you really want in something that's accepting keyboard commands).

The developers seem to be aware of how bad their crypto is, but for some reason refuse to fix or disable it.

Lessions learned:

• Never invent your own network encryption protocols. Even professional cryptographers can't get it right the first time (SSL 3.0, TLS 1.0). Always use a good TLS library.
• Always generate a random IV at the time of encryption.
• Don't use a key for anything other than a key. All other uses will leak information about it.
• It's better to have no encryption than to have broken encryption with a false sense of security.

CodeIgniter: Encryption is Not Authentication

After encrypting a string, the CodeIgniter PHP framework applies this function to the ciphertext. The function is a shift cipher using a hash of the encryption key.

The comment says they are doing it "to protect against Man-in-the-middle attacks on CBC mode ciphers." I have no idea what that means. I can only guess that they're slightly aware of the fact that encryption is not authentication, and that attackers can modify a CBC mode ciphertext to decrypt the way they want.

 /**  
  * Adds permuted noise to the IV + encrypted data to protect  
  * against Man-in-the-middle attacks on CBC mode ciphers  
  * http://www.ciphersbyritter.com/GLOSSARY.HTM#IV  
  *  
  * @param     string  
  * @param     string  
  * @return     string  
  */  
 protected function _add_cipher_noise($data, $key)  
 {  
      $key = $this->hash($key);  
      $str = '';  
   
      for ($i = 0, $j = 0, $ld = strlen($data), $lk = strlen($key); $i < $ld; ++$i, ++$j)  
      {  
           if ($j >= $lk)  
           {  
                $j = 0;  
           }  
   
           $str .= chr((ord($data[$i]) + ord($key[$j])) % 256);  
      }  
     
   return $str;  
 }  

Lessons Learned:

• If you need to detect malicious changes to ciphertexts, use HMAC over the IV and ciphertext, or an authenticating mode like OCB.
• If you find a vulnerability, don't come up with your own way to solve it. Find out how cryptographers have already solved it, and do it the right way.

CodeIgniter: Another rand() Stream Cipher

When the mcrypt functions are missing, the CodeIgniter PHP framework falls back to a homebrew stream cipher it calls "xor_encode."

 protected function _xor_encode($string, $key)  
 {  
      $rand = '';  
      do  
      {  
           $rand .= mt_rand();  
      }  
      while (strlen($rand) < 32);  
   
      $rand = $this->hash($rand);  
   
      $enc = '';  
      for ($i = 0, $ls = strlen($string), $lr = strlen($rand); $i < $ls; $i++)  
      {  
           $enc .= $rand[($i % $lr)].($rand[($i % $lr)] ^ $string[$i]);  
      }  
   
      return $this->_xor_merge($enc, $key);  
 }  
   
 protected function _xor_merge($string, $key)  
 {  
      $hash = $this->hash($key);  
      $str = '';  
      for ($i = 0, $ls = strlen($string), $lh = strlen($hash); $i < $ls; $i++)  
      {  
           $str .= $string[$i] ^ $hash[($i % $lh)];  
      }  
   
      return $str;  
 }  

Lessons Learned:

• Don't invent your own cipher.
• If your cryptography library is missing, don't encrypt! If you really need to fall back, include a well-tested implementation of a good cipher.

Myself: Using the Same Key to Encrypt Everything

This is one of my own mistakes. I saw that a lot of PHP encryption code is deeply flawed, so I wrote my own PHP encryption class and released it to the world. Little did I know, it was even more flawed than CakePHP's rand() cipher.

The problem was in the way I split the key given by the user into two keys: one for encryption, and the other for authentication. I did this by HMACing a purpose string (either "encryption" or "authentication") with the key. That works, but I screwed up the implementation. I didn't include the user's key in the call to hash_hmac(), and because PHP doesn't do type checking, it blindly accepted the next parameter, boolean "true", as the key.

Because of this mistake, everything was encrypted with the same key. Really bad.

Here's the diff of the fix:

  --- /tmp/Crypto.php   2013-05-02 06:39:28.059745329 -0600  
 +++ source/Crypto.php  2013-05-02 07:05:16.959752519 -0600  
 @@ -116,7 +116,7 @@  
    */  
    public static function CreateSubkey($master, $purpose, $bytes)  
    {  
 -    $source = hash_hmac("sha512", $purpose, true);  
 +    $source = hash_hmac("sha512", $purpose, $master, true);  
      if(strlen($source) < $bytes) {  
        trigger_error("Subkey too big.", E_USER_ERROR);  
        return $source; // fail safe  
 @@ -168,6 +168,16 @@  
        return false;  
      }  
 +    $key = mcrypt_create_iv(16, MCRYPT_DEV_URANDOM);  
 +    $data = "abcdef";  
 +    $ciphertext = Crypto::Encrypt($data, $key);  
 +    $wrong_key = mcrypt_create_iv(16, MCRYPT_DEV_URANDOM);  
 +    if (Crypto::Decrypt($ciphertext, $wrong_key))  
 +    {  
 +      echo "FAIL: Ciphertext decrypts with an incorrect key.";  
 +      return false;  
 +    }  
 +  
      echo "PASS\n";  
      return true;  
    }   

Lessons learned:

• Always write test suites for your encryption code. Always test that you can't decrypt with the wrong key.

CakePHP: Using the IV as the Key

A previous post highlighted CakePHP's abysmal homebrew cipher. They realized it was weak and attempted to fix it by using the Rijdnael cipher instead. They even used CBC mode. But instead of generating a random IV for every encryption, they used the key as the IV. This is really bad, because you can easily recover the key with a chosen ciphertext or chosen plaintext attack.

They realized that was a bad idea, and tried to switch to randomly-generated IVs in a backwards compatible way. Unfortunately, the way they did it makes 1 in every  2¹⁶ of the old ciphertexts undecryptable.

They are also using zero-byte padding, just stripping the trailing zero bytes after decryption. This is ambiguous: If the original message ends in zero bytes, they will be stripped off after the decryption, and that part of the message will be lost.

 public static function rijndael($text, $key, $operation) {  
      if (empty($key)) {  
           trigger_error(__d('cake_dev', 'You cannot use an empty key for Security::rijndael()'), E_USER_WARNING);  
           return '';  
      }  
      if (empty($operation) || !in_array($operation, array('encrypt', 'decrypt'))) {  
           trigger_error(__d('cake_dev', 'You must specify the operation for Security::rijndael(), either encrypt or decrypt'), E_USER_WARNING);  
           return '';  
      }  
      if (strlen($key) < 32) {  
           trigger_error(__d('cake_dev', 'You must use a key larger than 32 bytes for Security::rijndael()'), E_USER_WARNING);  
           return '';  
      }  
      $algorithm = MCRYPT_RIJNDAEL_256;  
      $mode = MCRYPT_MODE_CBC;  
      $ivSize = mcrypt_get_iv_size($algorithm, $mode);  
      $cryptKey = substr($key, 0, 32);  
      if ($operation === 'encrypt') {  
           $iv = mcrypt_create_iv($ivSize, MCRYPT_RAND);  
           return $iv . '$$' . mcrypt_encrypt($algorithm, $cryptKey, $text, $mode, $iv);  
      }  
      // Backwards compatible decrypt with fixed iv  
      if (substr($text, $ivSize, 2) !== '$$') {  
           $iv = substr($key, strlen($key) - 32, 32);  
           return rtrim(mcrypt_decrypt($algorithm, $cryptKey, $text, $mode, $iv), "\0");  
      }  
      $iv = substr($text, 0, $ivSize);  
      $text = substr($text, $ivSize + 2);  
      return rtrim(mcrypt_decrypt($algorithm, $cryptKey, $text, $mode, $iv), "\0");  
 }  

Lessons learned:

• Always use unambiguous encodings, i.e. don't rely on low probability of random data containing your separator, and use unambiguous padding.
• Don't use the key as anything other than the key. If you do, you're almost certainly leaking information about it.
• Always use an IV that is randomly generated (from a secure CSPRNG) at the time of encryption.

CakePHP: Using rand() to Build a Stream Cipher

The following function can be found in the CakePHP framework. It's marked as deprecated, and it's going to be removed in a future version, but it's pretty bad:

  public static function cipher($text, $key) {  
      if (empty($key)) {  
           trigger_error(__d('cake_dev', 'You cannot use an empty key for Security::cipher()'), E_USER_WARNING);  
           return '';  
      }  
      srand(Configure::read('Security.cipherSeed'));  
      $out = '';  
      $keyLength = strlen($key);  
      for ($i = 0, $textLength = strlen($text); $i < $textLength; $i++) {  
           $j = ord(substr($key, $i % $keyLength, 1));  
           while ($j--) {  
                rand(0, 255);  
           }  
           $mask = rand(0, 255);  
           $out .= chr(ord(substr($text, $i, 1)) ^ $mask);  
      }  
      srand();  
      return $out;  
 }  

Lesson learned:

• Don't invent your own cipher. Use a well-known cipher that has been tested and analyzed extensively by experts, like AES, Serpent, or Twofish.