0
\$\begingroup\$

I created an Encryption class that encrypts data and decrypts it using the AesCryptoServiceProvider class with an HMACSHA256 hash. The goal was to replace our existing encryption class with something that was FIPS compliant.

I would like a review of my structure, security and FIPS compliance.

Encryption.cs

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

namespace EncryptionTools
{
    public class Encryption
    {
        /// <summary>
        /// Represents an exploded set of data from a single payload
        /// </summary>
        private class ExplodedResult
        {
            /// <summary>
            /// The initialization vector used during decryption
            /// </summary>
            public byte[] IV { get; set; }

            /// <summary>
            /// Salt data containing keys and salt hashes
            /// </summary>
            public SaltData SaltData { get; set; }

            /// <summary>
            /// The cypher data that was originally encrypted
            /// </summary>
            public byte[] CypherData { get; set; }

            /// <summary>
            /// A hash of the entire original payload
            /// </summary>
            public byte[] AuthenticationHash { get; set; }

            /// <summary>
            /// The size of the original payload, excluding the authentication hash.
            /// </summary>
            public int SizeOfPayload { get; set; }
        }

        /// <summary>
        /// Represents the authentication salt and the corresponding key, along with the cryptographic salt and it's associated key.
        /// </summary>
        private class SaltData
        {
            /// <summary>
            /// Creates a new instance of the SaltData using the password to salt and iterations to 
            /// generate both a Cryptographic Salt, and Key along with an Authentication Salt and associated key.
            /// </summary>
            /// <param name="saltBlockSize">The size of the salt block</param>
            /// <param name="passwordToSalt">The password to create a cryptograph and authentication key</param>
            /// <param name="saltIterations">The number of times to hash the salt when creating the keys.</param>
            public SaltData(int saltBlockSize, string passwordToSalt, int saltIterations)
            {
                this.CryptographSalt = new byte[saltBlockSize];
                this.AuthenticationSalt = new byte[saltBlockSize];
                using (RNGCryptoServiceProvider rngCrypto = new RNGCryptoServiceProvider())
                {
                    rngCrypto.GetBytes(this.CryptographSalt);
                    rngCrypto.GetBytes(this.AuthenticationSalt);
                }

                // Generate a cryptographic key that will be used to perform the actual encryption.
                using (Rfc2898DeriveBytes cryptKeyGenerator = new Rfc2898DeriveBytes(passwordToSalt, this.CryptographSalt, saltIterations))
                {
                    this.CryptographKey = cryptKeyGenerator.GetBytes(SaltBitSize / BlockSize);
                }

                // Generate an authentication key that will be used to hash the entire payload and verify it's integrity during decryption.
                using (Rfc2898DeriveBytes authKeyGenerator = new Rfc2898DeriveBytes(passwordToSalt, this.AuthenticationSalt, saltIterations))
                {
                    this.AuthenticationKey = authKeyGenerator.GetBytes(SaltBitSize / BlockSize);
                }
            }

            /// <summary>
            /// Creates a new instance of the SaltData using the password to salt and iterations to 
            /// generate both a Cryptographic Salt, and Key along with an Authentication Salt and associated key.
            /// </summary>
            /// <param name="saltBlockSize">The size of the salt block</param>
            /// <param name="passwordToSalt">The password to create a cryptograph and authentication key</param>
            /// <param name="saltIterations">The number of times to hash the salt when creating the keys.</param>
            /// <param name="authenticationSalt">An existing salt that will be used to generate a key</param>
            /// <param name="cryptographicSalt">An existing cryptograph salt that will be used to generate a new key.</param>
            public SaltData(int saltBlockSize, string passwordToSalt, int saltIterations, byte[] authenticationSalt, byte[] cryptographicSalt)
            {
                this.CryptographSalt = cryptographicSalt;
                this.AuthenticationSalt = authenticationSalt;

                // Restore our cryptograph key so we can decrypt the contents of our encrypted data.
                using (var generator = new Rfc2898DeriveBytes(passwordToSalt, this.CryptographSalt, SaltIterations))
                {
                    this.CryptographKey = generator.GetBytes(SaltBitSize / BlockSize);
                }

                // Restore the authentication key so we can unhash and compare the data we've been given, with the data we hashed during the original encryption.
                using (var generator = new Rfc2898DeriveBytes(passwordToSalt, this.AuthenticationSalt, saltIterations))
                {
                    this.AuthenticationKey = generator.GetBytes(SaltBitSize / BlockSize);
                }
            }

            /// <summary>
            /// The cryptograph salt used to generate the Cryptograph Key
            /// </summary>
            public byte[] CryptographSalt { get; }

            /// <summary>
            /// The authentication salt used to generate the AuthenticationKey
            /// </summary>
            public byte[] AuthenticationSalt { get; }

            /// <summary>
            /// The key used to encrypt data
            /// </summary>
            public byte[] CryptographKey { get; }

            /// <summary>
            /// The key used to hash the overall payload
            /// </summary>
            public byte[] AuthenticationKey { get; }
        }

        // The block size used during the salt and encryption
        private const int BlockSize = 8;

        // The number of bits we're using to generate our Rfc2898 keys.
        private const int SaltBitSize = 128;

        // The size of our cryptograph and authentication keys.
        private const int KeyBitSize = 256;

        // The number of iterations we hash our salt. Instead of using 10,000
        // I use a random number close to 10,000 to make guessing our iterations harder.
        private const int SaltIterations = 9362;

        /// <summary>
        /// Pull the non-secret payload data off of the complete payload.
        /// </summary>
        /// <remarks>
        /// This will pull any public bytes off the total payload that was given back as part of
        /// the encryption process.
        /// This will not return the IV or Salts. Only the user given public information is return.
        /// No decryption is performed since the data given was never encrypted.
        /// </remarks>
        /// <param name="completePayload">The payload given returned during encryption.</param>
        /// <returns>Returns the non-secret data provided during the encryption process.</returns>
        public byte[] GetNonSecretPayload(byte[] completePayload)
        {
            // If we do not have a valid payload, we return an empty array.
            if (completePayload == null || completePayload.Length == 0)
            {
                return new byte[0];
            }

            using (var memoryStream = new MemoryStream(completePayload))
            {
                using (var binaryReader = new BinaryReader(memoryStream))
                {
                    return this.GetNonSecretPayload(binaryReader);
                }
            }
        }

        /// <summary>
        /// Encrypts the secret message using the password provided.
        /// </summary>
        /// <remarks>
        /// The secret message is encrypted using AES 256 + HMACSHA 256.
        /// The password is part of a 3 part key. If you loose the password, you can never unencrypt the data.
        /// If the return encrypted data is tampered with, the decrypting will fail.
        /// </remarks>
        /// <param name="secretMessage">The data you want to encrypt.</param>
        /// <param name="password">The password that will be used to decrypt the data.</param>
        /// <returns>Returns an array of bytes containing the encrypted message.</returns>
        public byte[] EncryptMessageWithPassword(byte[] secretMessage, string password)
        {
            return this.EncryptMessageWithPassword(secretMessage, password, new byte[0]);
        }

        /// <summary>
        /// Encrypts the secret message using the password provided.
        /// </summary>
        /// <remarks>
        /// The secret message is encrypted using AES 256 + HMACSHA 256.
        /// The password is part of a 3 part key. If you loose the password, you can never unencrypt the data.
        /// If the return encrypted data is tampered with, the decrypting will fail.
        /// </remarks>
        /// <param name="secretMessage">The data you want to encrypt.</param>
        /// <param name="password">The password that will be used to decrypt the data.</param>
        /// <returns>Returns a Base64 string containing the encrypted message.</returns>
        public string EncryptMessageWithPassword(string secretMessage, string password)
        {
            // Convert the string to an array of bytes so we can re-use the byte-based EncryptMessageWithPassword method.
            byte[] secreteMessageBytes = Encoding.UTF8.GetBytes(secretMessage);
            byte[] encryptedMessage = this.EncryptMessageWithPassword(secreteMessageBytes, password, new byte[0]);

            // Convert the encrypted data to a string for easier storage.
            return Convert.ToBase64String(encryptedMessage);
        }

        /// <summary>
        /// Encrypts the secret message using the password provided.
        /// </summary>
        /// <remarks>
        /// The secret message is encrypted using AES 256 + HMACSHA 256.
        /// The password is part of a 3 part key. If you loose the password, you can never unencrypt the data.
        /// If the return encrypted data is tampered with, the decrypting will fail.
        /// </remarks>
        /// <param name="secretMessage">The data you want to encrypt.</param>
        /// <param name="password">The password that will be used to decrypt the data.</param>
        /// <param name="nonSecretPayload">Data that is not considered sensitive and can be viewable in the public.</param>
        /// <returns>Returns a Base64 string containing the encrypted message + the non-secret data.</returns>
        public string EncryptMessageWithPassword(string secretMessage, string password, byte[] nonSecretPayload)
        {
            // Convert the string to an array of bytes so we can re-use the byte-based EncryptMessageWithPassword method.
            byte[] secreteMessageBytes = Encoding.UTF8.GetBytes(secretMessage);
            byte[] encryptedMessage = this.EncryptMessageWithPassword(secreteMessageBytes, password, nonSecretPayload);

            // Convert the encrypted data to a string for easier storage.
            return Convert.ToBase64String(encryptedMessage);
        }

        /// <summary>
        /// Encrypts the secret message using the password provided.
        /// </summary>
        /// <remarks>
        /// The secret message is encrypted using AES 256 + HMACSHA 256.
        /// The password is part of a 3 part key. If you loose the password, you can never unencrypt the data.
        /// If the return encrypted data is tampered with, the decrypting will fail.
        /// </remarks>
        /// <param name="secretMessage">The data you want to encrypt.</param>
        /// <param name="password">The password that will be used to decrypt the data.</param>
        /// <param name="nonSecretPayload">Data that is not considered sensitive and can be viewable in the public.</param>
        /// <returns>Returns an array of bytes containing the encrypted message + the non-secret data.</returns>
        public byte[] EncryptMessageWithPassword(byte[] secretMessage, string password, byte[] nonSecretPayload)
        {
            if (string.IsNullOrEmpty(password))
            {
                throw new InvalidOperationException("You can not provide an empty password, you must give a string that is at least 12 characters in size. If you just want to obfuscate the message without any protection, an alternative way is to use a Base64 String");
            }
            else if (password.Length < 12)
            {
                throw new InvalidOperationException("The minimum size your password can be is 12 characters.");
            }

            // Create a new set of salts and keys that can be used during the encryption
            var saltData = new SaltData(BlockSize, password, SaltIterations);
            using (AesCryptoServiceProvider aesProvider = new AesCryptoServiceProvider())
            {
                aesProvider.Key = saltData.CryptographKey;
                aesProvider.GenerateIV();
                aesProvider.Mode = CipherMode.CBC;
                aesProvider.Padding = PaddingMode.PKCS7;

                // Create our encryptor and write the secret message to the encryptor stream.
                byte[] cypherData;
                using (ICryptoTransform encryptor = aesProvider.CreateEncryptor(saltData.CryptographKey, aesProvider.IV))
                {
                    cypherData = this.TransformCryptoData(encryptor, secretMessage);
                }

                // Flatten our saltdata, cypher data, non-secret data and the IV into a single array.
                return FlattenDataToArray(nonSecretPayload, saltData, cypherData, aesProvider.IV);
            }
        }

        /// <summary>
        /// Decrypts the encrypted content provided using the password given.
        /// </summary>
        /// <remarks>
        /// Decrypting can only be done if the encrypted content was originally encrypted using the password
        /// provided and was encrypted using this library.
        /// </remarks>
        /// <param name="encryptedMessage">The encrypted message to decrypt.</param>
        /// <param name="password">The password used during the decryption process.</param>
        /// <returns>Returns the decrypted content as a string</returns>
        public string DecryptMessageWithPassword(string encryptedMessage, string password)
        {
            // We assume the string given is Base64. If not, decryption will fail.
            byte[] encryptedData = Convert.FromBase64String(encryptedMessage);
            byte[] decryptedData = this.DecryptMessageWithPassword(encryptedData, password);

            // Return the decrypted value as a string.
            return Encoding.UTF8.GetString(decryptedData);
        }

        /// <summary>
        /// Decrypts the encrypted content provided using the password given.
        /// </summary>
        /// <remarks>
        /// Decrypting can only be done if the encrypted content was originally encrypted using the password
        /// provided and was encrypted using this library.
        /// </remarks>
        /// <param name="encryptedMessage">The encrypted message to decrypt.</param>
        /// <param name="password">The password used during the decryption process.</param>
        /// <returns>Returns the decrypted content as an array of bytes</returns>
        public byte[] DecryptMessageWithPassword(byte[] encryptedMessage, string password)
        {
            ExplodedResult explodedData = this.ExplodePayload(encryptedMessage, password);
            SaltData saltData = explodedData.SaltData;

            if (!this.ValidateEncrpytedData(encryptedMessage, explodedData))
            {
                return null;
            }

            // Begin decrypting the contents of the cypher data.
            using (AesCryptoServiceProvider aesProvider = new AesCryptoServiceProvider())
            {
                aesProvider.Key = saltData.CryptographKey;
                aesProvider.IV = explodedData.IV;
                aesProvider.Mode = CipherMode.CBC;
                aesProvider.Padding = PaddingMode.PKCS7;

                // Create our encryptor and write the secret message to the encryptor stream.
                using (ICryptoTransform decryptor = aesProvider.CreateDecryptor(aesProvider.Key, aesProvider.IV))
                {
                    // Create a CryptoStream that is used by a BinaryWriter to write the secret message
                    // into the cryptographic stream.
                    using (MemoryStream memoryStreamFordecryptedData = new MemoryStream())
                    {
                        // Write the cypher data to a decryption stream, that we will then convert to an array
                        // of unencrypted byte data.
                        return this.TransformCryptoData(decryptor, explodedData.CypherData);
                    }
                }
            }
        }

        /// <summary>
        /// Verifies that the payload was not tampered with.
        /// </summary>
        /// <param name="payload">The complete payload from the original encryption.</param>
        /// <param name="explodedData">The payload already exploded.</param>
        /// <returns>Returns true if the payload is valid and tamper free.</returns>
        private bool ValidateEncrpytedData(byte[] payload, ExplodedResult explodedData)
        {
            SaltData saltData = explodedData.SaltData;

            // Verify the integrity of the data by unhashing our "copy" of the entire payload, and comparing
            // the copy of the original payload to the payload we are actually being given.
            using (HMACSHA256 hmac = new HMACSHA256(saltData.AuthenticationKey))
            {
                // Compute the hash of our data, not including the authentication data.
                byte[] hashOfDataGiven = hmac.ComputeHash(payload, 0, explodedData.SizeOfPayload);

                // The authentication data will always be the same block size. We take the hash size and 
                // divide by our block size. For instance, a 256 bit hash size, divided by our 8byte block size, will give
                // us a hashsize of 32 bits
                int hashSize = hmac.HashSize / BlockSize;

                // Compare the bytes of our computed hash of the original data, to the stored hash
                // of our original data to test for any tampering of the data.
                // If the hashes don't match, we consider this an invalid set of data and can't decrypt.
                for (int index = 0; index < hashSize; index++)
                {
                    if (hashOfDataGiven[index] != explodedData.AuthenticationHash[index])
                    {
                        return false;
                    }
                }
            }

            return true;
        }

        /// <summary>
        /// Takes the payload from the original encryption, and explodes it.
        /// </summary>
        /// <param name="payload">The original payload from when the data was encrypted.</param>
        /// <param name="password">The password used to encrypt the payload.</param>
        /// <returns>
        /// Returns the payload exploded into pieces representing any non-secrete data,
        /// the cryptographic salt, the authentication salt, the IV and the cypher.
        /// </returns>
        private ExplodedResult ExplodePayload(byte[] payload, string password)
        {
            ExplodedResult result = new ExplodedResult();
            byte[] cryptographSalt;
            byte[] authenticationSalt;

            // Read all of our data in first.
            using (var memoryStream = new MemoryStream(payload))
            {
                using (var binaryReader = new BinaryReader(memoryStream, Encoding.UTF8, true))
                {
                    // We don't need to return the non-secret data, that is what the 
                    // this.GetNonSecretPayload(payload) method is for. We just need
                    // to know how large it is, so we can skip over it to our IV.
                    // Read the non-secret payload if the size is greater than 0.
                    // Otherwise there isn't a non secret payload so we skip.
                    byte[] nonSecretPayload = this.GetNonSecretPayload(binaryReader) ?? new byte[0];

                    // Now that we've moved passed the non-secret payload data, if it existed
                    // we can read our Initialization Vector used during the decryption
                    int ivSize = binaryReader.ReadInt32();
                    result.IV = binaryReader.ReadBytes(ivSize);

                    // Fetch the salt keys
                    cryptographSalt = binaryReader.ReadBytes(BlockSize);
                    authenticationSalt = binaryReader.ReadBytes(BlockSize);

                    // Read the stored size of the cypher data, then read the cypher data itself.
                    int cypherDataSize = binaryReader.ReadInt32();
                    result.CypherData = binaryReader.ReadBytes(cypherDataSize);

                    // Determine what the size of the remaining authentication data is
                    int sizeOfPayloadWithoutAuthenticationData = 
                        + sizeof(Int32)                 // Size of non-secret payload (int)
                        + nonSecretPayload.Length       // Number of elements in the non-secret payload array
                        + sizeof(Int32)                 // Size of IV (int)
                        + result.IV.Length              // Number of elements in the IV array
                        + cryptographSalt.Length        // Number of elements in the cryptographic salt array
                        + authenticationSalt.Length     // Number of elements in the authentication salt array
                        + sizeof(Int32)                 // Size of cypher data
                        + result.CypherData.Length;     // Number of elements in the cypher data array

                    // Determine the size of the authentication content that was hashed using HMac during encryption.
                    int sizeOfAuthenticationData = payload.Length - sizeOfPayloadWithoutAuthenticationData;
                    result.SizeOfPayload = sizeOfPayloadWithoutAuthenticationData;
                    result.AuthenticationHash = binaryReader.ReadBytes(sizeOfAuthenticationData);
                }
            }

            result.SaltData = new SaltData(BlockSize,SaltIterations, password, authenticationSalt, cryptographSalt);
            return result;
        }

        /// <summary>
        /// Writes the given data to the Crypto Transform, transforming the data to the desired transform implementation.
        /// </summary>
        /// <param name="cryptoTransform">The desired ICryptoTransform implementation to transform the bytes into an encrypted/decrypted value</param>
        /// <param name="cryptoData">The data to transform</param>
        /// <returns>Returns the transformed byte collection</returns>
        private byte[] TransformCryptoData(ICryptoTransform cryptoTransform, byte[] cryptoData)
        {
            // Create a CryptoStream that is used by a BinaryWriter to write the secret message
            // into the cryptographic stream.
            using (MemoryStream memoryStreamForEncryptedData = new MemoryStream())
            {
                // Create the crypto stream for the transform we've been given.
                using (CryptoStream cryptoStream = new CryptoStream(memoryStreamForEncryptedData, cryptoTransform, CryptoStreamMode.Write))
                {
                    // Write the data into the writer using the crypto transform.
                    using (BinaryWriter writer = new BinaryWriter(cryptoStream))
                    {
                        writer.Write(cryptoData);
                    }

                    // Pull the encrypted cypher data out of the memory stream and return it.
                    return memoryStreamForEncryptedData.ToArray();
                }
            }
        }

        /// <summary>
        /// Pulls the non-secret payload if one exists from the underlying stream.
        /// </summary>
        /// <param name="binaryReader">The reader that will be used to read the non-secret data</param>
        /// <returns>Returns the non-secret data, or null if none exists.</returns>
        private byte[] GetNonSecretPayload(BinaryReader binaryReader)
        {
            // Determine the length of the non-secret payload. 
            // If it's 0, then we return an empty array.
            int payloadSize = binaryReader.ReadInt32();
            if (payloadSize == 0)
            {
                return null;
            }

            // Return the original un-encrypted non-secret payload.
            return binaryReader.ReadBytes(payloadSize);
        }

        /// <summary>
        /// Flattens the non-secret payload, the cryptographic salt, authentication salt, cypher data and IV into a single array.
        /// </summary>
        /// <param name="nonSecretPayload">The non-secret payload that was not included in the encryption</param>
        /// <param name="saltData">SaltData containing the salt keys used during the decryption</param>
        /// <param name="cypherData">The encrypted data.</param>
        /// <param name="aesProvider">The provider used during the encryption. This is used to pull the IV out of.</param>
        /// <returns>Returns an array of bytes representing the values given to flatten.</returns>
        private byte[] FlattenDataToArray(byte[] nonSecretPayload, SaltData saltData, byte[] cypherData, byte[] initializationVector)
        {
            using (MemoryStream memoryStream = new MemoryStream())
            {
                // Write our IV out first so we can pull the IV off later during decryption.
                // The IV does not need to be encrypted, it is safe to store as as unencrypted buffer in the encrypted byte array.
                using (BinaryWriter publicDataWriter = new BinaryWriter(memoryStream, Encoding.UTF8, true))
                {
                    // The first two writes to the stream should be the size of the non-secret payload
                    // and the payload itself if one exists.
                    if (nonSecretPayload == null || nonSecretPayload.Length == 0)
                    {
                        publicDataWriter.Write(0);
                    }
                    else
                    {
                        publicDataWriter.Write(nonSecretPayload.Length);
                        publicDataWriter.Write(nonSecretPayload);
                    }

                    // Write the Initialization Vector size and the value.
                    publicDataWriter.Write(initializationVector.Length);
                    publicDataWriter.Write(initializationVector);

                    // Write out our salts so we can decrypt and authenticate during decryption
                    publicDataWriter.Write(saltData.CryptographSalt);
                    publicDataWriter.Write(saltData.AuthenticationSalt);

                    // Write out the size of our encrypted data + the encrypted data
                    // so we know how much to read in during decryption.
                    publicDataWriter.Write(cypherData.Length);
                    publicDataWriter.Write(cypherData);
                    publicDataWriter.Flush();

                    byte[] authenticationData = this.HashStreamUsingSha256(memoryStream, saltData.AuthenticationKey);
                    publicDataWriter.Write(authenticationData);
                }

                return memoryStream.ToArray();
            }
        }

        /// <summary>
        /// Hashes the contents of the memory stream using the authentication key provided.
        /// </summary>
        /// <param name="memoryStream">The memory stream that will have its contents hashed.</param>
        /// <param name="authenticationKey">The authentication key used to perform the hashing.</param>
        /// <returns>Returns the hashed content of the memory stream</returns>
        private byte[] HashStreamUsingSha256(MemoryStream memoryStream, byte[] authenticationKey)
        {
            byte[] authenticationData;

            // Hash the entire contents of our stream, and write it back into the stream for verification during decryption
            using (HMACSHA256 authentication = new HMACSHA256(authenticationKey))
            {
                authenticationData = authentication.ComputeHash(memoryStream.ToArray());
            }

            return authenticationData;
        }
    }
}

I used the following unit tests during the refactor to make sure I didn't break our existing API. My test suite could be better, but this at least covers the basics and lets you run through and debug the code easier if you want to.

Encryption.Tests.cs

using System;
using System.Text;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace EncryptionTools
{
    [TestClass]
    public class EncryptionTests
    {
        /// <summary>
        /// The content we will encrypt during the tests
        /// </summary>
        private const string _ContentToEncrypt = "This is a test to make sure the encryption Type actually encrypts the data right.";

        /// <summary>
        /// The password used to encrypt and decrypt the content during testing
        /// </summary>
        private const string _Password = "EncryptedPassword1";

        [TestMethod]
        [ExpectedException(typeof(InvalidOperationException))]
        public void Encrypt_with_null_password_throws_exception()
        {
            // Arrange
            var encryption = new Encryption();

            // Act
            string encryptedValue = encryption.EncryptMessageWithPassword(_ContentToEncrypt, string.Empty);

            // Assert
            Assert.Fail();
        }

        [TestMethod]
        [ExpectedException(typeof(InvalidOperationException))]
        public void Encrypt_with_to_small_of_a_password_fails()
        {
            // Arrange
            var encryption = new Encryption();

            // Act
            string encryptedValue = encryption.EncryptMessageWithPassword(_ContentToEncrypt, "123456789");

            // Assert
            Assert.Fail();
        }

        [TestMethod]
        public void Encrypt_encoded_bytes()
        {
            // Arrange
            var encryption = new Encryption();
            byte[] bytesToEncrypt = Encoding.UTF8.GetBytes(_ContentToEncrypt);

            // Act
            byte[] encryptedBytes = encryption.EncryptMessageWithPassword(bytesToEncrypt, _Password);

            // Assert
            Assert.AreNotEqual(bytesToEncrypt, encryptedBytes);
        }

        [TestMethod]
        public void Encrypt_string_content()
        {
            // Arrange
            var encryption = new Encryption();

            // Act
            string encryptedValue = encryption.EncryptMessageWithPassword(_ContentToEncrypt, _Password);

            // Assert
            Assert.AreNotEqual(_ContentToEncrypt, encryptedValue);
        }

        [TestMethod]
        public void Decrypt_encrypted_bytes()
        {
            // Arrange
            var encryption = new Encryption();
            byte[] bytesToEncrypt = Encoding.UTF8.GetBytes(_ContentToEncrypt);
            byte[] encryptedBytes = encryption.EncryptMessageWithPassword(bytesToEncrypt, _Password);

            // Act
            byte[] decryptedBytes = encryption.DecryptMessageWithPassword(encryptedBytes, _Password);
            string decryptedContent = Encoding.UTF8.GetString(decryptedBytes);

            // Assert
            Assert.AreEqual(_ContentToEncrypt, decryptedContent);
        }

        [TestMethod]
        public void Decrypt_with_different_password_does_not_return_decrypted_value()
        {
            // Arrange
            var encryption = new Encryption();
            byte[] bytesToEncrypt = Encoding.UTF8.GetBytes(_ContentToEncrypt);
            byte[] encryptedBytes = encryption.EncryptMessageWithPassword(bytesToEncrypt, _Password);

            // Act
            byte[] decryptedBytes = encryption.DecryptMessageWithPassword(encryptedBytes, _Password.Substring(1));

            // Assert
            Assert.IsNull(decryptedBytes);
        }

        [TestMethod]
        public void Decrypt_string_content()
        {
            // Arrange
            var encryption = new Encryption();
            string encryptedValue = encryption.EncryptMessageWithPassword(_ContentToEncrypt, _Password);

            // Act
            string decryptedValue = encryption.DecryptMessageWithPassword(encryptedValue, _Password);

            // Assert
            Assert.AreEqual(_ContentToEncrypt, decryptedValue);
        }

        [TestMethod]
        public void Extract_non_secret_payload_content_from_encrypted_string()
        {
            // Arrange
            var encryption = new Encryption();
            string nonSecretData = "This payload is not considered secret and can be pulled out of the payload without decrypting";

            // Convert the secret and non-secret data into a byte array
            byte[] payload = Encoding.UTF8.GetBytes(nonSecretData);
            byte[] encodedBytes = Encoding.UTF8.GetBytes(_ContentToEncrypt);

            // Encrypt the secret data while injecting the nonsecret payload into the encrypted stream.
            byte[] encryptedValue = encryption.EncryptMessageWithPassword(encodedBytes, _Password, payload);

            // Act
            // Pull the non-secret payload out of the encrypted message - without having to decrypt it.
            byte[] UnencryptedPayloadWithinEncryptedArray = encryption.GetNonSecretPayload(encryptedValue);
            string payloadContent = Encoding.UTF8.GetString(UnencryptedPayloadWithinEncryptedArray);

            // Assert
            Assert.AreEqual(nonSecretData, payloadContent);
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

EncryptMessageWithPassword(string secretMessage, string password)

This method and its overloaded version are sharing almost identical code. I would call the overloaded version from this one like so

public string EncryptMessageWithPassword(string secretMessage, string password)
{
     return EncryptMessageWithPassword(secretMessage, password, new byte[0]);
}

and while we are at this methods, I miss a proper parameter validation. Sure, if e.g secretMessage == null an ArgumentNullException is thrown by the GetBytes() method of the Encoding but this will expose an implementation detail of your code. A user of the code doesn't need to know that you are using Encoding.UTF8 he/she only needs to know that passing null as a password will result in an ArgumentNullException thrown by the method itself like so (needs to be expanded to validate the remaining parameters)

public string EncryptMessageWithPassword(string secretMessage, string password)
{
     if (secretMessage == null) 
     {
         throw new ArgumentNullException(nameof(secretMessage), "Parameter is null");
     }
     return EncryptMessageWithPassword(secretMessage, password, new byte[0]);
}  

but using a string for the password is considered insecure.


You are using a little bit to many comments in your code. Most of them are obvious because you already named your methods, parameters and variables good and meaningful. Having to many comments will decrease the readability because it makes it hard to grasp the code at first glance.

Sometimes the comments are misleading as well like here

   private byte[] TransformCryptoData(ICryptoTransform cryptoTransform, byte[] cryptoData)
    {
        // Create a CryptoStream that is used by a BinaryWriter to write the secret message
        // into the cryptographic stream.
        using (MemoryStream memoryStreamForEncryptedData = new MemoryStream())
        {
            // Create the crypto stream for the transform we've been given.
            using (CryptoStream cryptoStream = new CryptoStream(memoryStreamForEncryptedData, cryptoTransform, CryptoStreamMode.Write))
            {
                // Write the data into the writer using the crypto transform.
                using (BinaryWriter writer = new BinaryWriter(cryptoStream))
                {
                    writer.Write(cryptoData);
                }
                // Pull the encrypted cypher data out of the memory stream and return it.  
                return memoryStreamForEncryptedData.ToArray();
            }
        }
    }  

You are stating two times that a cryptostream is created but at the first time you are creating a memorystream.

In addition you could simply stack the usings like so

    private byte[] TransformCryptoData(ICryptoTransform cryptoTransform, byte[] cryptoData)
    {
        using (MemoryStream memoryStreamForEncryptedData = new MemoryStream())
        using (CryptoStream cryptoStream = new CryptoStream(memoryStreamForEncryptedData, cryptoTransform, CryptoStreamMode.Write))
        using (BinaryWriter writer = new BinaryWriter(cryptoStream))
        {
            writer.Write(cryptoData);
        }
        return memoryStreamForEncryptedData.ToArray();
    }  

which reduces the horizontal spacing resulting in increased readability.


   public byte[] DecryptMessageWithPassword(byte[] encryptedMessage, string password)
    {
        ExplodedResult explodedData = this.ExplodePayload(encryptedMessage, password);
        SaltData saltData = explodedData.SaltData;

        if (!this.ValidateEncrpytedData(encryptedMessage, explodedData))
        {
            return null;
        }

        // Begin decrypting the contents of the cypher data.
        using (AesCryptoServiceProvider aesProvider = new AesCryptoServiceProvider())
        {
            aesProvider.Key = saltData.CryptographKey;
            aesProvider.IV = explodedData.IV;
            aesProvider.Mode = CipherMode.CBC;
            aesProvider.Padding = PaddingMode.PKCS7;

            // Create our encryptor and write the secret message to the encryptor stream.
            using (ICryptoTransform decryptor = aesProvider.CreateDecryptor(aesProvider.Key, aesProvider.IV))
            {
                // Create a CryptoStream that is used by a BinaryWriter to write the secret message
                // into the cryptographic stream.
                using (MemoryStream memoryStreamFordecryptedData = new MemoryStream())
                {
                    // Write the cypher data to a decryption stream, that we will then convert to an array
                    // of unencrypted byte data.
                    return this.TransformCryptoData(decryptor, explodedData.CypherData);
                }
            }
        }
    }  

here the created memorystream is never used. Get rid of it.


The overloaded methods GetNonSecretPayload(byte[]) and GetNonSecretPayload(BinaryReader) are to far apart. If one reads the code of the first method he/she needs to scroll almost to the bottom of the class to find the second method. You should group your methods better.


ExplodePayload()

I don't quite understand why you are using that overloaded constructor of the BinaryReader. Why do you need the underlaying stream to be left open after the reader is disposed ? IMO this doesn't buy you anything so use the default constructor which takes only a Stream as parameter. This reduces the amount of code because it will use Encoding.UTF8 by default.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the review. I'll clean up the unused memory stream and fix my misleading comments. I am usually not as verbose with my comments, but it took me two days to figure out what all of the Cryptographic classes were and how they were used. I wanted to try and help the next person who has to maintain this code out by avoiding the need to research as much as I did for the original implementation. Good catch on the BinaryReader overload usage. That's an artifact from prior to my refactoring. \$\endgroup\$ May 9 '16 at 2:39
  • \$\begingroup\$ The GetNonSecretPayload overload is a private method, so it's lumped in with the other private methods. I can move the public version of that method down to the bottom so it is the last public method, and the private overload as the first private method to group them together thanks. The rest of the method grouping throughout the code is difficult since they're private and private methods go after public/internal/protected etc. \$\endgroup\$ May 9 '16 at 2:41
  • \$\begingroup\$ In our scenario, we're loading the password from an environment variable on the server, or a web config. When we load the password, even if the developer converts it to a char before it hits my Decrypt/Encrypt methods, the string containing it will already exist in memory. How would you solve for that when storing the password and loading it at runtime? \$\endgroup\$ May 9 '16 at 2: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.