trjhtr

The goal of the code below is to:

• Support authenticated encryption using AES in the CBC mode and using HMAC SHA.


• Support encryption and decryption of data of size larger than memory (potentially).

Please criticize :)

public class AesCbcThenHmacSha : IDisposable

 private readonly Func<HMAC> hmacShaTransformFactory;
 private readonly AesCryptoServiceProvider aesProvider;
 private bool disposed;

 public AesCbcThenHmacSha(byte aesKey, byte aesIv, byte macKey)
 
 if (aesKey == null) throw new ArgumentNullException(nameof(aesKey));
 if (aesIv == null) throw new ArgumentNullException(nameof(aesIv));
 if (macKey == null) throw new ArgumentNullException(nameof(macKey));

 // key size
 // --------
 // the key size property is set based on the provided
 // key. the validation of length is also performed
 // by the AesCryptoServiceProvider.

 // block size
 // ----------
 // there is only one valid block size for AES and
 // it does not need to be specified explicitly. it
 // is 128 by default (and cannot be changed to any
 // different value).

 // initialization vector
 // ---------------------
 // the iv has to match the block size, so validation
 // of the length is also performed beneath.

 this.aesProvider = new AesCryptoServiceProvider
 
 Key = aesKey,
 IV = aesIv,
 Padding = PaddingMode.PKCS7,
 Mode = CipherMode.CBC
 ;

 this.hmacShaTransformFactory = CreateHmacShaTransform(macKey);
 

 private static Func<HMAC> CreateHmacShaTransform(byte key)
 
 var bits = key.Length * 8;

 switch (bits)
 
 case 256: return () => new HMACSHA256(key);
 case 384: return () => new HMACSHA384(key);
 case 512: return () => new HMACSHA512(key);

 default:
 throw new ArgumentException($"the bits-bit HMAC SHA is not supported");
 
 

 public void Encrypt(Stream inputStream, Stream outputStream, out byte hmacSha)
 
 if (inputStream == null) throw new ArgumentNullException(nameof(inputStream));
 if (outputStream == null) throw new ArgumentNullException(nameof(outputStream));

 var hmacShaTransform = this.hmacShaTransformFactory.Invoke();
 var aesTransform = this.aesProvider.CreateEncryptor();

 var hmacShaStream = new CryptoStream(outputStream, hmacShaTransform, CryptoStreamMode.Write);
 var aesStream = new CryptoStream(hmacShaStream, aesTransform, CryptoStreamMode.Write);

 inputStream.CopyTo(aesStream);
 aesStream.FlushFinalBlock();

 hmacSha = hmacShaTransform.Hash;
 hmacShaTransform.Dispose();
 

 public void Decrypt(Stream inputStream, Stream outputStream, byte expectedHmacSha)
 
 if (inputStream == null) throw new ArgumentNullException(nameof(inputStream));
 if (outputStream == null) throw new ArgumentNullException(nameof(outputStream));
 if (expectedHmacSha == null) throw new ArgumentNullException(nameof(expectedHmacSha));

 using (var hmacShaTransform = this.hmacShaTransformFactory.Invoke())
 using (var hmacShaStream = new CryptoStream(Stream.Null, hmacShaTransform, CryptoStreamMode.Write))
 
 inputStream.CopyTo(hmacShaStream);
 hmacShaStream.FlushFinalBlock();

 var hmacSha = hmacShaTransform.Hash;
 if (!hmacSha.SequenceEqual(expectedHmacSha))
 
 throw "message authentication code does not match the expected value".Security();
 
 

 inputStream.Position = 0;

 var aesCryptoTransform = this.aesProvider.CreateDecryptor();
 var aesStream = new CryptoStream(outputStream, aesCryptoTransform, CryptoStreamMode.Write);
 inputStream.CopyTo(aesStream);
 aesStream.FlushFinalBlock();
 

 public void Dispose()
 
 if (!this.disposed)
 
 this.aesProvider.Dispose();
 this.disposed = true;
 
 

My main concern with your design is that it encourages IV reuse - which, as far as I understand, pretty much defeats the point of having an IV in the first place. With IV reuse, the same plaintext always results in the same ciphertext - and plaintexts with the same prefix produce ciphertexts where the first block(s) are the same. That is leaking information that could be useful for an attacker.

I would either move the IV parameter from the constructor to Encrypt, or let Encrypt generate an IV (in a cryptographically secure way) and prepend it to the ciphertext (Decrypt should then discard the first decrypted block).

Some more notes:

• Why is the MAC 'returned', instead of appended to the output stream?


• Do you have a specific reason for combining AES-CBC with a hash-based MAC, instead of using an authenticated mode such as CCM or GCM? Cryptography is not my area of expertise, so I can't tell you which of these is 'best', but I'm just pointing out their existence in case you didn't know about them.


• There are several things that are not being disposed, mostly in Encrypt but also in Decrypt. Disposal is also inconsistent, with a few using statements and a few 'loose' Dispose calls (without any try-finally constructs).


• Related to the previous point, CryptoStream.Dispose takes ownership of the stream that's given to it, and prior to .NET 4.7.2 there's no way to override that behavior, so that may explain why you're not disposing those. If that's the case, I'd recommend documenting that - otherwise you won't be able to tell whether you simply forgot or you had a specific reason for not disposing those streams.


• Personally I wouldn't make Security an extension method, as I don't consider strings to be 'inherently convertible' to exceptions.


• It's good to see documentation, but why not use XML comments (<summary>...</summary>)? That allows your IDE to show relevant information whenever you're using this code, without you (or someone else) having to look at the implementation. You may also want to document which key sizes are valid and which padding mode is used, as well as which exception types a caller may expect.