3
\$\begingroup\$

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(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.CreateDecryptor(), CryptoStreamMode.Write))    {
                            cs.Write(message, 0, message.Length);
                        }
                    }
                    return ms.ToArray();
                }
            }
        }
    
        public static string AESEncryptString(this string message, byte[] sharedSecret)
        {
            return Convert.ToBase64String(AESEncryptBytes(Encoding.UTF8.GetBytes(message), sharedSecret));
        }
    
        public static string AESDecryptString(this string message, byte[] sharedSecret)
        {
            return Encoding.UTF8.GetString(AESDecryptBytes(Convert.FromBase64String(message), sharedSecret));
        }
    }
    
  • 2- Salt.cs

    public class Salt
    {
        public Salt()
        {
            Key = GenerateRandomKey();
        }
    
        public byte[] Key { get; private set; }
    
        public byte[] GenerateRandomKey()
        {
            return Crypto.GenerateKey(256);
        }
    
        public Salt Set(string salt)
        {
            Key = Encoding.UTF8.GetBytes(salt);
            return this;
        }
    
        public Salt Set(byte[] salt)
        {
            Key = salt;
            return this;
        }
    }
    
  • 3- Usage

    static void Main()
    {
        #region Cryptography
    
        const String message = "ABCDEFGH"; //Message to be encrypted/decrypted
        String encryptedMessage;
        Byte[] sharedSecret = {0x01, 0x02, 0x03, 0x04, 0x05}; //Shared secret generated using RSA or DH Key Exchange (or whatever).
        Crypto.Salt = new Salt(); //Generates a new random dummy key *YOU HAVE TO SAVE THIS SALT FOR FUTURE DECRYPTING*.
        Crypto.Salt.Set(@"ek]U)/XOdwqP{u<el(K=\DNPR,iM%"); //Or you can specify your own.
    
        Console.WriteLine("1- Salted MD5 Hash:-");
        Console.Write("Encrypted: ");
        Console.WriteLine(encryptedMessage = message.GetMd5Hash());
        Console.Write("Identical: ");
        Console.WriteLine(message.VerifyMd5Hash(encryptedMessage));
        Console.WriteLine();
    
        Console.WriteLine("2- Salted AES Encryption:-");
        Console.Write("Encrypted: ");
        Console.WriteLine(encryptedMessage = message.AESEncryptString(sharedSecret));
        Console.Write("Decrypted: ");
        Console.WriteLine(encryptedMessage.AESDecryptString(sharedSecret));
        Console.WriteLine();
    
        Console.WriteLine("3- Random Key Generation:-");
        Console.WriteLine(BitConverter.ToString(Crypto.GenerateKey(16)).Replace("-", ""));
        Console.WriteLine();
    
        #endregion
    
        Console.Read();
    }
    
  • I think using Encoding here is kinda inappropriate so any ideas for that too ?

\$\endgroup\$
  • \$\begingroup\$ I can think of a small enhancement. You could make overloads for GetMd5Hash() and any other method that uses hardcoded encodings such as UTF8, hex or base64. You could specify the encodings in a method parameter. This might make your extensions more versatile. \$\endgroup\$ – Sam Leach Mar 13 '14 at 20:19
6
\$\begingroup\$

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.

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

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

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

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

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

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

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

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

  9. TransformFinalBlock is much simpler than all that stream stuff.

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

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

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

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

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

\$\endgroup\$
  • \$\begingroup\$ so after you destroyed me with all that notices :D are there any already implemented and verified methods somewhere that do the same as the methods i tried to write above, i want to encrypt/decrypt strings as well as byte[] using AES, also a good way to MD5 hash passwords. I mean at least some implementations that are verified and i can relay on to write my own. \$\endgroup\$ – RuneS Mar 13 '14 at 20:29
  • \$\begingroup\$ @RuneS jbtule's answer I linked looks reasonable to me. For password hashing you shouldn't look at MD5, but rather at Rfc2898DeriveBytes \$\endgroup\$ – CodesInChaos Mar 13 '14 at 20:34
  • \$\begingroup\$ You mean that i can simply use Rfc2898DeriveBytes and return .GetBytes as a hashed password ?, also how could i Verify hash then ? \$\endgroup\$ – RuneS Mar 13 '14 at 20:36
  • \$\begingroup\$ For number 5 notice, but that is how it was implemented every where on the internet, Rfc2898DeriveBytes is commonly use to generate a salted Key, also the sharedSecret variable at my method is supposed to be pre generated using an asymmetric function... RSA or DH for instance. So i don't get point 5, 7, and 8 correctly as that is the conclusion of what i found all over the internet. \$\endgroup\$ – RuneS Mar 13 '14 at 20:55
  • \$\begingroup\$ Concerning 5: You need to use Rfc2898DeriveBytes if you want to derive a key from a user supplied password. This should only be used in places where you can't generate a high quality random secret. A random key encrypted with RSA can be used directly, a DH shared secret should be hashed with a cheap hash (e.g. key=sha2(shared-secret)) or a bit better style using a cheap key-derivation-function (KDF) like HKDF instead of an expensive KDF like PBKDF2. \$\endgroup\$ – CodesInChaos Mar 13 '14 at 21:02
1
\$\begingroup\$

You code is readable, your names are meaningful and it seems like you know your way around C#. Keep up the good work :)

Some general non-crypto C# comments:

  1. using blocks

    Your using using a lot and it's good. But try to minimize the code inside them.

    If you've got a resource R that needs careful handling - then acquire it, use it and then release it as soon as possible. Especially when using time and synchronization sensitive resources such as DB connections or files - close it the moment you're done with it. It's not a big deal for the classes you're using, but it's a good practice. Also, try not to use control flow directions (such as return) inside it. So I'd rewrite GenerateKey:

    public static byte[] GenerateKey(int size)
    {
        var buffer = new byte[size];
        using (var cryptoProvider = new RNGCryptoServiceProvider()) {
            cryptoProvider.GetBytes(buffer);
        }
        return buffer;
    }
    
  2. VerifyMd5Hash - I find this form more straight-forward:

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

    This method is redundant - it just calls another (static) function. If you do want to keep it - make it static. A rule of thumb: if there's no reason not to make a method static - make it static.

  4. Salt.Set

    These kind of method should probably be of type void. Seeing that in order to call them you have to hold a reference to the Salt instance - what's the use of returning another reference?

    Maybe what you need is a factory function:

    public static Salt Set(string saltString)
    {
        var salt = new Salt(); // calling the default constructor now calls GenerateRandomKey, which is not needed
        salt.Key = Encoding.UTF8.GetBytes(saltString);
        return salt;
    }
    
\$\endgroup\$
  • \$\begingroup\$ There are amazing tips, i will re look the way i am doing things that way then. \$\endgroup\$ – RuneS Mar 13 '14 at 20:38
  • \$\begingroup\$ at the last function i can tell that you can't double define salt \$\endgroup\$ – RuneS Mar 13 '14 at 20:49

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.