11
\$\begingroup\$

I'm trying to symmetrically encrypt some data using C#, and there seems to be a lot of misleading or incorrect information out there on the subject.

I created a project on GitHub to act as some sort of standard for encrypting and decrypting data in C#: Encryptamajig on Github

It would be great if you guys could review the code below for any security holes, but also, please check out the verbiage in the README on Github. I tried to do some research and be as accurate as possible, but I'm sure that there are some things I'm missing.

If you do find anything, don't hesitate to send me a pull request.

namespace Encryptamajig
{
    using System;
    using System.Collections.Generic;
    using System.Linq;
    using System.Text;
    using System.Security.Cryptography;
    using System.Diagnostics;
    using System.IO;

    /// <summary>
    /// A simple wrapper to the AesManaged class and the AES algorithm.
    /// To create a new Key and IV simple "new up" an AesManaged object and grab the Key and IV from that.
    /// Make sure to save the Key and IV if you want to decrypt your data later!
    /// </summary>
    public class AesEncryptamajig
    {
        public static byte[] EncryptStringToBytes(string plainText, byte[] key, byte[] iv)
        {
            // Check arguments.
            if (string.IsNullOrEmpty(plainText))
                throw new ArgumentNullException("plainText");
            if (key == null || key.Length <= 0)
                throw new ArgumentNullException("key");
            if (iv == null || iv.Length <= 0)
                throw new ArgumentNullException("iv");

            MemoryStream memoryStream = null;
            AesManaged aesAlg = null;

            try
            {
                // Create the encryption algorithm object with the specified key and IV.
                aesAlg = new AesManaged();
                aesAlg.Key = key;
                aesAlg.IV = iv;

                // Create an encryptor to perform the stream transform.
                var encryptor = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV);

                // Create the streams used for encryption.
                memoryStream = new MemoryStream();

                using (var cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))
                using (var streamWriter = new StreamWriter(cryptoStream))
                {
                    //Write all data to the stream.
                    streamWriter.Write(plainText);
                }
            }
            finally
            {
                if (aesAlg != null)
                    aesAlg.Clear();
            }

            // Return the encrypted bytes from the memory stream.
            return memoryStream.ToArray();
        }

        public static string DecryptStringFromBytes(byte[] cipherText, byte[] key, byte[] iv)
        {
            // Check arguments.
            if (cipherText == null || cipherText.Length <= 0)
                throw new ArgumentNullException("cipherText");
            if (key == null || key.Length <= 0)
                throw new ArgumentNullException("key");
            if (iv == null || iv.Length <= 0)
                throw new ArgumentNullException("iv");

            AesManaged aesAlg = null;
            string plaintext = null;

            try
            {
                // Create a the encryption algorithm object with the specified key and IV.
                aesAlg = new AesManaged();
                aesAlg.Key = key;
                aesAlg.IV = iv;

                // Create a decrytor to perform the stream transform.
                var decryptor = aesAlg.CreateDecryptor(aesAlg.Key, aesAlg.IV);

                // Create the streams used for decryption.
                using (var memoryStream = new MemoryStream(cipherText))
                using (var cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Read))
                using (var streamReader = new StreamReader(cryptoStream))
                {
                    // Read the decrypted bytes from the decrypting stream
                    // and place them in a string.
                    plaintext = streamReader.ReadToEnd();
                }
            }
            finally
            {
                if (aesAlg != null)
                    aesAlg.Clear();
            }

            return plaintext;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Why the using inside the namespace? Is that just style, or something else? \$\endgroup\$ – ANeves Jan 30 '12 at 15:30
  • 1
    \$\begingroup\$ There is a subtle difference outlined in this StackOverflow question. \$\endgroup\$ – John Bubriski Jan 30 '12 at 16:30
  • 1
    \$\begingroup\$ @ANeves It's a Resharper thing. Helps with scoping. \$\endgroup\$ – lorddev Oct 22 '12 at 1:49
  • \$\begingroup\$ I'm getting an invalid padding exception when trying to decrypt. \$\endgroup\$ – lorddev Oct 23 '12 at 3:31
6
\$\begingroup\$

I wouldn't have the IV passed in either routine. There is two much potential for it to be used incorrectly. I've seen lots of examples out there which will either reuse the IV all the time or just use the same IV per Key. If your IV is predictable there are possible exploits for an adversary to get your plain text, such as a padding oracle since your above uses CBC.

The best practice would be to use aesAlg.GenerateIV() and then prepend the iv to the cipher text. When you decrypt you just read that block out first before you decrypt the rest.

\$\endgroup\$
5
\$\begingroup\$

You need to Dispose() of your disposable objects (those which implement IDisposable: MemoryStream in EncryptStringToBytes and AesManaged and the encryptor in both methods. They can be simplified as such:

namespace Encryptamajig
{
    using System;
    using System.IO;
    using System.Security.Cryptography;

    /// <summary>
    /// A simple wrapper to the AesManaged class and the AES algorithm.
    /// To create a new Key and IV simple "new up" an AesManaged object and grab the Key and IV from that.
    /// Make sure to save the Key and IV if you want to decrypt your data later!
    /// </summary>
    public static class AesEncryptamajig
    {
        public static byte[] EncryptStringToBytes(string plainText, byte[] key, byte[] iv)
        {
            // Check arguments.
            if (string.IsNullOrEmpty(plainText))
                throw new ArgumentNullException("plainText");
            if (key == null || key.Length <= 0)
                throw new ArgumentNullException("key");
            if (iv == null || iv.Length <= 0)
                throw new ArgumentNullException("iv");

            // Create the encryption algorithm object with the specified key and IV.
            using (var aesAlg = new AesManaged { Key = key, IV = iv })
            // Create an encryptor to perform the stream transform.
            using (var encryptor = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV))
            // Create the streams used for encryption.
            using (var memoryStream = new MemoryStream())
            using (var cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))
            using (var streamWriter = new StreamWriter(cryptoStream))
            {
                // Write all data to the stream.
                streamWriter.Write(plainText);

                // Return the encrypted bytes from the memory stream.
                return memoryStream.ToArray();
            }
        }

        public static string DecryptStringFromBytes(byte[] cipherText, byte[] key, byte[] iv)
        {
            // Check arguments.
            if (cipherText == null || cipherText.Length <= 0)
                throw new ArgumentNullException("cipherText");
            if (key == null || key.Length <= 0)
                throw new ArgumentNullException("key");
            if (iv == null || iv.Length <= 0)
                throw new ArgumentNullException("iv");

            // Create a the encryption algorithm object with the specified key and IV.
            using (var aesAlg = new AesManaged { Key = key, IV = iv })
            // Create a decryptor to perform the stream transform.
            using (var decryptor = aesAlg.CreateDecryptor(aesAlg.Key, aesAlg.IV))
            // Create the streams used for decryption.
            using (var memoryStream = new MemoryStream(cipherText))
            using (var cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Read))
            using (var streamReader = new StreamReader(cryptoStream))
            {
                // Read the decrypted bytes from the decrypting stream
                // and place them in a string.
                return streamReader.ReadToEnd();
            }
        }
    }
}
\$\endgroup\$
2
\$\begingroup\$

A not encryption related note: I'd change the following

AesManaged aesAlg = null;
try {
    aesAlg = new AesManaged();
    ...
} finally {
    if (aesAlg != null)
        aesAlg.Clear();
}

to a simpler one:

AesManaged aesAlg = new AesManaged();
try {
    ...
} finally {
    aesAlg.Clear();
}

If the constructor of AesManaged throws an exception the reference remains null, so Clear won't be called.

\$\endgroup\$

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.