6
\$\begingroup\$

I have written a tool for encrypting string using the AesCryptoServiceProvider in .NET. The folllowing parameters are used:

  • Block Cipher Mode: CBC
  • Initialization Vector: 16 bytes (randomized per encryption operation), stored in last 16 bytes of final message
  • Key/Block size: 128 bit Padding: PKCS #7

Overview of code I have added two functions below. The first prepares a message for encryption, takes the result and appends the Initialization Vector to the cipher message. The second function encrypts the provided message using the specified key and initialization vector.

Obviously, i'm interested in any security holes that exist with this code. Also any optimizations are very much appreciated.

Side questions. In development I always store bytes as strings in their hex representation, seen below. Is this bad? The only other alternative I see is base64 encoding? Which is better?

public void cryptoExample {

    AesCryptoServiceProvider aes = new AesCryptoServiceProvider();
    aes.KeySize = 128;
    aes.BlockSize = 128;

    aes.Key = Helper.StringToByteArray("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"); // Sample key
    aes.GenerateIV();

    String plainMessage = "Hello AES Crypto!!";

    String encryptedMessage = Helper.ByteArrayToString(CryptoHelper.EncryptStringToBytes_Aes(plainMessage, aes.Key, aes.IV));

    // Must always use a random IV, meaning it needs to be stored with the cipher message.
    // Append it to the end. IV is a constant 16 bytes so we just extract and remove it before decryption.
    String strIv = Helper.ByteArrayToString(aes.IV);

    String messageToTransmit = encryptedMessage += strIv;
}
public static byte[] EncryptStringToBytes_Aes(string plainText, byte[] Key, byte[] IV)
{
    // Check arguments.
    if (plainText == null || plainText.Length <= 0)
        throw new ArgumentNullException("plainText");
    if (Key == null || Key.Length <= 0)
        throw new ArgumentNullException("Key");
    if (IV == null || IV.Length <= 0)
        throw new ArgumentNullException("IV");
    byte[] encrypted;
    // Create an AesCryptoServiceProvider object
    // with the specified key and IV.
    using (AesCryptoServiceProvider aesAlg = new AesCryptoServiceProvider())
    {
        aesAlg.Key = Key;
        aesAlg.IV = IV;
        aesAlg.Mode = CipherMode.CBC;
        aesAlg.Padding = PaddingMode.PKCS7;
        // Create a decrytor to perform the stream transform.
        ICryptoTransform encryptor = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV);

        // Create the streams used for encryption.
        using (MemoryStream msEncrypt = new MemoryStream())
        {
            using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
            {
                using (StreamWriter swEncrypt = new StreamWriter(csEncrypt))
                {

                    //Write all data to the stream.
                    swEncrypt.Write(plainText);
                }
                encrypted = msEncrypt.ToArray();
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Base64 or Hex. Hex coding (with 'smart' algorithms) is slightly faster to encode/decode than Base64, but is about 25% bigger for IO. Base64 encoding/decoding is almost as fast as Hex, but is smaller, so overall could be faster if IO is involved. Huh, JonSkeet answers: stackoverflow.com/a/3183880/1305253 \$\endgroup\$ – rolfl Dec 1 '13 at 17:26
  • \$\begingroup\$ Great, I think for basic representation of small data I will go with Hex for readability. Base64 for large blobs. Cheers! \$\endgroup\$ – jim Dec 2 '13 at 10:17
6
\$\begingroup\$
  1. You can simplify it by replacing all those streams with a call to encrypter.TransformFinalBlock.
  2. You don't have a MAC, so an attacker who can send a modifier message to the decrypter can probably exploit a padding oracle to decrypt the message
  3. Why ask the caller for an IV? You could generate it internally. If you want to keep a deterministic implementation around, at least create an overload without IV.
  4. Helper.ByteArrayToString and StringToByteArray should have a name that says which encoding (hex, Base64, etc.) they use. I'd use BytesToHexString and HexStringToBytes.
\$\endgroup\$
  • \$\begingroup\$ Thanks for this. I will look into TransformFinalBlock. Could you please describe how to incorporate a MAC? Also, silly me, no need to pass in the IV! \$\endgroup\$ – jim Dec 2 '13 at 10:53
  • \$\begingroup\$ Generating the IV internally and then also appending it to the crypto string internally is quite smart, given it would ensure that it is unique for each message, rather than relying on the user to implement it correctly. \$\endgroup\$ – Tom Heard Feb 24 '14 at 2:08
5
\$\begingroup\$

Few tiny notes:

  1. In the method cryptoExample, the usage of AesCryptoServiceProvider should be wrapped in a using block much like it is in the EncryptStringToBytes_Aes method.

  2. Similarly, ICryptoTransform encryptor = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV) in the EncryptStringToBytes_Aes method is an IDisposable resource and should also be wrapped in a using block.

  3. The comment above that line says "create a decryptor", yet, it's creating an encryptor. Will confuse future readers.

\$\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.