2
$\begingroup$

I’m porting over some crypto code from Mcrypt (deprecated) to OpenSSL. I’m following a process that a former employee implemented, which is as follows:

  1. Create a random salt for use in a PBKDF2 key derivation function.

  2. Create a random IV for symmetric AES-CBC encryption.

  3. Encrypt the plaintext using the derived key from step 1 and the IV from step 2.

  4. Create an HMAC of the token (concatenated IV + salt + ciphertext).

  5. Append the HMAC from step 4 to the token.

Is this process acceptable (if implemented properly), or does it have some structural weakness that leaves it vulnerable to attacks?


Ps. Here's my actual PHP code. I realize this is sort of a large chunk of code to digest, but I hope that it may help fill in any relevant details that I may have left out from the high-level description above.

final class SymmetricEncryption
{
    /**
     * @const int
     */
    private const SALT_LENGTH_IN_BYTES = 32;

    /**
     * @const int
     */
    private const IV_LENGTH_IN_BYTES = 16;

    /**
     * @const int
     */
    private const KEY_LENGTH_IN_BYTES = 32;

    /**
     * @const int
     */
    private const HMAC_LENGTH_IN_BYTES = 64;

    /**
     * @const string
     */
    private const SYMMETRIC_ALGORITHM = "aes-256-cbc";

    /**
     * @const string
     */
    private const PBKDF2_ALGORITHM = "sha256";

    /**
     * @const string
     */
    private const HMAC_ALGORITHM = "sha256";

    /**
     * Default number of iterations used by the hash_pbkdf2 function to derive a key
     *
     * @var int
     */
    private $pbkdf2_iterations = 10000;

    /**
     * Symmetric key used to encrypt/decrypt using SYMMETRIC_ALGORITHM.
     * Must be a 32-bit key
     *
     * @var string
     */
    private $symmetric_key;

    public function __construct($symmetric_key, $pbkdf2_iterations = 10000)
    {
        $this->symmetric_key     = $symmetric_key;
        $this->pbkdf2_iterations = $pbkdf2_iterations;
    }

    /**
     * Steps:
     *
     * 1) Compute a random salt for the pbkdf2 key derivation function
     * 2) Create a random IV for the aes encryption
     * 3) Derive a key via pbkdf2
     * 4) Encrypt the plaintext with the derived key from step 3
     * 5) Concatenate the iv, salt, and ciphertext in hexadecimal format.
     * 6) Compute an hmac of the token created in step 5
     * 7) Concatenate the token with the hmac
     *
     * @param $plainText
     * @return string
     */
    public function encrypt($plainText)
    {
        $salt          = openssl_random_pseudo_bytes(self::SALT_LENGTH_IN_BYTES);
        $salt_hex      = bin2hex($salt);
        $iv            = openssl_random_pseudo_bytes(self::IV_LENGTH_IN_BYTES);
        $iv_hex        = bin2hex($iv);
        $key           = $this->computePbkdf2KeyDerivation($this->symmetric_key, $salt);
        $cipher        = openssl_encrypt($plainText, self::SYMMETRIC_ALGORITHM, $key, OPENSSL_RAW_DATA, $iv);
        $token         = $iv_hex . $salt_hex . bin2hex($cipher);
        $hmac_of_token = hash_hmac(self::HMAC_ALGORITHM, $token, $key);

        return $token . $hmac_of_token;
    }

    /**
     * Steps:
     *
     * 1) Extract the iv, salt, cipher, and hmac from the token. Confirm that they are each valid strings.
     * 2) Compute the pbkdf2 key derivation function with the symmetric key and extracted salt (binary)
     * 3) Compute the hmac of the token using the derived key from step 2
     * 4) If the computed hmac matches the expected hmac, decrypt the cipher (binary), otherwise throw an exception.
     *
     * @param $token
     * @return string
     * @throws InvalidTokenException
     */
    public function decrypt($token)
    {
        $iv_length     = self::IV_LENGTH_IN_BYTES * 2;
        $salt_length   = self::SALT_LENGTH_IN_BYTES * 2;
        $cipher_length = strlen($token) - $iv_length - $salt_length - self::HMAC_LENGTH_IN_BYTES;
        //Extract the iv, salt, cipher, and expected hmac from the token
        $iv_hex        = substr($token, 0, $iv_length);
        $salt_hex      = substr($token, $iv_length, $salt_length);
        $cipher_hex    = substr($token, $iv_length + $salt_length, $cipher_length);
        $expected_hmac = substr($token, self::HMAC_LENGTH_IN_BYTES * -1, self::HMAC_LENGTH_IN_BYTES);
        if(!$iv_hex || !$salt_hex || !$cipher_hex || !$expected_hmac) {
            throw new InvalidTokenException("Invalid token");
        }
        //Convert hexadecimal representations of the iv, salt, and cipher into binary
        $iv_binary     = hex2bin($iv_hex);
        $salt_binary   = hex2bin($salt_hex);
        $cipher_binary = hex2bin($cipher_hex);
        //Derive a key given the salt that we found in the string and compute an hmac value
        $derived_key        = $this->computePbkdf2KeyDerivation($this->symmetric_key, $salt_binary);
        $token_without_hmac = substr($token, 0, strlen($token) - self::HMAC_LENGTH_IN_BYTES);
        $computed_hmac      = hash_hmac(self::HMAC_ALGORITHM, $token_without_hmac, $derived_key);
        //If the expected hmac matches the computed hmac decrypt the data
        if(strcmp($expected_hmac, $computed_hmac) === 0) {
            return openssl_decrypt(
                $cipher_binary,
                self::SYMMETRIC_ALGORITHM,
                $derived_key,
                OPENSSL_RAW_DATA,
                $iv_binary
            );
        }

        throw new InvalidTokenException("Invalid hmac");
    }

    /**
     * The key and salt should both be binary strings.
     *
     *
     * @link http://php.net/manual/en/function.hash-pbkdf2.php
     * @param $key
     * @param $salt
     * @return string - binary encoded string
     */
    private function computePbkdf2KeyDerivation($key, $salt) {
        return hash_pbkdf2(
            self::PBKDF2_ALGORITHM,
            $key,
            $salt,
            $this->pbkdf2_iterations,
            self::KEY_LENGTH_IN_BYTES,
            true
        );
    }
}
$\endgroup$
  • $\begingroup$ MarkDown has specific formatting to create (un-) numbered lists (also available as button). I've definitely seen worse code, great that you're moving away from mcrypt. $\endgroup$ – Maarten Bodewes May 31 '18 at 1:33
  • $\begingroup$ Code review is considered off topic for this exchange even when the code involves cryptography. If you want to know if some construct is secure then you can ask a question about that. Then you should replace code with a precise statement of what algorithms are being used and the specifics of how they're used. $\endgroup$ – Future Security May 31 '18 at 2:10
  • $\begingroup$ The crypto part of this code looks really hack-y. Normally you use one key to encrypt many messages and use a different IV for each message. It is unnecessary (but acceptable) to derive a new key for each one for typical algorithms. What's extra sketchy is the KDF use. 1st you shouldn't use a 32 bit key even if you stretch it. 2nd you shouldn't re-derive a key (new or not) from a password. If your time cost is T and you derive 2 keys, you use 2T time and each key has T brute-force resistance. Instead derive one key in 2T time and use the master key to derive new keys (without more stretching) $\endgroup$ – Future Security May 31 '18 at 2:26
  • $\begingroup$ If you look into your actual requirements then you'll find that the former employee didn't implement the best construction regardless of how the algorithms are implemented or what libraries are used. $\endgroup$ – Future Security May 31 '18 at 2:28
  • $\begingroup$ @FutureSecurity Are you suggesting that I rely on the master key rather than the derived key for the AES encryption? Is the point there simply that using a derived key is unnecessary? The 32-bit is a typo and should read 32-bytes (256 bit) for the key size. $\endgroup$ – Robert May 31 '18 at 19:29
4
$\begingroup$

Is transporting a salt, iv, cipher and hmac in an encrypted token an acceptable practice?

No, but that's not what is taking place. You are transporting a salt, IV, cipher(text) and HMAC over the former, and that's normal practice for encrypt-then-MAC. The salt doesn't really need to be authenticated because any alteration would also change the HMAC key, but that's OK.


Code comments

private $pbkdf2_iterations = 10000;

This should be configurable somehow, 10K iterations is generally thought to be on the low side nowadays (this is a moving target, which is why the number of iterations is often stored or derived from information with the password hash or ciphertext).

...
/* Must be a 32-bit key
...
private $symmetric_key;

Not really, must be password < 64 bytes after encoding.


Other notes

Hexadecimals are relatively inefficient. Why not first concatenate and then base64 encode?

Using the key for encryption and HMAC is not considered secure in a strictly theoretical sense. If somebody was to use e.g. AES-CBC-MAC instead of HMAC then you may run into trouble.

The salt size is relatively large, 16 bytes should suffice in case you need the space.

| improve this answer | |
$\endgroup$
  • $\begingroup$ Thank you for the quality answer. I think you're spot on that I should use be base64 encoding. The main reason I went with hex was A) consistency with the previous library, and B) predictable encoding size. Use of the same key for AES and MAC came up in an internal code review, and I'm going to fix that today. The 32-bit comment is a typo and should read 32 bytes. I also updated the default number of iterations to 25,000 and it's configurable through the class' constructor. $\endgroup$ – Robert May 31 '18 at 19:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.