2

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.

hakre
  • 193,403
  • 52
  • 435
  • 836
kwe
  • 37
  • 1
  • 10
  • 1
    aren't you forgetting to url decode? – Denis de Bernardy May 13 '13 at 09:32
  • The value in the querystring is being url decoded correctly. I've compared it with direct output from the encryption function and both values are identical. – kwe May 13 '13 at 09:38
  • Both strings are identical when urlencoded as well. – kwe May 13 '13 at 10:02
  • It is plain wrong using an `IV` with `ECB encoding`, using `ECB` in the first place rather use `CBC`. – Baba May 13 '13 at 10:09
  • Thanks for your suggestion. I'm not very familiar with encryption in php and it's someone else's code I'm debugging so I'll have to read up on it. – kwe May 13 '13 at 10:14
  • 2
    @kwe: don't use encryption then. If you don't understand it (or are not familiar with it), don't use it. End of story. Use a library (like Zend\Crypt). But doing it as you are is dangerous and is leaving a false sense of security... – ircmaxell May 13 '13 at 12:05
  • I'll look into Zend\Crypt then, thanks. – kwe May 14 '13 at 06:56

2 Answers2

4

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;
    }
}
Community
  • 1
  • 1
Baba
  • 94,024
  • 28
  • 166
  • 217
  • Not bad at all. Two minor points (or not so minor points), you should be using different keys for encryption as you do for signing. Also, remove the `utf8_encode` calls, they are just going to screw up the data. Other than that, it looks pretty decent... Oh, and look into PKCS padding instead of just an arbitrary rtrim (which can destroy data)... – ircmaxell May 13 '13 at 12:08
  • I think I'll take your advice on using a tested library, thanks for explaining. I'd upvote if I could (: – kwe May 14 '13 at 06:58
  • @Baba: I've partially moved the encoding out (and in): https://gist.github.com/hakre/723253295fd6d8b602d6 - Can you share why inside the class per the default you have 32 for the keys / signature while in the example you take 16? Is this also based on mode? – hakre May 14 '13 at 14:39
  • Key size should be 16 , 24 for 32 bytes for AES-128, 192 and 256 ,, the idea was to validate the keys and make sure it meets that criteria or set the maximum .. but latter forgot to add that part ... – Baba May 17 '13 at 09:20
1

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;
}
hakre
  • 193,403
  • 52
  • 435
  • 836
Hugo Delsing
  • 13,803
  • 5
  • 45
  • 72
  • Removing the `trim()` around `mcrypt_encrypt(MCRYPT_RIJNDAEL_256, $key, trim($encrypt), MCRYPT_MODE_ECB, $iv);` seems to have solved the issue. I removed all the other trims as well just in case. Thanks for your help! – kwe May 13 '13 at 10:08
  • In my first test I only had to remove the trim around `$decoded` and in another I only needed to remove the one you mentioned. So removing all is probably the best solution :) – Hugo Delsing May 13 '13 at 10:10
  • Didnt even look at that. He asked why his code wasnt working, it was because of the `trim`, not because of ECB. – Hugo Delsing May 13 '13 at 12:08
  • 1
    Its even in the PHP manual (in spanish, but still) :) But I get your point. Its a good idea to atleast comment on certain parts. So thanks for pointing it out. – Hugo Delsing May 13 '13 at 12:23