1
\$\begingroup\$

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

    }

}
Improve this question
\$\endgroup\$
1
  • 3
    \$\begingroup\$ Don't call GC.Collect(). Don't call .Close() for resources already wrapped with using. Don't call .Dispose() directly; rely on using. \$\endgroup\$
    – Jesse C. Slicer
    May 23 '17 at 18:41
6
\$\begingroup\$

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

Improve this answer
\$\endgroup\$
6
  • \$\begingroup\$ Thanks for you feedback, could you explain why password = RandomByteArray(password.length); wouldnt work? \$\endgroup\$
    – HodorKing
    May 23 '17 at 19:34
  • \$\begingroup\$ And when I enter a password by wpf or forms, will the string be around for a while in memory? Does Something like gch = GCHandle.Alloc(string, GCHandleType.Pinned); and after use ZeroMemory(gch.AddrOfPinnedObject(), string.Length * 2) help? \$\endgroup\$
    – HodorKing
    May 23 '17 at 19:41
  • \$\begingroup\$ Also in addition I now added extra information in front of decrypted data, the salt, a byte pair from key.GetBytes(2) for verification of password without decrypting it with the password in question (if the both passwords produces not same 2 bytes it definitely isnt the right, but when they do I do a deeper verification by decrypting the data with the password and comparing the checksums, this is my third information in front of the data, a checksum of HMACSHA512 based on a secret key generated by key.GetBytes(32) and the inital data before encyption \$\endgroup\$
    – HodorKing
    May 23 '17 at 22:21
  • \$\begingroup\$ @HodorKing - password = RandomByteArray(password.Length) is not recommeneded because your method does not replace, but does a "new"byte[] result = new byte[length];. That's why I suggested you pass reference of the array instead of the length (ex: void RandomizeByteArray(byte[] password)). \$\endgroup\$
    – Svek
    May 24 '17 at 3:45
  • \$\begingroup\$ @HodorKing Your comments about dealing with the password string in WPF is beyond the scope of your code review. I would suggest opening up a separate question for stack + heap security, strings can be tricky, especially if needs to be passed around through a UI. \$\endgroup\$
    – Svek
    May 24 '17 at 3:47
0
\$\begingroup\$

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.

Improve this answer
\$\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.