2
\$\begingroup\$

i am not the best at security but need a custom way to securely store personal identifiable information like name, address etc - Can i please have some feedback on this custom code, from my perspective it looks okay but with security not being my strong point i don't trust myself.

The data is passed from my application over HTTPS in the header along with a custom auth code which is made up from the users unique ID, fcm ID, package name and the epoch date they obtained the auth code.

This is sent to the server on every request and decrypted to make sure that we have the correct user - the server won't provide any details back to the unless the epoch date matches (this is stored in the database on the auth creation)

Here is the encrypt / decrypt function

function encrypt($string, $public_key = "public key", $action = "e") {
     $secret_key = "secret key";
     $encrypt_method = "AES-256-CBC";
     $key = hash( 'sha256', $public_key );
     $secret = substr( hash( 'sha256', $secret_key), 0, 16 );

     if( $action == 'e' ) {
          $output = base64_encode( openssl_encrypt( $string, $encrypt_method, $key, 0, $secret) );
     } else if ( $action == 'd') {
          $output = openssl_decrypt( base64_decode( $string ), $encrypt_method, $key, 0, $secret);
     }
     if (empty($output)) {
          denyAccess();
     }
     return $output;
}

the data stored in the database uses the same function but with different keys so the data is encrypted twice.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review. I couldn't answer your question, too much code is missing for my taste, but I do notice two things: 1. The variable $secret_key is never used, and the variable $secret_iv is never defined. Is that a mistake? 2. Your function encrypts and decrypts, but is simply called encrypt(). That is very confusing. This should be split into 2 functions, or be a class. A tip: Give us more code to review, that way you will get a better review. There simply isn't much to review here. \$\endgroup\$ – KIKO Software Apr 28 at 7:07
  • 1
    \$\begingroup\$ Also: $output will probably never be empty. Unless, of course, you provide an invalid $action. However, I think you want to respond to the input $string of this function, not the output. \$\endgroup\$ – KIKO Software Apr 28 at 8:30
  • \$\begingroup\$ else is more appropriate than if (empty($output)) {. \$\endgroup\$ – mickmackusa Apr 28 at 9:43
  • \$\begingroup\$ @KIKOSoftware yes sorry i changed the variable names when copying on here for easier readability the secret_iv is the secret_key \$\endgroup\$ – Im a Developer Apr 28 at 11:20
  • \$\begingroup\$ Are we really only to review this solitary function? Are we right to assume that denyAccess(); kills everything (the function encrypt() never gets a chance to return)? How are encrypt() calls performed? We'd like to see more code. \$\endgroup\$ – mickmackusa Apr 28 at 11:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.