AES Encryption/Decryption of Files and Bytes

Question

I have gathered some information about aes256 in c# from the internet and put together this code and I would like to know if this is secure or if its lacking in security.

Working

encrypt/decrypt bytes, encrypt/decrypt files

Problems

File encryption/decryption uses 1 byte buffer because otherwise I get larger encrypted files and even larger decryption files with for example many NULNUL values at the end of a .txt file if .txt file is smaller than buffer

Code

public static class Encryptions {

    public const int AES256KeySize = 256;

    public static byte[] RandomByteArray(int length) {

        byte[] result = new byte[length];

        using(RNGCryptoServiceProvider provider = new RNGCryptoServiceProvider()) {

            provider.GetBytes(result);

            return result;

        }

    }

    public static bool AESEncryptFile(string filePath, byte[] password, bool delete) {

        byte[] salt = RandomByteArray(16);

        using(FileStream fs = new FileStream(filePath + ".enc", FileMode.Create)) {

            var key = GenerateKey(password, salt);

            password = null;
            GC.Collect();

            using(Aes aes = new AesManaged()) {

                aes.KeySize = AES256KeySize;
                aes.Key = key.GetBytes(aes.KeySize / 8);
                aes.IV = key.GetBytes(aes.BlockSize / 8);
                aes.Padding = PaddingMode.ISO10126;
                aes.Mode = CipherMode.CBC;

                fs.Write(salt, 0, salt.Length);

                using(CryptoStream cs = new CryptoStream(fs, aes.CreateEncryptor(), CryptoStreamMode.Write)) {

                    using(FileStream fsIn = new FileStream(filePath, FileMode.Open)) {

                        byte[] buffer = new byte[1];
                        int read;

                        key.Dispose();

                        try {

                            while((read = fsIn.Read(buffer, 0, buffer.Length)) > 0) {

                                cs.Write(buffer, 0, read);

                            }

                            if(delete) {

                                File.Delete(filePath);

                            }

                            cs.Close();
                            fs.Close();
                            fsIn.Close();

                            return true;

                        } catch (Exception e) {

                            return false;

                        }

                    }

                }

            }

        }

    }

    public static bool AESDecryptFile(string filePath, byte[] password, bool keep) {

        byte[] salt = new byte[16];

        using(FileStream fsIn = new FileStream(filePath, FileMode.Open)) {

            fsIn.Read(salt, 0, salt.Length);

            var key = GenerateKey(password, salt);

            password = null;
            GC.Collect();

            using(Aes aes = new AesManaged()) {

                aes.KeySize = AES256KeySize;
                aes.Key = key.GetBytes(aes.KeySize / 8);
                aes.IV = key.GetBytes(aes.BlockSize / 8);
                aes.Padding = PaddingMode.ISO10126;
                aes.Mode = CipherMode.CBC;

                using(CryptoStream cs = new CryptoStream(fsIn, aes.CreateDecryptor(), CryptoStreamMode.Read)) {

                    using(FileStream fsOut = new FileStream(filePath.Remove(filePath.Length - 4), FileMode.Create)) {

                        byte[] buffer = new byte[1];
                        int read;

                        key.Dispose();

                        try {

                            while((read = cs.Read(buffer, 0, buffer.Length)) > 0) {

                                fsOut.Write(buffer, 0, buffer.Length);

                            }

                            cs.FlushFinalBlock();

                            fsOut.Close();
                            fsIn.Close();
                            cs.Close();

                            return true;

                        } catch(Exception e) {

                            return false;

                        }

                    }

                }

            }

        }

    }

    public static byte[] AESEncryptBytes(byte[] clear, byte[] password, byte[] salt) {

        byte[] encrypted = null;

        var key = GenerateKey(password, salt);

        password = null;
        GC.Collect();

        using(Aes aes = new AesManaged()) {

            aes.KeySize = AES256KeySize;
            aes.Key = key.GetBytes(aes.KeySize / 8);
            aes.IV = key.GetBytes(aes.BlockSize / 8);
            aes.Padding = PaddingMode.PKCS7;
            aes.Mode = CipherMode.CBC;

            using(MemoryStream ms = new MemoryStream()) {

                using(CryptoStream cs = new CryptoStream(ms, aes.CreateEncryptor(), CryptoStreamMode.Write)) {

                    cs.Write(clear, 0, clear.Length);
                    cs.Close();

                }

                encrypted = ms.ToArray();

            }

            key.Dispose();

        }

        return encrypted;

    }

    public static byte[] AESDecryptBytes(byte[] encrypted, byte[] password, byte[] salt) {

        byte[] decrypted = null;

        var key = GenerateKey(password, salt);

        password = null;
        GC.Collect();

        using(Aes aes = new AesManaged()) {

            aes.KeySize = AES256KeySize;
            aes.Key = key.GetBytes(aes.KeySize / 8);
            aes.IV = key.GetBytes(aes.BlockSize / 8);
            aes.Padding = PaddingMode.PKCS7;
            aes.Mode = CipherMode.CBC;

            using(MemoryStream ms = new MemoryStream()) {

                using(CryptoStream cs = new CryptoStream(ms, aes.CreateDecryptor(), CryptoStreamMode.Write)) {

                    cs.Write(encrypted, 0, encrypted.Length);
                    cs.Close();

                }

                decrypted = ms.ToArray();

            }

            key.Dispose();

        }

        return decrypted;

    }

    public static bool CheckPassword(byte[] password, byte[] salt, byte[] key) {

        using(Rfc2898DeriveBytes r = GenerateKey(password, salt)) {

            byte[] newKey = r.GetBytes(AES256KeySize / 8);
            return newKey.SequenceEqual(key);

        }

    }

    public static Rfc2898DeriveBytes GenerateKey(byte[] password, byte[] salt) {

        return new Rfc2898DeriveBytes(password, salt, 52768);

    }

}

Answer 1

Accessibility

With security in mind, I don't think you want this field accessible as public:

public const int AES256KeySize = 256;

Instead, you may want to change the access modifier to private as I can't see the importance of exposing this int outside of Encryptions class. Like so:

private const int AES256KeySize = 256; 

---

Keeping the Heap Clean + No Garbage Collection

To be very secure, you need to clean up your sensitive info as soon as you no longer need it, for example such as the byte[] password parameter. As you don't want it floating around in the heap.

I have noticed that you are already doing this, but I would recommend either clearing it or randomizing it instead of causing a heavy-handed call to GC.Collect().

public static void ExampleMethodThatUsesPassword(byte[] password)
{

    ...

    password = null;
    GC.Collect(); // probably not something you want to do... a lot of overhead!

    //********************************************************************************
    // instead of the above, fill byte[] with new bytes (blank/random), then resize.
    //********************************************************************************

    // overwrite the heap with blanks
    Array.Clear(password , 0, password.Length); // still exposes the byte[] length!

    // if you want to be fancy, load up some random bytes into the byte[] instead...
    // note that your current implementation of RandomByteArray() method won't overwrite.

    Array.Resize(ref password, 1); // resize the byte[]
    password = null; 
    // no longer need to force a GC.Collect()

   ...

}

Cleaning up your salt

You do not clean up the salt byte[] like you do with password. I can imagine this is getting a bit over-kill, but for the sake of completion:

byte[] salt = RandomByteArray(16) // **16 should be a const**
// clean up your salt like the password above

---

Readability Suggestion + Avoiding Dispose()

One more point is not much about security, but just readability and good practice.

var key = GenerateKey(password, salt);
using (Aes aes = new AesManaged())
{
    ...
    key.Dispose();
}

Could be simplified to

using (var key = GenerateKey(password, salt))
using (Aes aes = new AesManaged())
{
    ...
}

---

Exception Handling

And finally, you should probably deal with possible exceptions being thrown here:

using (CryptoStream cs = new CryptoStream(ms, aes.CreateDecryptor(), CryptoStreamMode.Write))
{
    cs.Write(encrypted, 0, encrypted.Length);
    cs.Close();
}

The exceptions can be found here: CryptoStream.Write Method (Byte[], Int32, Int32).

Answer 2

Cleaning the salt in the public static Rfc2898DeriveBytes GenerateKey(byte[] password, byte[] salt) method won't work. Because you need it in fs.Write(salt, 0, salt.Length);. Otherwise, data won't be decrypted properly because it needs the same salt value while decrypting. So, cleaning has to be done after you wrote the salt to file.