Cryptographic Extensions

I have created a small crypto extension and I want a deep review of it, such as possible fixes (for hidden problems) and tweaks...

-
1- Crypto.cs

```
public static class Crypto
{
public static Salt Salt = new Salt();

public static string GetMd5Hash(this string input)
{
using (var md5Hash = new HMACMD5(Salt.Key)) {
byte[] data = md5Hash.ComputeHash(Encoding.UTF8.GetBytes(input));
var sBuilder = new StringBuilder();
foreach (byte t in data)
{
sBuilder.Append(t.ToString("x2"));
}
return sBuilder.ToString();
}
}

public static bool VerifyMd5Hash(this string input, string hash)
{
string hashOfInput = GetMd5Hash(input);
StringComparer comparer = StringComparer.OrdinalIgnoreCase;
return 0 == comparer.Compare(hashOfInput, hash);
}

public static byte[] GenerateKey(int size)
{
using (var cryptoProvider = new RNGCryptoServiceProvider()) {
var buffer = new byte[size];
cryptoProvider.GetBytes(buffer);
return buffer;
}
}

public static byte[] AESEncryptBytes(this byte[] message, byte[] sharedSecret)
{
using (var pdb = new Rfc2898DeriveBytes(Convert.ToBase64String(sharedSecret), Salt.Key)) {
using (var ms = new MemoryStream()) {
using (Aes aes = new AesManaged()) {
aes.Key = pdb.GetBytes(aes.KeySize/8);
aes.IV = pdb.GetBytes(aes.BlockSize/8);
using (var cs = new CryptoStream(ms, aes.CreateEncryptor(), CryptoStreamMode.Write)) {
cs.Write(message, 0, message.Length);
}
}
return ms.ToArray();
}
}
}

public static byte[] AESDecryptBytes(this byte[] message, byte[] sharedSecret)
{
using (var pdb = new Rfc2898DeriveBytes(Convert.ToBase64String(sha

Lots and lots of badness. Please don't write crypto before you've spend some effort studying crypto. Take a look at jbtule's answer to Encrypt/Decrypt string in .NET for how to symmetric encryption.

-
I don't get the what the goal of GetMd5Hash is. It doesn't compute an MD5 hash. It uses a MAC (HMAC-MD5) so it's name is wrong.

It's not used like a MAC (a MAC is keyed), but somewhat similar to a password hash, but the construction is no password hash. Password hashing should use PBKDF2 (Rfc2898DeriveBytes) not HMAC since the latter is too fast.

-
A global static salt doesn't make much sense. Salts are mainly useful for password hashes, but they should be unique for each call.

-
The hex encoding should be in its own method. That method doesn't have a single purpose.

-
VerifyMd5Hash has input dependent runtime. That's a security weakness if it were a MAC, not so much if it's a password hash.

-
AESEncryptBytes is using Rfc2898DeriveBytes on the key. Keys are supposed to be uniformly distributed and have high entropy. So this is just a waste of resources. Rfc2898DeriveBytes should be used on passwords, not keys.

-
You pass the same global salt to Rfc2898DeriveBytes and HMAC. That's a bad idea for two reasons: It's fixed, so it misses the point. And it's using the same value as HMAC key and PBKDF2 salt. Keys should only ever be used in one place.

-
You're using a fixed IV. Similarly to a salt, a new IV should be used for each encryption.

-
You're using unauthenticated encryption, so you're vulnerable to padding oracles if you face active attackers.

-
TransformFinalBlock is much simpler than all that stream stuff.

-
The per-process salt means that decryption won't work once you restart the application

-
A 256 byte salt is insanely large. 128 bits enough.

-
Prefer a uniformly random salt over a string. (Your Salt.Set(string) isn't a good idea)

-
Salt contains global mutable state, that's bad design.

-
Salt.Key is a nonsensical expression. Something is either a salt, or a key. Not both.

StackExchange Code Review Q#44266, answer score: 6