MCRYPT - are there any flaws or areas for improvement in this class?

Question

I am working on a class for encryption to use on my site. I have read through many examples of these functions and would just like to clarify a few points I have read and check if this code is worthy. It is for storing sensitive information in a database. I am aware that making this method secure does not mean that the information is 100% secure, I simply need an appraisal of the code.

<?php

class Encryption
{
    private $eMethod = MCRYPT_RIJNDAEL_128;
    private $siteKey = "01234567890123456789012345678901"; # strlen = 32
    private $ivSize;
    private $iv;

    public function __construct()
    {
        $this->ivSize     = mcrypt_get_iv_size( $this->eMethod, MCRYPT_MODE_CBC );
        $this->iv         = mcrypt_create_iv( $this->ivSize, MCRYPT_RAND );
    }

    public function encrypt( $data, $salt )
    {
        $encryptedMessage = mcrypt_encrypt( $this->eMethod, $this->siteKey, $data, MCRYPT_MODE_CBC, $this->iv );
        $encryptedSalt    = mcrypt_encrypt( $this->eMethod, $this->siteKey, $salt, MCRYPT_MODE_CBC, $this->iv );
        $saltIdentifier   = str_pad( strlen($encryptedSalt), 6, '0', STR_PAD_LEFT );

        $newData          = $encryptedSalt.($this->iv).$encryptedMessage.$saltIdentifier;
        return($newData);
    }

    public function decrypt( $data )
    {
        $thisSaltID       = substr( $data, -6 );
        $saltID           = ltrim($thisSaltID,'0');        
        $thisSalt         = substr( $data, 0, $saltID );        
        $goTo             = $saltID + ($this->ivSize);
        $thisIv           = substr( $data, $saltID, ($this->ivSize) );        
        $thisHash         = substr( $data, $goTo, -6 );

        $decryptedMessage = mcrypt_decrypt( $this->eMethod, $this->siteKey, $thisHash, MCRYPT_MODE_CBC, $thisIv );
        return($decryptedMessage);
    }
}

?>

---

When encrypting, it takes the data and a salt, encrypts both and then combines them with the iv and the padded strlen of the salt as per this line:

$newData = $encryptedSalt.($this->iv).$encryptedMessage.$saltIdentifier;

Then I know which order things appear and can pull it apart accordingly.

1. Is this salt unnecessary or poorly implemented?

2. When creating an object and encrypting a string, I pad the string to a length of multiple 16, and generate a random alphanumeric string of length 16 for the salt. Is it unnecessary to make sure the inputs are formatted in this way?

3. Should the site key be random every time (or static as it is now)? And are there advisements on its length?

4. Is using RIJNDAEL_128 with a 256-bit key the equivalent of using AES-256?

5. Is this code okay to use?/area of criticism

Answer 1

I'm no security expert, however I think answering this would be fun!

I'll start off by doing my best answering your four questions:

1. The salt is unnecessary.

   a salt is random data that is used as an additional input to a one-way function... The primary function of salts is to defend against dictionary attacks versus a list of password hashes and against pre-computed rainbow table attacks.

   These first few lines from the Wikipedia page for salts basically sum it up. Because we're working with encryption (opposed to say, hashing), the private key and the IV are our protection, not a salt. Reiterated over here on our Security Stack Exchange site.

   This section elaborates on why it's necessary for hashing to have salts:

   Without a salt, a successful SQL injection attack may yield easily crackable passwords.

2. Well, we just found out that having a salt infused really isn't very beneficial. So if we eliminate the salt, then all we have left in the encrypted output is the actual encryption, and the IV. Because the IV size won't be changing in this example, it's A-OK to strip the IV from the string using the IV size, and then the left over is the message encrypted.

3. No, it cannot be unique. How would you decrypt it later on if the data left the class?

   I'm not sure how you're creating the key, but I suggest something such as to get a truly random one:

   pack("H*", bin2hex(openssl_random_pseudo_bytes(32)));
   

4. Yes. Rijndael has three members in it's family: Rijndael-128, Rijndael-192, and Rijndael-256 (there're more, but only these three are AES standard). However, each has a block size of 128 bits. So therefore Rijndael-256 with a (the only option) block size of 128 would be the same as saying AES-256.

   The AES algorithm is capable of using cryptographic keys of 128, 192, and 256 bits to encrypt and decrypt data in blocks of 128 bits.

   Via NIST

5. Depends. Where do you want to use it? In production, I'd say pick up a framework which already has this topic highly covered and critiqued. It also depends on what you're encrypting.

---

Other comments

private $eMethod = MCRYPT_RIJNDAEL_128;

never changes, so you could turn this into a constant.

return($newData);

It's up to you, but it's more common to leave the parentheses off.

Your whitespace and vertical alignment can have negative effects on readability. Of course, it's up to you, but you may want to check out some other coding styles.