3
\$\begingroup\$

I have a new job coming up soon and want to increase my PHP knowledge.

This is my first attempt at creating and using a class. I just want to know if it's the correct way to do things.

It's very basic: a simple encrypt / decrypt of a string with a salt.

/**
* Encryption Class
*
* Use for encrypting / decrypting a string securely
* 
*/

class stringEncryption
{

  // generate random string if the variable in a encrypt / decrypt is set as false
  public function generateRandomString($length = 10) {
    return substr(str_shuffle("0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"), 0, $length);
  }

  // encrypts the string
  public function encryptString($string, $salt)
  {
    //if there is no salt, generate one
    if (!$salt)
    {
      $salt = self::generateRandomString();
    }
    return trim(base64_encode(mcrypt_encrypt(MCRYPT_RIJNDAEL_256, $salt, $string, MCRYPT_MODE_ECB, mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB), MCRYPT_RAND))));
  }

  // decrypt the string
  public function decryptString($string, $salt)
  {
    return trim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256, $salt, base64_decode($string), MCRYPT_MODE_ECB, mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB), MCRYPT_RAND)));
  }

}

This is how I have created 2 objects and encrypted / decrypted

include 'libs/PHP_Classes/class.encryption.php';

// Password BEFORE encryption
$passwordBefore = "TestPassword";
$salt = "8i3bf92f";

echo "Current Password: $passwordBefore<br>";

// object to handle the new password to be encrypted
$safePassword = new stringEncryption;
// object to handle the decryption of a password
$decrypt = new stringEncryption;

// create the encrypted password
$encryptedPassword = $safePassword->encryptString($passwordBefore, $salt);
// decrypt the password
$decryptedPassword = $decrypt->decryptString($encryptedPassword, $salt);

// show the password with encryption
echo "Encrypted: $encryptedPassword<br />";
// show the password decrypted
echo "Decrypted: $decryptedPassword";
\$\endgroup\$
4
  • 2
    \$\begingroup\$ How is your code involved with PDO? \$\endgroup\$ – Kid Diamond Sep 15 '14 at 0:06
  • 3
    \$\begingroup\$ There's a lot wrong with the security of your code. I suggest you don't show this code to your new employer. Perhaps take a look at this similar question, and maybe hang out for a bit over on Sec.SE. \$\endgroup\$ – Alex L Sep 15 '14 at 1:40
  • 1
    \$\begingroup\$ self::generateRandomString(); is a static method call, whereas the signature: public function generateRandomString is a non-static method. That's bad, mkay \$\endgroup\$ – Elias Van Ootegem Sep 15 '14 at 7:10
  • \$\begingroup\$ Read somewhere to use self:: but realise now the issues. Have since changed it to $this-> as I think that is correct. Still learning my way around the different types (static, protected, public etc) and how each one works correctly. As or the security issues, the purpose of this class was not to make something secure, but just to get the basics of creating / using classes. It will not be shown to employers :) A long while until new job and classes etc is just something I'm taking upon my self to learn more about. \$\endgroup\$ – Lovelock Sep 15 '14 at 8:37
1
\$\begingroup\$

Salt

Your salt isn't a salt, it's a key.

And you are generating a random key if none is given, and the user has no acces to this key. This means that if I do this:

$encryptedPassword = $safePassword->encryptString($passwordBefore);
$decryptedPassword = $decrypt->decryptString($encryptedPassword);

My output will be:

Current Password: TestPassword
Encrypted: ZWZNn1FC8F0NCO7Ac7YZojlUITPupOXsDV3BLvS3W2M=
Decrypted: æÁSLkz-HGÐyV]l¥S3]‹*õA"VÚX'

This shouldn't happen.

I would remove the generation code in encryptString and rename salt to key.

Hashing vs Encryption

Generally, passwords are not encrypted, they are hashed. Is there a reason you are encrypting your passwords?

Padding oracle attacks

You are vulnerable to padding oracle attacks. I linked to some resources about this in this post.

Object creation

Why are you creating two stringEncryption objects? One would be enough.

Style

  • be consistent: either curly brackets go on the same line or the next
  • follow conventions: class names start with an uppercase letter

If you want to learn more about OOP in PHP, I would suggest you write something where your objects have a state (for example Conway's Game of Life).

\$\endgroup\$

Your Answer

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

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