Today we’re going to be talking about this code, shared on Twitter by Paul Moore.

It’s worth noting that this code snippet was after Paul attempted to alert them to security issues with the previous iteration of their encryption software, which looked like this:

If you’re interested in a deeper dive into the insecurity of their software, Paul published a series of posts about this topic.

• CyberAlarm: An independent security review… and why you should avoid it
• CyberAlarm: Testing the “production version”… and why you should avoid it
• Police CyberAlarm: Absymal security, yet again

I’m not going to be as harsh as Paul–not out of any particular sentiment towards law enforcement, but because there’s enough vitriol in the security industry. I don’t feel like joining the incumbent tone or risk making a neophyte’s impostor syndrome worse than it already is.

The Code in Question

I have transcribed their source code from the screenshot on Twitter below.

<?php

function pervade_encrypt($data)
{
    $encryption_key = (defined('NEW_PERVADE_KEY') ? NEW_PERVADE_KEY : PERVADE_KEY);
    $iv = openssl_random_pseudo_bytes(openssl_cipher_iv_length('aes-256-cbc'), $csprng_check);

    while((strpos($iv, '::') !== false) || !$csprng_check) {
        $iv = openssl_random_pseudo_bytes(openssl_cipher_iv_length('aes-256-cbc'), $csprng_check);
    }

    $encrypted = openssl_encrypt($data, 'aes-256-cbc', $encryption_key, 0, $iv);
    $length = strlen($encrypted);
    $hash = hash_hmac('sha256', $encrypted . $iv, $encryption_key);
    return base64_encode($encrypted . '::' . $length . '-' . $hash . '::' . $iv);
}

function pervade_decrypt($data)
{
    $encryption_key = (defined('NEW_PERVADE_KEY') ? NEW_PERVADE_KEY : PERVADE_KEY);
    $exp = explode('::', base64_decode($data), 3);

    if (isset($exp[2])) {
        $encrypted_data = $exp[0];
        $length = $exp[1];
        $iv = $exp[2];
        $lenexp = explode('-', $length);

        if (isset($lenexp[1])) {
            $hmac = str_replace($lenexp[0] . '-', '', $length);
            $length = $lenexp[0];
        }
        else {
            $hmac = hash_hmac('sha256', $encrypted_data . $iv, $encryption_key);
        }

        $hashcheck = hash_hmac('sha256', $encrypted_data . $iv, $encryption_key);
        if ((strlen($encrypted_data) == $length) && ($hmac == $hashcheck)) {
            return @openssl_decrypt($encrypted_data, 'aes-256-cbc', $encryption_key, 0, $iv);
        }
    }
    else if (isset($exp[1])) {
        $encrypted_data = $exp[0];
        $iv = $exp[1];
        return @openssl_decrypt($encrypted_data, 'aes-256-cbc', $encryption_key, 0, $iv);
    }
}

If you’re interested in Paul’s challenge, take a moment to read this PHP code carefully and see if you can spot the flaws. There’s more than one.

How to Review Cryptographic Protocols

Whenever you are confronted with a novel cryptographic implementation (be it a custom protocol or a greenfield implementation of a standard design), always start with the reader, not the writer.

• With encryption, this means starting with the decrypt function.
• With digital signatures or symmetric authenticators, this means starting with the verify function.

The reasoning is simple: In most threat models, attackers have control over the data being fed into the reader. This lets them perform far more impactful attacks (e.g. padding oracle attacks) than passing information to the writer would reveal (i.e. chosen-plaintext attacks, which overwhelmingly aren’t relevant for protocols that use standard cryptographic algorithms; i.e. AES).

In this case, the pervade_decrypt() function is clearly written in order to support a data format migration from the original implementation to the new format.

To their credit, pervade_encrypt() only writes the new format, so they clearly intended to retire the old message format eventually. However, they never took the time to learn the proper way to handle cryptographic migrations.

Vulnerabilities in the Decrypt Function

Downgrade Attack

Take an encrypted message that you’re interested in decrypting.

Zis1Q2FkbFVmYzRJbEMwczc0MWdyTFdvSWtGbWp3dkVMT0d6MVp2Q1lWST06OjQ0LThlODI2ZTFiZDVjZDM1YmYwZmQ5MzlkYmM5NDZlNjk4MzA0ZTIzN2FhMWI0ZTIyOTA4Mzk2MGI0ZjA5MThhOGE6OgVzUPsnsRFbrFySXaWdNt0=

Base64-decode it. Remove the value between the first :: and the second :: (including one of the separators).

Re-encode it with base64, then feed this altered message into the system.

Zis1Q2FkbFVmYzRJbEMwczc0MWdyTFdvSWtGbWp3dkVMT0d6MVp2Q1lWST06OgVzUPsnsRFbrFySXaWdNt0=

Exploit Code

function exploit1(string $chosen): string {
    $pieces = explode('::', base64_decode($chosen));
    unset($pieces[1]);
    return base64_encode(implode('::', $pieces));
}

Now you’ve successfully downgraded the message to the legacy format, which didn’t provide any authentication over the ciphertext.

Update (2022-07-05): Actually, this story is worse than I thought.

HMAC Verification Bypass

Take an encrypted message that you’re interested in decrypting.

Zis1Q2FkbFVmYzRJbEMwczc0MWdyTFdvSWtGbWp3dkVMT0d6MVp2Q1lWST06OjQ0LThlODI2ZTFiZDVjZDM1YmYwZmQ5MzlkYmM5NDZlNjk4MzA0ZTIzN2FhMWI0ZTIyOTA4Mzk2MGI0ZjA5MThhOGE6OgVzUPsnsRFbrFySXaWdNt0=

Base64-decode it. After the first ::, remove the length of the message and the hyphen.

Zis1Q2FkbFVmYzRJbEMwczc0MWdyTFdvSWtGbWp3dkVMT0d6MVp2Q1lWST06OjQ0LThlODI2ZTFiZDVjZDM1YmYwZmQ5MzlkYmM5NDZlNjk4MzA0ZTIzN2FhMWI0ZTIyOTA4Mzk2MGI0ZjA5MThhOGE6OgVzUPsnsRFbrFySXaWdNt0=

Exploit Code

function exploit2(string $chosen): string {
    $pieces = explode('::', base64_decode($chosen));
	$pieces[1] = preg_replace('/^\-+/', '', $pieces[1]);
    return base64_encode(implode('::', $pieces));
}

Why does this work?

Because this will put your verification logic into a separate branch that will compare the HMAC it computes… against the HMAC it computes, rather than the one provided.

$lenexp = explode('-', $length);
// If this isset() call returns FALSE...
if (isset($lenexp[1])) {
    $hmac = str_replace($lenexp[0] . '-', '', $length);
    $length = $lenexp[0];
}
else {
     // ...then the value of $hmac...
     $hmac = hash_hmac('sha256', $encrypted_data . $iv, $encryption_key);
}
// ...will always be equal to $hashcheck
$hashcheck = hash_hmac('sha256', $encrypted_data . $iv, $encryption_key);
if ((strlen($encrypted_data) == $length) && ($hmac == $hashcheck)) {
    return @openssl_decrypt($encrypted_data, 'aes-256-cbc', $encryption_key, 0, $iv);
}

With this method, we can completely bypass the HMAC check without stripping the HMAC off the message. Removing the length prefix is sufficient to defeat this security control.

Padding Oracle Attack on AES-CBC Decryption

Using either of the two methods, with the HMAC check completely bypassed, you’ve reduced the security of this construction to unauthenticated AES-CBC mode, which is vulnerable to a Padding Oracle Attack.

To exploit the padding oracle attack against pervade_decrypt(), you need the ability to provide a modified ciphertext to their application. When a padding error occurs, pervade_decrypt() will return false instead of a string value, so you also need some observable behavioral change (e.g. as simple as a page load time difference) to leak this information back to the attacker.

Timing Attack on HMAC Validation

Not that it matters much, since you can just bypass the security control entirely, but == is not the correct way to compare hash function outputs.

You want hash_equals() instead.

Confused Deputy Attack

Modern encryption-at-rest software allows users to specify some Additional Authenticated Data to prevent ciphertexts from being reordered or replayed in an incorrect context.

The encryption used by Police CyberAlarm doesn’t expose an Additional Authenticated Data mechanism. All fields are also encrypted with the same, static, hard-coded key.

Imagine a situation where a user’s ZIP code and legal name are encrypted with pervade_encrypt(). You, as an attacker, have access to the database, but not to the source code.

You also have a limited access user account that lets you view zip codes in a directory listing, but not legal name.

To exploit this, simply overwrite the user’s zip_code field with their encrypted legal_name field and refresh your directory listing. Bob’s your uncle.

To mitigate confused deputy attacks, an AEAD construction is recommended.

It’s also possible to implement a custom protocol using AES-CBC and HMAC-SHA256 that includes AAD support, but extra care must be taken to prevent canonicalization attacks.

Other Cryptography Design Flaws

Using the Same Key for Multiple Algorithms

Police CyberAlarm uses the same encryption key for both AES-CBC encryption and for HMAC-SHA256. This violates one of the tenets of cryptography protocol design: Never use the same key for more than one purpose.

If you ever fail to uphold this tenet, you introduce the risk of related key and cross-protocol attacks in your application. While this isn’t currently known to be exploitable with AES and HMAC, it is very much exploitable in some setups using AES and CBC-MAC.

Instead of reusing $encryption_key for both openssl_encrypt() and hash_hmac(), the correct thing to do is use a key derivation function (i.e. HKDF) to split the incoming key into two different keys: One for encryption, the other for authentication.

$ikm = (defined('NEW_PERVADE_KEY') ? NEW_PERVADE_KEY : PERVADE_KEY);
$encryption_key = hash_hkdf('sha256', $ikm, 32, 'encryption');
$hmac_key = hash_hkdf('sha256', $ikm, 32, 'hmac');

OpenSSL CSPRNG Check Infinite Loop

If this code sets $csprng_check to false, you will DoS your own application.

$iv = openssl_random_pseudo_bytes(openssl_cipher_iv_length('aes-256-cbc'), $csprng_check);

while((strpos($iv, '::') !== false) || !$csprng_check) {
    $iv = openssl_random_pseudo_bytes(openssl_cipher_iv_length('aes-256-cbc'), $csprng_check);

The reason is subtle: $csprng_check is kind of a useless feature. It’s vestigal to OpenSSL’s RAND_pseudo_bytes() API. In PHP, if the value is ever set to false, it will continue to be false for the duration of the process.

It’s also a superfluous check: If OpenSSL’s RNG is insecure (i.e. the Debian weak key debacle), would you trust OpenSSL to be aware of its insecurity?

If you’re not familiar with PHP’s OpenSSL extension, you might be tempted to say, “You didn’t understand the purpose of this loop! It’s just meant to prevent a separator (::) from appearing in the random IV,” but this is what the code is actually doing:

1. Generate 16 random bytes from OpenSSL’s fork-unsafe API, setting the value of $csprng_check to a boolean (passed by-reference).
2. If the random bytes contain a :: separator -OR- $csprng_check is false: Go back to step 1.

Regardless of what the authors may have intended, the effect of their code is that it’s doing both what I said and what you said.

And since $csprng_check will remain false forever, this creates an infinite loop.

Recommendation

You want random_bytes() here. For PHP 5, paragonie/random_compat.

With random_bytes(), if the RNG fails, an Exception will be thrown. This will fail closed and prevent the application from proceeding with insecure randomness.

Summary

The cryptography used by Police CyberAlarm is not secure and should be replaced with something more secure.

A much better implementation is ext/sodium (PHP 7.2+) or paragonie/sodium_compat (PHP 5.2+) provides the NaCl/libsodium default that other experts recommend. To get started, refer to this Quick Reference page.

Alternatively, defuse/php-encryption supports PHP 5 and provides authenticated encryption.

Update (2022-07-05)

u/disclosure5 points out:

Ahh but if the server supports the new key (defined('NEW_PERVADE_KEY')) it’ll only ever use it, so it can’t actually decrypt legacy messages even if it supports the packing format used with the older key.

The problem with looking at a fractal of incorrectness is that you’re liable to overlook an element of what they got wrong.

They will never use the old key to decrypt data, so any notion of supporting a legacy message format is, therefore, moot. They could have simply enforced the HMAC entirely.