2
\$\begingroup\$

I was looking for a proper implementation of the AES256 encryption in C# but didn't find anything I was entirely satisfied with, so I decided to take this opportunity to write a class myself and maybe learn a thing or two.

After doing some research I managed to come up with this class :

class AES256
{

    private readonly byte[] _secretKey;

    public AES256(byte[] secretKey)
    {
        _secretKey = secretKey;
    }

    public byte[] Encrypt(byte[] plainBytes)
    {
        return Encrypt(plainBytes, GetRijndaelManaged(_secretKey));
    }

    public byte[] Decrypt(byte[] encryptedBytes)
    {
        return Decrypt(encryptedBytes, GetRijndaelManaged(_secretKey));
    }

    private byte[] Encrypt(byte[] plainBytes, RijndaelManaged rijndaelManaged)
    {
        byte[] transformFinalBlock = rijndaelManaged.CreateEncryptor().TransformFinalBlock(plainBytes, 0, plainBytes.Length);
        return transformFinalBlock;
    }

    private byte[] Decrypt(byte[] encryptedData, RijndaelManaged rijndaelManaged)
    {
        return rijndaelManaged.CreateDecryptor().TransformFinalBlock(encryptedData, 0, encryptedData.Length);
    }

    private RijndaelManaged GetRijndaelManaged(byte[] key)
    {
        var keyBytes = new byte[16];
        var secretKeyBytes = key;
        Array.Copy(secretKeyBytes, keyBytes, System.Math.Min(keyBytes.Length, secretKeyBytes.Length));
        var rijndaelManaged = new RijndaelManaged
        {
            Mode = CipherMode.CBC,
            Padding = PaddingMode.PKCS7,
            KeySize = 256,
            BlockSize = 128,
            Key = keyBytes,
            IV = keyBytes
        };
        return rijndaelManaged;
    }

}

It works, - but obviously that alone doesn't make the code good. What can I improve in this class, is there anything I can optimize? Feedback is welcome !

\$\endgroup\$
3
\$\begingroup\$
    private RijndaelManaged GetRijndaelManaged(byte[] key)
    {
        var keyBytes = new byte[16];
        var secretKeyBytes = key;
        Array.Copy(secretKeyBytes, keyBytes, System.Math.Min(keyBytes.Length, secretKeyBytes.Length));

Why is it even possible for keyBytes.Length to differ from key.Length?

        var rijndaelManaged = new RijndaelManaged
        {
            Mode = CipherMode.CBC,

Are you forced to use CBC for backwards compatibility with something, or is it a free choice? If the latter, it would be worth documenting the reason for choosing it over e.g. GCM.

            Padding = PaddingMode.PKCS7,
            KeySize = 256,
            BlockSize = 128,
            Key = keyBytes,

keyBytes was initialised as new byte[16]. 16 * 8 = 128, so there's no point initialising KeySize to 256. (And the class name isn't actually correct).

            IV = keyBytes

I think you need to read up on the point of IV. Each message should have a different IV, and it should be independent of the key. Normally that means that the IV needs to be transmitted along with the ciphertext.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for your feedback ! Really appreciate it. Regarding the GCM ciphermode, - there seems to be no option in the CipherMode enumeration for GCM? \$\endgroup\$
    – yq8dev
    Sep 16 '17 at 23:14
  • \$\begingroup\$ Also I noticed that both, - ICryptoTransform and Rijndael are disposable. So i can wrap them into a using statement. \$\endgroup\$
    – yq8dev
    Sep 16 '17 at 23:49
  • 1
    \$\begingroup\$ Ah, yes. GCM has to be self-implemented, which brings risks. As for disposing, the situation is similar to MemoryStream: it's disposable because most implementations need disposing, but it's an exception, so although it may be best practice to dispose it's not a problem not to. \$\endgroup\$ Sep 17 '17 at 5:23
4
\$\begingroup\$

Peter already pointed out several important issues which I won't repeat, but here's what I've got:

  • Instead of verifying that the length of the key is correct, your code silently pads or trims the key. I would much rather see an exception being thrown instead. Especially security-related code should be easy to use correctly and difficult to use incorrectly.
  • Because the encryption key can only be specified when constructing an AES256 instance, your design 'encourages' key reuse. On its own that may not necessarily be a problem, but you're not using IVs correctly, and together that does introduce certain vulnerabilities.
  • RijndaelManaged and ICryptoTransform are both disposable, but they're not being disposed anywhere.
  • The private Encrypt/Decrypt methods are so short that merging them into the public methods would actually make the code easier to manage.
  • You may want to document which cipher and padding modes your code uses (and why), so other programmers (and future you) don't need to look at the implementation to find out (or wonder why certain decisions were made).
  • MSDN recommends using the Aes class instead of RijndaelManaged. It also appears that the Rijndael implementation isn't 'FIPS compliant', which apparently makes it unusable under certain security configurations.
  • As far as I understand there's no need to create a new RijndaelManaged instance for each operation, so why not create one instance and store it in a field? Or, instead of creating a very thin wrapper class, you could just create a factory method and some extension methods for encryption/decryption. By having these extension methods operate on SymmetricAlgorithm they can also be reused for other encryption algorithms, should the need ever arise.
\$\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.