User Password Encryption in C#

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:

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

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

-
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

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:

• For computing hash.

• For generating salt.

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

StackExchange Code Review Q#96494, answer score: 8