PHP function for encrypt / decrypt using openssl This is a function that is the building block of one wordpress plugin. I came up with this code after some search but i need to optimize it and get any info from more experienced guys.

public static function afz_encrypt_decrypt( $action, $string ) {
    $output = false;

    $encrypt_method = "AES-256-CBC";
    $secret_key     = '12345678901234567890123456789012';
    $iv_size        = 16;

    if ( $action == 'encrypt' ) {
        $encrypt_iv = mcrypt_create_iv( $iv_size, MCRYPT_DEV_URANDOM );
        $output     = openssl_encrypt( $string, $encrypt_method, $secret_key, 0, $encrypt_iv );
        $output     = base64_encode( $encrypt_iv . $output );
    } else if ( $action == 'decrypt' ) {
        $decrypt_iv = substr( base64_decode( $string ), 0, $iv_size );
        $output     = stripslashes( openssl_decrypt( substr( base64_decode( $string ), $iv_size ), $encrypt_method,
            $secret_key,
            0,
            $decrypt_iv ) );
        if ( false === $output ) {
            $output = $string;
        }
    }

    return $output;
}


Update: The stripslashes is indeed unneeded and i will take it out of context of this function. I need it because wordpress automatically escapes strings regardless if the relevant PHP Setting. See the NOTES section here: CODEX Link

What i still need though is more insight (if possible) to Tim's answer in regards to the security aspect of the source.
I haven't looked at the security aspect of this, so you might want to wait for answers on that aspect.

Function Design

Your function does too many things (encryption and decryption), making it harder to read than necessary and hard to use. 

At a minimum you would need to document it properly, as otherwise, users would need to look at the actual code to use it, which isn't ideal. 

But really, it will be much simpler to have two functions:

function afz_encrypt($plaintext) {}
function afz_dencrypt($ciphertext) {}


If you don't want to duplicate the encryption parameters, saving those in the class may be a good idea. 

I would also pass the key to the function instead of hardcoding it in the function itself.

Return values and error handling

The return values do not seem that well defined. The function returns:


false if an invalid action is given
false if it can't base64 encode on encryption
the base64 encoded iv if it can't encrypt (meaning the input is lost).
the encrypted input string if it can't decrypt (and this happens without any warnings by PHP).


There are probably more irregularities, but I think the point is clear: A user cannot figure out what exactly went wrong, and sometimes cannot even tell that something went wrong, which will sooner or later lead to bugs. 

You should throw exceptions on invalid input stating what exactly went wrong (eg wrong action, cannot base64 decode, etc), and check the output of all the functions you use.

Invalid Output


why stripslashes? It means that decrypt(encrypt(plain)) != plain in some situations (eg foo'bar, a\b, etc).
Also, decrypt(encrypt([empty string])) != [empty string] and decrypt(aaaaaaaaaaaaaaaaaaaaaaa) = aaaaaaaaaaaaaaaaaaaaaaa because of the issues outlined above.