kwe
kwe

Reputation: 37

PHP decryption fails on some strings with trim()'s

I'm having trouble decrypting values that end with %3D%3D. Upon decrypting I get a return value that's completely illegible. The encrypted value is being passed via the querystring, but I've run a test looping through values 0 to 200 to rule out problems with url encoding.

Encryption and decryption functions:

function encryptValue($encrypt) {
    $key = variable_get_local("privateKey", $default = "");
    $iv = mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB),   MCRYPT_RAND);
    $passcrypt = trim(mcrypt_encrypt(MCRYPT_RIJNDAEL_256, $key, trim($encrypt), MCRYPT_MODE_ECB, $iv));
    $encode = urlencode(base64_encode($passcrypt));
    return $encode;
}


function decryptValue($decrypt) {
    $key = variable_get_local("privateKey", $default = "");
    $decoded = base64_decode(urldecode($decrypt));
    $iv = mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB), MCRYPT_RAND);
    $decrypted = trim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256, $key, trim($decoded), MCRYPT_MODE_ECB, $iv));
    return $decrypted;
}

I've tried keeping the iv value identical across encryption and decryption but this doesn't change the output. I've also tried removing the trim() around trim($decoded) but that didn't change anything either.

Below is what I used to identify the problem. Between 0 and 200 the encryption will produce a value ending on %3D%3D 9 times and result in the decryption failing.

for($i=0;$i<200;$i++) {
    echo encryptValue($i) . "<br/>";
    echo decryptValue(encryptValue($i)) . "<br/><hr/>";
}

Thanks for reading.

Upvotes: 2

Views: 1442

Answers (2)

Baba
Baba

Reputation: 95151

Here are some observations in the current script :

  • The ECB mode is not secure use CBC instead
  • MCRYPT_RAND is just the same as rand see str_shuffle and randomness use MCRYPT_DEV_URANDOM instead
  • For better security use encryption + authentication using HMAC to prevent prevent oracle padding attack
  • Use a proper tested library as well Zend\Crypt rather than create yours sine you are not a security expert

Example :

// Encription Key
$encryptionKey = mcrypt_create_iv(16, MCRYPT_DEV_URANDOM); // Stored securely

// Signature Key
$signatureKey = mcrypt_create_iv(16, MCRYPT_DEV_URANDOM); // Stored securely

// Start DataEncryption Object
$obj = new DataEncryption($encryptionKey, $signatureKey);
$obj->setEncoding(DataEncryption::ENCODE_BASE64);

// Test
for($i = 0; $i < 200; $i ++) {
    printf("%s = %s\n", $encode = $obj->encrypt($i), $obj->decrypt($encode));
}

Output

eSCknmsoMHY2oo5lpW3NpQhDigs4+Fw8aObeIhK+wPyUImQbvlh/aUrW = 0 
qFswb3VO5+Foi4kjVn6s5lpZbiWgdKmfObh37/xjPyqB4ZFfAXNUeYYX = 1 
WKG0BCKUxOXWU6S3YJ/dNL46Lcn7lt+ihG4tEoZuORDoJXSjz6Vrcepn = 2 
K24QqkGYC86btzGQ5HKKMewVhiEIdKOajpgLx8SMKVKfCwlOJlbRwpaz = 3 
0DbJycPZ24FOAhQrhQJmgMsP0p2nFzYUlFVOFlbQ8zhLTXkcdnNOhVfi = 4 
l7saQG2BTPAZR2EnYjxfNmTxEBBaAh+n9+8eOCITDGzEVShw9wOxP7Pt = 5 
eUhvvHJOFsy6ZaBu40XgU+N5VtuFBesRVx0ryfManIXva5y7J0ShiKcE = 6 
TaX+172N60X1UTVmMWYcdcn7YzN7xoAOVEPpaD7r1pE3OtX5Erg4nja8 = 7 
0LM3W0pkQ73IsmqAgRiQvqL0/rdkk7YvuwcVwoe1NI+qZo7Jq8gyFIvn = 8 
....
38m8fEoUhoTyPPBukg3KVhrmwVDyVCcnWx/5erAslUDzEP7Bddzj5y8Y = 196 
Dwi6t7sX30bxjbVXMKCWEZs0FxTUZM4IPHKR3VD6kygi7op0Q6ARCZJW = 197 
TJ/faDaIuE0mDPHmGar1BeIyAnfVD0Z47ZtCcHjz5AZzaQ1YWH8kF1bU = 198 
FYh+8Kts4ubVvTT5o0vZYfKC+8ExhpD5pWgHK3EhvGWkcPwKerSIvkK0 = 199

Class Used

class DataEncryption {
    private $keyEncryption;
    private $keySignature;
    private $ivSize;
    private $cipher = MCRYPT_RIJNDAEL_128;
    private $mode = MCRYPT_MODE_CBC;
    private $signatureLength = 10;
    private $encoding = 2; // I prefer hex
    const ENCODE_BASE64 = 1;
    const ENCODE_HEX = 2;

    function __construct($encryptionKey = null, $signatureKey = null) {
        // Set Keys
        $this->keyEncryption = empty($encryptionKey) ? mcrypt_create_iv(32, MCRYPT_DEV_URANDOM) : $encryptionKey;
        $this->keySignature = empty($signatureKey) ? mcrypt_create_iv(32, MCRYPT_DEV_URANDOM) : $signatureKey;

        // Get IV Size
        $this->ivSize = mcrypt_get_iv_size($this->cipher, $this->mode);
    }

    public function getKeys() {
        return array(
                "encryption" => $this->keyEncryption,
                "signature" => $this->keySignature
        );
    }

    public function setMode($mode) {
        $this->mode = $mode;
    }

    public function setCipher($cipher) {
        $this->cipher = $cipher;
    }

    public function setEncoding($encode) {
        $this->encoding = $encode;
    }

    public function encrypt($data) {

        // add PKCS7 padding to data
        $block = mcrypt_get_block_size($this->cipher, $this->mode);
        $pad = $block - (strlen($data) % $block);
        $data .= str_repeat(chr($pad), $pad);

        $iv = $this->rand($this->ivSize);
        $cipherData = mcrypt_encrypt($this->cipher, $this->keyEncryption, $data, $this->mode, $iv);
        $finalData = $iv . $cipherData;

        // protected against padding oracle attacks
        $finalData = $this->sign($finalData) . $finalData;
        return $this->encode($finalData);
    }

    public function decrypt($data) {
        $data = $this->decode($data);
        // Check Integrity
        if (! $this->check($data)) {
            return false;
        }
        $data = substr($data, $this->signatureLength);

        // Break Data
        $iv = substr($data, 0, $this->ivSize);
        $cipherData = substr($data, $this->ivSize);
        $data = mcrypt_decrypt($this->cipher, $this->keyEncryption, $cipherData, $this->mode, $iv);

        // Remove PKCS7 padding
        $block = mcrypt_get_block_size($this->cipher, $this->mode);
        $pad = ord($data[($len = strlen($data)) - 1]);

        // $data = rtrim($data, "\0..\32");
        return substr($data, 0, $len - $pad);
    }

    public function encode($data) {
        return $this->encoding === self::ENCODE_BASE64 ? base64_encode($data) : bin2hex($data);
    }

    public function decode($data) {
        return $this->encoding === self::ENCODE_BASE64 ? base64_decode($data) : pack("H*", $data);
    }

    public function sign($data) {
        $hash = hash_hmac('sha256', $data, $this->keySignature, true);
        return substr($hash, 0, $this->signatureLength);
    }

    public function check($data) {
        $signature = substr($data, 0, $this->signatureLength);
        $data = substr($data, $this->signatureLength);
        $hash = hash_hmac('sha256', $data, $this->keySignature, true);
        // return $signature === substr($hash, 0, $this->signatureLength);

        return $this->compare($signature, substr($hash, 0, $this->signatureLength));
    }

    public function rand($no) {
        return mcrypt_create_iv($no, MCRYPT_DEV_URANDOM);
    }

    /**
     * Prevent Timing Attacks
     * @param string $a
     * @param string $b
     * @return boolean
     */
    public function compare($a, $b) {
        if (strlen($a) !== strlen($b)) {
            return false;
        }
        $result = 0;
        for($i = 0; $i < strlen($a); $i ++) {
            $result |= ord($a[$i]) ^ ord($b[$i]);
        }
        return $result == 0;
    }
}

Upvotes: 4

Hugo Delsing
Hugo Delsing

Reputation: 14173

Your problem is with all the trim function, that shouldnt be used. You dont trim encoded data as data will be removed, could be vital to the key as you have noticed.

It works like this:

function encryptValue($encrypt) {
    $key = variable_get_local("privateKey", $default = "");
    $iv = mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB),   MCRYPT_RAND);
    $passcrypt = mcrypt_encrypt(MCRYPT_RIJNDAEL_256, $key, trim($encrypt), MCRYPT_MODE_ECB, $iv);
    $encode = urlencode(base64_encode($passcrypt));
    return $encode;
}


function decryptValue($decrypt) {
    $key = variable_get_local("privateKey", $default = "");
    $decoded = base64_decode(urldecode($decrypt));
    $iv = mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB), MCRYPT_RAND);
    $decrypted = mcrypt_decrypt(MCRYPT_RIJNDAEL_256, $key, $decoded, MCRYPT_MODE_ECB, $iv);
    return $decrypted;
}

Upvotes: 1

Related Questions