1
\$\begingroup\$

I'm working with another company in a new application for my company. My job right now is to send on demand to the other company the data that they need, ciphering some of it.

I should point out that the key is not going to be stored in any database.

Here's the code that I wrote, intended to be used in both services.

using System;
using System.IO;
using System.Security.Cryptography;
using System.Text;

namespace RETACrypto
{
    public static class CryptoHelper
    {
        #region Fields
        private const int maxIVLength = 16;
        private const int maxSaltLength = 32;
        private const string password = "lmnKNG6IJVEcsMkaJgnFyZVG0lINDOzv";
        #endregion

        #region Public Methods
        public static string EncryptString(string source)
        {
            try
            {
                string result = "";
                byte[] resultBytes;
                byte[] encriptedSource;
                byte[] salt = GetSalt(maxSaltLength);

                using (var aes = new AesManaged { Key = GetKey(salt, Encoding.UTF8.GetBytes(password)), Mode = CipherMode.CBC, Padding = PaddingMode.PKCS7 })
                {
                    byte[] sourceByteArray = Encoding.UTF8.GetBytes(source);

                    using (var encryptor = aes.CreateEncryptor(aes.Key, aes.IV))
                    {
                        encriptedSource = encryptor.TransformFinalBlock(sourceByteArray, 0, sourceByteArray.Length);
                    }

                    resultBytes = new byte[aes.IV.Length + salt.Length + encriptedSource.Length];
                    Array.Copy(aes.IV, 0, resultBytes, 0, aes.IV.Length);
                    Array.Copy(encriptedSource, 0, resultBytes, aes.IV.Length, encriptedSource.Length);
                    Array.Copy(salt, 0, resultBytes, aes.IV.Length + encriptedSource.Length, salt.Length);
                }
                result = Convert.ToBase64String(resultBytes);

                return result;
            }
            catch (Exception)
            {
                throw;
            }
        }

        public static string DecryptString(string source)
        {
            try
            {
                string result = "";
                byte[] sourceByte = Convert.FromBase64String(source);
                byte[] resultIV = GetPartOfTheMessage(sourceByte, 0, maxIVLength);
                byte[] salt = GetPartOfTheMessage(sourceByte, sourceByte.Length - maxSaltLength, maxSaltLength);
                sourceByte = GetPartOfTheMessage(sourceByte, maxIVLength, sourceByte.Length - maxIVLength - maxSaltLength);

                byte[] resultByte;
                int decryptedByteCount = 0;

                using (var aes = new AesManaged { Key = GetKey(salt, Encoding.UTF8.GetBytes(password)), IV = resultIV, Mode = CipherMode.CBC, Padding = PaddingMode.PKCS7 })
                {
                    using (ICryptoTransform AESDecrypt = aes.CreateDecryptor(aes.Key, aes.IV))
                    {
                        using (MemoryStream memoryStream = new MemoryStream(sourceByte))
                        {
                            using (CryptoStream cs = new CryptoStream(memoryStream, AESDecrypt, CryptoStreamMode.Read))
                            {
                                resultByte = new byte[sourceByte.Length];
                                decryptedByteCount = cs.Read(resultByte, 0, resultByte.Length);
                            }
                        }
                    }
                    result = Encoding.UTF8.GetString(resultByte);
                    if (decryptedByteCount < result.Length)
                    {
                        result = result.Substring(0, decryptedByteCount);
                    }
                }

                return result;
            }
            catch (Exception)
            {
                throw;
            }
        }
        #endregion

        #region Private Methods       
        private static byte[] GetSalt(int length)
        {
            byte[] salt = new byte[length];
            using (RNGCryptoServiceProvider random = new RNGCryptoServiceProvider())
            {
                random.GetNonZeroBytes(salt);
            }
            return salt;
        }

        private static byte[] GetKey(byte[] salt, byte[] password)
        {
            byte[] combine = new byte[salt.Length + password.Length];
            byte[] result;
            try
            {
                Array.Copy(salt, combine, salt.Length);
                Array.Copy(password, 0, combine, salt.Length, password.Length);

                using (SHA256 sha256Hash = SHA256.Create("SHA256"))
                {
                    result = sha256Hash.ComputeHash(combine);
                }
            }
            catch (Exception)
            {
                throw;
            }

            return result;
        }

        private static byte[] GetPartOfTheMessage(byte[] source, int startPoint, int length)
        {
            byte[] result = new byte[length];
            Array.Copy(source, startPoint, result, 0, length);
            return result;
        }
        #endregion
    }
}

Now, my questions:

  • What's the best way of storing maxIVLength, maxSaltLength and; most importantly, password so there are no magic variables in my code?
  • Right now I am storing the IV at the beginning of the message and the salt at the end. Should I keep it like this or store all at the beginning or end?
  • Is my use of RNGCryptoServiceProvider ok or should I make a static variable so I only construct it once? How do you dispose of it in that case?

There probably are other potential problems or mistakes that I've overlooked, feel free to point them out.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! (problems of mistakes such as no spelling checker being able to tell or should go where of stands?) \$\endgroup\$ – greybeard Feb 15 at 10:51
  • \$\begingroup\$ @greybeard I guess that's what happens when you use google translate as your main spell checker. Thanks for pointing it out! \$\endgroup\$ – ikerbera Feb 15 at 10:55
  • \$\begingroup\$ The try/catch/throw statements do nothing. \$\endgroup\$ – Brad M Feb 15 at 15:46
1
\$\begingroup\$

Just to preface this answer: It will be about the crypto-related issues I spotted, not so much about things like code style, idioms, patterns and thelike, because I know more about that than proper C# style. The points are sorted descending by severity. Oh and I'm already sorry for ignoring your concrete questions...


Rolling your own crypto

As you may have figured out from this answer barely talking about code quality and being really long, coming up with good cryptographic schemes is hard. Really hard. So as your scenario seems to be "establish a secure connection with some other party sharing a high-entropy token with me", I'd suggest you have a look at using TLS with one of the PSK cipher-suites which takes care of all of the problems mentioned here (and some more).

For example BouncyCastle implements these cipher suites.

Initialization Vector

Currently the code doesn't set the IV property of the aesManaged class and I couldn't find any documentation saying that a sensible default will be chosen. Thereby I have to assume this will just be a hard-coded value, e.g. all zeroes. This is really bad. This leaks whether two messages share the same prefix for CBC mode!

So if you send AABBCCEF and AABBCDEF an attacker will be able to learn that the messages are equal up to and including AABBC (with 16-byte granularity).

A better approach is to generate this value independently at random for each message. Do not deterministically derive it from the key and / or the message because this will leak whether two messages are equal!

CBC-Mode

Currently this code uses the infamous CBC-mode which time and again has lead to attacks in when it was used in TLS. The problem here is that an attacker may modify the message in a malicious way, e.g. to exploit a parser bug or trigger some other reaction without the receiver having a high chance of noticing it before it's too late. The better solution is to use authenticated encryption (like AES-GCM) which will make sure the message is unmodified before handing out the plaintext.

The password

Currently the password is a magic string, probably intended to be used by both sides. For starters I really hope you didn't intend to use the password posted here in your actual production now that it is forever on the internet.

Next it's usually best-practice to not store credentials (like a password) in a (plain) file or hard-code it into the source, but to put it into an environment variable. This way it can be quickly swapped out and it won't accidently end up in your or a public git repository.

Next it appears that the "password" really is just a shared random string already. So why bother with readable characters? Just generate a cryptographically secure 256-bit random value and put its base64 encoded version into an environment variable! This way you can be sure it won't be brute-forced and due to that you can also forego the salt in the key derivation phase and either use it directly or if you also want to use it for other cryptographic applications, use something like HKDF.

Password-based Key Derivation

The code uses SHA256(salt||password) as the key. That is a simple salted hash. Assuming the password isn't good this won't protect you from brute-force which will just throw a bunch of GPUs at the problem and arrive at the password at a rate of about 8.64TH/USD, that is 8,640,000,000,000 passwords for a single US-Dollar on a public cloud service. If the password isn't strong this won't protect you. If it is actually really strong you can forego the salting. If it is in the middle, you should use to a better password-based key derivation, e.g. using Argon2.

\$\endgroup\$
  • \$\begingroup\$ I very much appreciate all your comments and suggestions and will take a look at them ASAP but also I have some clarifications that I probably should have included in the answer: "Currently the code doesn't set the IV property of the aesManaged class". In this case I had an aes.GenerateIV() line but debugging I realized that the IV also initialized when constructing AesManaged so I did some test and thought that line was redundant. \$\endgroup\$ – ikerbera Feb 18 at 6:25
  • \$\begingroup\$ I had to make two comments because my answer was too long. "Currently the password is a magic string". "For starters I really hope you didn't intend to use the password posted here in your actual production". Right now that password is for testing purposes and I'm changing it every week until I find the best way of making and storing passwords for this case. \$\endgroup\$ – ikerbera Feb 18 at 6:25

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.