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

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.

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.

-
Is this salt unnecessary or poorly implemented?

-
When creating an object and encry

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:

-
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.

• 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.

-
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)));

-
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

-
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.

StackExchange Code Review Q#49267, answer score: 5