Symmetric encryption/decryption routine using AES

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

• C# uses PascalCase naming convention for method names.

-
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))
{

-
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()
    {
        ....
    }
}

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

StackExchange Code Review Q#13714, answer score: 2