User Password Encryption in C#

Question

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 "";
        }

    }

}

Answer 1

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

Answer 2

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.