2
\$\begingroup\$

I have written the below class which encrypts and decrypts a PHP array. The encrypted data and the hash are sent over an insecure connection and I would appreciate if you could check the code for any vulnerabilities I may have overlooked.

class ArrayEncryption
{
    const encryptionAlgorithm = 'aes-256-cbc';
    const hashAlgorithm = 'sha256';
    const replayTime = 10;

    /**
     * Function packData
     *
     * @param array $data - the data array which shall be encrypted
     * @param $hashKey - applicable hash key
     * @param $encryptionKey - applicable encryption key
     * @param $encryptionIv - applicable encryption iv
     * @return array - contains encrypted data and a hash
     */
    public static function packData(array $data, $hashKey, $encryptionKey, $encryptionIv)
    {
        // in addition to the data, always transfer a timestamp
        $data = array_merge(array('timestamp' => time()), $data);
        // convert the array to json
        $jsonData = json_encode($data);
        // encrypt the data (and base64 encode)
        $encryptedData = base64_encode(openssl_encrypt($jsonData, self::encryptionAlgorithm, $encryptionKey, 0, $encryptionIv));
        // create a hash key for the data
        $hash = hash_hmac(self::hashAlgorithm, $encryptedData, $hashKey, false);
        // return the encrypted data and hash
        return array(
            'encryptedData' => $encryptedData,
            'hash'          => $hash,
        );
    }

    /**
     * Function verifyUnpackData
     *
     * @param $encryptedData - encrypted data which has been received, must contain a timestamp
     * @param $hash - hash for encrypted data which has been received
     * @param $hashKey - applicable hash key
     * @param $encryptionKey - applicable encryption key
     * @param $encryptionIv - applicable encryption iv
     * @param bool|true $timeOut - if true, check for timeouts / replay attacks
     * @return array - the originally encrypted data
     */
    public static function verifyUnpackData($encryptedData, $hash, $hashKey, $encryptionKey, $encryptionIv, $timeOut = true)
    {
        // create hmac hash from encrypted data and the given hash key
        $checkValue = hash_hmac(self::$hashAlgorithm, $encryptedData, $hashKey, false);
        // check whether the hmac hash matches with the hash provided
        if (!hash_equals($checkValue, $hash)) {
            throw new \Exception('Hash check failed!', 73100);
        }
        // decrypt and decode all data
        $data = openssl_decrypt(base64_decode($encryptedData), self::$encryptionAlgorithm, $encryptionKey, 0, $encryptionIv);
        $data = json_decode($data, true);
        if (json_last_error() != JSON_ERROR_NONE) {
            throw new \Exception('Invalid data decrypted / JSON decode failed!', 73101);
        }
        // verify that an array has been received
        if (!is_array($data)) {
            throw new \Exception('Decrypted data is not an array!', 73102);
        }
        // check for timeout / prevent replay attacks
        if ($timeOut) {
            if (!isset($data['timestamp'])) {
                throw new \Exception('Timeout check failed, no timestamp available!', 73103);
            } elseif (time() - $data['timestamp'] > self::$replayTime) {
                throw new \Exception('Timeout, possibly replay attack!', 73104);
            }
        }
        // return the data
        return $data;
    }
}

/**
 * Function hash_equals
 *
 * This function checks whether the provided strings are equal byte by byte
 * TODO : built-in hash_equals included in PHP 5.6
 *
 * @param $str1 - string one for comparison
 * @param $str2 - string two for comparison
 * @return bool - true if equal, false otherwise
 */
function hash_equals($str1, $str2)
{
    if (strlen($str1) != strlen($str2)) {
        return false;
    } else {
        $res = $str1 ^ $str2;
        $ret = 0;
        for ($i = strlen($res) - 1; $i >= 0; $i--) {
            $ret |= ord($res[$i]);
        }
        return !$ret;
    }
}

Note: No other data than the encrypted data and the hash shall be sent.

\$\endgroup\$
3
\$\begingroup\$

First, a note on style. When I write encryption code, I like to encapsulate as much crypto-related logic inside the encrypt and decrypt functions (pack/unpack in your case), including things like IV generation. Keeping crypto functionality in one place is helpful, since crypto code is tricky, tends to be interdependent, and needs careful review.

I notice that you're using AES-CBC with HMAC-SHA256. That's fine.

I see that you're using two keys, one for encryption and one for the MAC. There's no key generation code included, so I'll assume that you're generating the keys separately and appropriately.

Note: No other data than the encrypted data and the hash shall be sent.

I think you forgot to mention: you also need to transmit the IV. Typically, the IV would be transmitted alongside the ciphertext ($encryptedData) and MAC ($hash), otherwise you cannot decrypt at the other end. This presents a problem: an attacker can modify the IV while it's in transit! In most settings, this is a "game over" type of vulnerability, since it allows the attacker to tamper with part of the message, and potentially even discover the plaintext.

The solution here is to concatenate the IV in front of $encryptedData before calculating the MAC. Since the IV will be included in the MAC, you can be sure that it can't be tampered with.

The IV generation code isn't included so I'll just mention: Be sure to randomly generate the IV using a secure random number generator, using a new IV every time you encrypt.

const $replayTime = 10;
// ... code omitted here ...
} elseif (time() - $data['timestamp'] > self::$replayTime) {
    throw new \Exception('Timeout, possibly replay attack!', 73104);
}

In most scenarios, it's unlikely that this would prevent a replay attack (10 seconds is plenty of time for an attacker to act). A better way of preventing replay attacks is to give each encrypted message an ID (1, 2, 3..), and keep track of the last ID received. If an encrypted message arrives with an ID <= the last ID received, then reject it. The MAC must cover the message ID as well to prevent an attacker from tampering with it. That said -- make sure that you're not reinventing TLS here.

As you can see, cryptography gets complicated. I strongly recommend using an encryption library like php-encryption if possible. On the other hand, if you're just here to learn, that's awesome.

\$\endgroup\$

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.