9
\$\begingroup\$

I am maintaining an old codebase of one of our legacy .Net applications which has a component that encrypts and decrypts user passwords using symmetric key cryptography. Since cryptography is not my domain, I need help figuring out whether this existing code needs to be fixed or not. I have a hunch that this code is cringe-inducing for obvious reasons but I digress.

Some potential issues I see here are:

  1. The usage of MD5 hashing, and from what I know, MD5 has been found to have collisions and is no longer secure.

  2. Usage of key stored in the code file which an attacker could figure out if they are successful in getting their hands on this code or by decompiling the assembly.

  3. Maybe the passwordIterations variable needs to be bumped up to a number in the range [10, 20]?

using System;
using System.IO;
using System.Text;
using System.Security.Cryptography;
using System.Runtime.InteropServices;
using System.Runtime.CompilerServices;
using System.Reflection;

namespace RijndaelEncDec
{

    public interface EncryptDecrypt
    {
        string Encrypt(string Data);
        string Decrypt(string Data);
    }

    [ClassInterface(ClassInterfaceType.AutoDual)]
    public class RijndaelEncDec : EncryptDecrypt
    {
        public string Encrypt(string Data)
        {
            try
            {

                string passPhrase = "bananax97";
                string saltValue = "pepper";
                string hashAlgorithm = "MD5";
                int passwordIterations = 1;
                string initVector = "koxskfruvdslbsxu";
                int keySize = 128;

                byte[] initVectorBytes = Encoding.ASCII.GetBytes(initVector);
                byte[] saltValueBytes = Encoding.ASCII.GetBytes(saltValue);
                byte[] plainTextBytes = Encoding.UTF8.GetBytes(Data);

                PasswordDeriveBytes password = new PasswordDeriveBytes(passPhrase,saltValueBytes,hashAlgorithm,passwordIterations);
                byte[] keyBytes = password.GetBytes(keySize/8);

                RijndaelManaged symmetricKey = new RijndaelManaged();
                symmetricKey.Mode = CipherMode.CBC;

                ICryptoTransform encryptor = symmetricKey.CreateEncryptor(keyBytes,initVectorBytes);
                MemoryStream memoryStream = new MemoryStream();
                CryptoStream cryptoStream = new CryptoStream(memoryStream,encryptor,CryptoStreamMode.Write);

                cryptoStream.Write(plainTextBytes,0,plainTextBytes.Length);
                cryptoStream.FlushFinalBlock();         

                byte[] cipherTextBytes = memoryStream.ToArray();

                memoryStream.Close();
                cryptoStream.Close();

                string cipherText = Convert.ToBase64String(cipherTextBytes);

                return cipherText;

            }
            catch {}

            return "";
        }

        public string Decrypt(string Data)
        {
            try
            {

                string passPhrase = "bananax97";
                string saltValue = "pepper";
                string hashAlgorithm = "MD5";
                int passwordIterations = 1;
                string initVector = "koxskfruvdslbsxu";
                int keySize = 128;

                byte[] initVectorBytes = Encoding.ASCII.GetBytes(initVector);
                byte[] saltValueBytes = Encoding.ASCII.GetBytes(saltValue);
                byte[] cipherTextBytes = Convert.FromBase64String(Data);

                PasswordDeriveBytes password = new PasswordDeriveBytes(passPhrase,saltValueBytes,hashAlgorithm,passwordIterations);
                byte[] keyBytes = password.GetBytes(keySize/8);

                RijndaelManaged symmetricKey = new RijndaelManaged();
                symmetricKey.Mode = CipherMode.CBC;

                ICryptoTransform decryptor = symmetricKey.CreateDecryptor(keyBytes,initVectorBytes);
                MemoryStream memoryStream = new MemoryStream(cipherTextBytes);
                CryptoStream cryptoStream = new CryptoStream(memoryStream,decryptor,CryptoStreamMode.Read);

                byte[] plainTextBytes = new byte[cipherTextBytes.Length];

                int decryptedByteCount = cryptoStream.Read(plainTextBytes,0,plainTextBytes.Length);

                memoryStream.Close();
                cryptoStream.Close();

                string plainText = Encoding.UTF8.GetString(plainTextBytes,0,decryptedByteCount);

                return plainText;

            }
            catch {}

            return "";
        }

    }

}
\$\endgroup\$
  • 1
    \$\begingroup\$ Dont have time to look over this just yet but: use bcrypt use bcrypt use bcrypt. If you're serious about password encryption, don't roll your own. However, if you're just doing this for learning purposes, you don't ever want to encrypt passwords. You want to hash them. EDIT: Nvm, I didn't read your problem space. If your passwords are currently encrypted I'd strongly recommend decoding and then hashing them if this is at all possible. Encryption is not a secure method of storing passwords. \$\endgroup\$ – Dan Pantry Jul 10 '15 at 16:52
  • 2
    \$\begingroup\$ Many of those classes, PasswordDeriveBytes, RijndaelManaged, ICryptoTransform, MemoryStream and CryptoStream implement IDisposable and should be wrapped in appropriate using constructs to ensure proper deterministic disposal of resources. \$\endgroup\$ – Jesse C. Slicer Jul 10 '15 at 16:56
  • \$\begingroup\$ @DanPantry Why is encryption not a secure method for storing passwords? \$\endgroup\$ – johnny 5 Jan 10 '16 at 5:24
  • 1
    \$\begingroup\$ @johnny5 I can't explain that in 600 characters, so I wrote it up in a gist. I'd encourage you to do your own research on this topic, though, as I'm not an authority on it. \$\endgroup\$ – Dan Pantry Jan 10 '16 at 10:55
7
\$\begingroup\$

Your code has few flaws:

  • You decrypting password - if someone gain access to your source code he can brake all your passwords with no effort. All you need to do is verify, not know it. Instead of decrypting it just check hash of provided password matches.
  • Have one salt for every password / user. It should be unique.
  • Doesn't have random seed.
  • Salt is too short - there is no reason for shortening it since nobody have to remember it.
  • Uses MD5 as hashing algorithm. You should move to PBKDF2 (see RFC2898)
  • Have only one iteration of hashing function - for example LastPass uses 100k rounds of hashing. (read more here)

You need 3 methods:

  1. For computing hash.
  2. For generating salt.
  3. For checking if password hash matches.

    using System.Web.Security;
    using System.Security.Cryptography;
    
    namespace RijndaelEncDec{
    public static class PasswordHasher
    {
    // 24 = 192 bits
    private const int SaltByteSize = 24;
    private const int HashByteSize = 24;
    private const int HasingIterationsCount = 10101;
    
    public static byte[] ComputeHash(string password, byte[] salt, int iterations=HasingIterationsCount, int hashByteSize=HashByteSize)
    {
        Rfc2898DeriveBytes hashGenerator = new Rfc2898DeriveBytes(password, salt);
        hashGenerator.IterationCount = iterations;
        return hashGenerator.GetBytes(hashByteSize);
    }
    
    public static byte[] GenerateSalt(int saltByteSize=SaltByteSize)
    {
        RNGCryptoServiceProvider saltGenerator = new RNGCryptoServiceProvider();
        byte[] salt = new byte[saltByteSize];
        saltGenerator.GetBytes(salt);
        return salt;
    }
    
    public static bool VerifyPassword(String password, byte[] passwordSalt, byte[] passwordHash)
    {
        byte[] computedHash = ComputeHash(password, passwordSalt);
        return AreHashesEqual(computedHash, passwordHash);
    }
    
    //Length constant verification - prevents timing attack
    private static bool AreHashesEqual(byte[] firstHash, byte[] secondHash)
    { 
        int minHashLenght = firstHash.Length <= secondHash.Length ? firstHash.Length : secondHash.Length;
        var xor = firstHash.Length ^ secondHash.Length;
        for (int i = 0; i < minHashLenght; i++)
            xor |= firstHash[i] ^ secondHash[i];
        return 0 == xor;
    }
    }   
    }
    

Unfortunately you will have to change your interface a little...

\$\endgroup\$
4
\$\begingroup\$

Some changes I would make:

Limit your use of whitespace. You shouldn't need whitespace after opening or before closing braces.

I.e. start it as:

namespace RijndaelEncDec
{
    public interface EncryptDecrypt
    {
        string Encrypt(string Data);
        string Decrypt(string Data);
    }

    [ClassInterface(ClassInterfaceType.AutoDual)]
    public class RijndaelEncDec : EncryptDecrypt
    {
        public string Encrypt(string Data)
        {
            try
            {
                string passPhrase = "bananax97";
                string saltValue = "pepper";

The come to:

                return cipherText;
            }
            catch {}

            return "";
        }

        public string Decrypt(string Data)
        {
            try
            {
                string passPhrase = "bananax97";

Lastly:

                string plainText = Encoding.UTF8.GetString(plainTextBytes,0,decryptedByteCount);

                return plainText;
            }
            catch {}

            return "";
        }
    }
}

It helps deflate the file and make it easier to associate blocks.

I would declare the following variables (from each method) to private const fields:

string hashAlgorithm = "MD5";
int passwordIterations = 1;
string initVector = "koxskfruvdslbsxu";
int keySize = 128;

You should be using (pun intended) your MemoryStream and CryptoStream objects. They are IDisposable for a reason.

Lastly, your namespace: you should name it RijndaelCryptography to follow along with what it is meant to represent a collection of. Likewise, your interface should have two interfaces: IEncryptor, IDecryptor.

Otherwise, there's nothing glaring to me.

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