Symmetric encryption/decryption routine using AES

Question

The following is a symmetric encryption/decryption routine using AES in GCM mode. This code operates in the application layer, and is meant to receive user specific and confidential information and encrypt it, after which it is stored in a separate database server. It also is called upon to decrypt encrypted information from the database.

I am looking for a review of the code with respect to its level of security, which in this case refers primarily to the correct implementation of the class called AuthenticatedAesCng found here:

http://clrsecurity.codeplex.com/

My encryption/decryption routines are based on the code found here:

source 1

Any comments or advice is very much appreciated.

Here is the code:

class encryptionHelper
{
 // Do not change.
 private static int IV_LENGTH = 12;
 private static int TAG_LENGTH = 16;
 // EncryptString - encrypts a string
 // Pre: passed a non-empty string
 // Post: returns the encrypted string in the format [IV]-[TAG]-[DATA]
 public static string EncryptString(string str)
 {
 if (String.IsNullOrEmpty(str))
 {
 throw new ArgumentNullException("encryption string invalid");
 }
 using (AuthenticatedAesCng aes = new AuthenticatedAesCng())
 {
 byte[] message = Encoding.UTF8.GetBytes(str); // Convert to bytes.
 aes.Key = getEncryptionKey(); // Retrieve Key.
 aes.IV = generateIV(); // Generate nonce.
 aes.CngMode = CngChainingMode.Gcm; // Set Cryptographic Mode.
 aes.AuthenticatedData = getAdditionalAuthenticationData(); // Set Authentication Data.
 using (MemoryStream ms = new MemoryStream())
 {
 using (IAuthenticatedCryptoTransform encryptor = aes.CreateAuthenticatedEncryptor())
 {
 using (CryptoStream cs = new CryptoStream(ms, encryptor, CryptoStreamMode.Write))
 { 
 // Write through and retrieve encrypted data.
 cs.Write(message, 0, message.Length);
 cs.FlushFinalBlock();
 byte[] cipherText = ms.ToArray(); 
 // Retrieve tag and create array to hold encrypted data.
 byte[] authenticationTag = encryptor.GetTag(); 
 byte[] encrypted = new byte[cipherText.Length + aes.IV.Length + authenticationTag.Length];
 // Set needed data in byte array.
 aes.IV.CopyTo(encrypted, 0); 
 authenticationTag.CopyTo(encrypted, IV_LENGTH);
 cipherText.CopyTo(encrypted, IV_LENGTH + TAG_LENGTH);
 // Store encrypted value in base 64.
 return Convert.ToBase64String(encrypted);
 }
 }
 }
 } 
 }
 // DecryptString - decrypts a string
 // Pre: passed the base 64 string from the database to be decrypted
 // Post: returns the decrypted string
 public static string DecryptString(string str)
 {
 if (String.IsNullOrEmpty(str))
 {
 throw new ArgumentNullException("decryption string invalid");
 }
 using (AuthenticatedAesCng aes = new AuthenticatedAesCng())
 {
 byte[] encrypted = Convert.FromBase64String(str); // Convert string to bytes.
 aes.Key = getEncryptionKey(); // Retrieve Key.
 aes.IV = getIV(encrypted); // Parse IV from encrypted text.
 aes.Tag = getTag(encrypted); // Parse Tag from encrypted text.
 encrypted = removeTagAndIV(encrypted); // Remove Tag and IV for proper decryption.
 aes.CngMode = CngChainingMode.Gcm; // Set Cryptographic Mode.
 aes.AuthenticatedData = getAdditionalAuthenticationData(); // Set Authentication Data.
 using (MemoryStream ms = new MemoryStream())
 {
 using (ICryptoTransform decryptor = aes.CreateDecryptor())
 {
 using (CryptoStream cs = new CryptoStream(ms, decryptor, CryptoStreamMode.Write))
 {
 // Decrypt through stream.
 cs.Write(encrypted, 0, encrypted.Length);
 cs.FlushFinalBlock();
 // Remove from stream and convert to string.
 byte[] decrypted = ms.ToArray();
 return Encoding.UTF8.GetString(decrypted);
 }
 }
 } 
 }
 }
 // getEncryptionKey - retrieves encryption key from somewhere close to Saturn.
 // Pre: nada.
 // Post: Don't worry bout it
 private static byte[] getEncryptionKey()
 {
 // Normally some magic to retrieve the key.
 // For now just hard code it.
 byte[] key = { 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00 };
 return key;
 }
 // generateIV - generates a random 12 byte IV.
 // Pre: none.
 // Post: returns the random nonce.
 private static byte[] generateIV()
 {
 using (RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider())
 {
 byte[] nonce = new byte[IV_LENGTH];
 rng.GetBytes(nonce);
 return nonce;
 }
 }
 // getAdditionalAuthenticationData - retrieves authentication data.
 // Pre: none;.
 // Post: returns the AAD as a byte array.
 private static byte[] getAdditionalAuthenticationData()
 {
 // hardcode for now
 string str_1 = "A promise that I know the key";
 return Encoding.UTF8.GetBytes(str_1);
 }
 // getTag - parses authentication tag from the ciphertext.
 // Pre: passed the byte array.
 // Post: returns the tag as a byte array.
 private static byte[] getTag(byte[] arr)
 {
 byte[] tag = new byte[TAG_LENGTH];
 Array.Copy(arr, IV_LENGTH, tag, 0, TAG_LENGTH);
 return tag;
 }
 // getIV - parses IV from ciphertext.
 // Pre: Passed the ciphertext byte array.
 // Post: Returns byte array containing the IV.
 private static byte[] getIV(byte[] arr)
 {
 byte[] IV = new byte[IV_LENGTH];
 Array.Copy(arr, 0, IV, 0, IV_LENGTH);
 return IV;
 }
 // removeTagAndIV - removes the tag and IV from the byte array so it may be decrypted.
 // Pre: Passed the ciphertext byte array.
 // Post: Peturns a byte array consisting of only encrypted data.
 private static byte[] removeTagAndIV(byte[] arr)
 {
 byte[] enc = new byte[arr.Length - TAG_LENGTH - IV_LENGTH];
 Array.Copy(arr, IV_LENGTH + TAG_LENGTH, enc, 0, arr.Length - IV_LENGTH - TAG_LENGTH);
 return enc;
 }
}

• Bill Barry: There are specific style guides that Microsoft publishes regarding C# documentation comments and capitalization conventions.
• crawfish: I'm used to writing c++ comments and sometimes forget formatting, sorry
• jbtule: It's not Authentication Data, but Authenticated Data, often referred to as Additional Authenticated Data. The idea is that you can have data that doesn't need to be encrypted and needs to be included with the cipher text. You still need it to be authenticated for integrity -- so that you know that this additional unencrypted data hasn't been tampered with. It is also optional you don't need to use it.

Answer 1

1. C# uses PascalCase naming convention for method names.

2. It is accepted practice to reduce indent with multiple using blocks like this (in cases where no additional statements have to be executed in the outer using block):

   using (MemoryStream ms = new MemoryStream())
   using (IAuthenticatedCryptoTransform encryptor = aes.CreateAuthenticatedEncryptor())
   using (CryptoStream cs = new CryptoStream(ms, encryptor, CryptoStreamMode.Write))
   { 
   

3. You have some methods to copy individual elements out of the array containing the encrypted data (getIV, getTag) but you manually fill it in in the encryption method. Also you have delete the IV and tag to get just the data. I would encapsulate this in a EncryptedMessage class where you can set these properties individually and it deals with the layout of it. Something along these lines:

   public class EncryptedMessage
   {
    public byte[] IV { get; set; }
    public byte[] Tag { get; set; }
    public byte[] Data { get; set; }
    private EncryptedMessage(byte[] iv, byte[] tag, byte[] data)
    {
    IV = iv;
    Tag = tag;
    Data = data;
    }
    public static EncryptedMessage FromBase64String(string input)
    {
    ...
    }
    public string ToBase64String()
    {
    ....
    }
   }
   

4. I would not make this class a static class. If you have a few places where you want to use this then make it non-static and give it an interface like IStringEncrypter which you pass around. This will ease unit testing and remove an implicit dependency on the static helper class.