current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

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.

share|improve this question
up vote 1 down vote

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.
share|improve this answer
    
Thank you! I will wait for more insights if possible. I added the stripslashes because when i type quotes " in the text, then in the db i am getting an extra slash from php... So i am having a recursive \ problem..... – e4rthdog May 9 '16 at 11:24
    
@e4rthdog that sounds like a bug in the db code, it definitely shouldn't be fixed here. – tim May 9 '16 at 11:38
    
Take a look.... stackoverflow.com/questions/8949768/… – e4rthdog May 9 '16 at 11:43
    
@e4rthdog ah yes, WPs magic quotes are a pain. No idea why they still have it... Anyways, it still definitely should not be handled here, but in the db access/save code, some generic function that fixes WPs magic quotes, or somewhere else that makes sense in the context of your code. – tim May 9 '16 at 11:48
    
mmm Probably i can do this on decrypt: $string_unslashed = array_map('stripslashes_deep', $string); – e4rthdog May 9 '16 at 12:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.