7
\$\begingroup\$

I would like to have my C# cryptography code reviewed.

I programmed an application that encrypts files with a password (provided by the user). It basically functions as a password manager, on the most basic level. After having learned the basics and the principles of cryptography I am now planning to implement it barebones in C++.

Be as critical as you want/can be; I'm willing to learn and accept mistakes.

public class Crypto
{
    private const int _saltSizeBytes = 32;
    private const int _IVSizeBytes = 16;
    private const int _PBKDF2Iterations = 10000;

    /// <summary>
    /// Encrypts a file's content and creates a new one to store the cipher text in
    /// </summary>
    /// <param name="fileContent">Array of bytes containing the file content</param>
    /// <param name="password">Password to derive the encryption key from</param>
    /// <param name="cipherMode">Ciphermode to use for encryption. It is CBC by default.</param>
    public void EncryptFile(byte[] fileContent, string password, string fileNameAndExtension, CipherMode cipherMode = CipherMode.CBC)
    {
        Console.WriteLine("Encrypting " + fileNameAndExtension);
        using (AesManaged aesManaged = new AesManaged())
        {

            //Set ciphermode for the AES algoritm (CBC, cipher block chaining, by default)
            aesManaged.Mode = cipherMode;

            //Generate initialization vector, IV is 16 bytes (128 bits) long 
            aesManaged.GenerateIV();

            //Generate a random salt
            byte[] salt = GenerateRandomSalt();

            //Generate a 256 bits key using the password and the salt
            aesManaged.Key = GenerateKey(password, salt);

            //Open filestream
            using (FileStream outputFileStream = new FileStream("../../Files/" + fileNameAndExtension, FileMode.Create))
            {
                //Append salt to filestream
                outputFileStream.Write(salt, 0, salt.Length);

                //Append initialization vector to filestream
                outputFileStream.Write(aesManaged.IV, 0, aesManaged.IV.Length);

                //Link the filestream to a Cryptostream(which handles cryptographic transformations, such as AES).
                using (CryptoStream cryptoStream = new CryptoStream(outputFileStream, aesManaged.CreateEncryptor(), CryptoStreamMode.Write))
                {
                    //Write the salt, initialization vector and encrypted content to a file.
                    cryptoStream.Write(fileContent, 0, fileContent.Length);
                }
            }
        }
    }

    /// <summary>
    /// Decrypts an encrypted file and creates a new on to store the original content in
    /// </summary>
    /// <param name="cipherText">Array of bytes containing the cipher text</param>
    /// <param name="password">Password to derive the encryption key from</param>
    public void DecryptFile(byte[] cipherText, string password, string fileNameAndExtension)
    {
        using (AesManaged aesManaged = new AesManaged())
        {

            byte[] salt = GetSaltFromCiphertext(cipherText);
            byte[] initializationVector = GetInitializationVectorFromCiphertext(cipherText);
            byte[] fileContentToDecrypt = GetContentFromCiphertext(cipherText);
            byte[] decryptedBytes = null;
            //Initialize the AES instance with the key and the initialization vector
            aesManaged.Key = GenerateKey(password, salt);
            aesManaged.IV = initializationVector;
            //Create MemoryStream to load file into memory before writing
            //This way the exception for a wrong password gets thrown before writing occurs

            using (MemoryStream memoryStream = new MemoryStream(fileContentToDecrypt))
            {
                using (CryptoStream cryptoStream = new CryptoStream(memoryStream, aesManaged.CreateDecryptor(), CryptoStreamMode.Write))
                {

                    //Write the cryptostream to the memorystream
                    cryptoStream.Write(fileContentToDecrypt, 0, fileContentToDecrypt.Length);
                    decryptedBytes = memoryStream.ToArray();
                }

                using (FileStream fileStream = new FileStream("../../Files/" + fileNameAndExtension, FileMode.Create))
                {
                    Console.WriteLine("Decrypting " + fileNameAndExtension);
                    fileStream.Write(decryptedBytes, 0, decryptedBytes.Length);
                }
            }
        }
    }

    /// <summary>
    /// Generates a random salt using the RNGCryptoServiceProvider
    /// </summary>
    /// <returns>An array of non-zero bytes</returns>
    private byte[] GenerateRandomSalt()
    {
        //Initialize byte array to store salt, the salt is 32 bytes (256 bits) long
        byte[] salt = new byte[32];

        //Generate salt using (pseudo)random bytes
        //Use using statement since RNGCryptoServiceProvider implements IDisposable.
        using (var random = new RNGCryptoServiceProvider())
        {
            random.GetNonZeroBytes(salt);
        }

        return salt;
    }

    /// <summary>
    /// Generates a 256 bits symmetric encryption key using the PBKDF2 algorithm
    /// </summary>
    /// <param name="password">Password used to lock and unlock te file</param>
    /// <param name="salt">Random salt to prevent rainbow table hash cracking</param>
    /// <returns>An array of bytes representing the 256 bits key</returns>
    private byte[] GenerateKey(string password, byte[] salt)
    {
        //Use password derivation function PBKDF2 with 10.000 iterations (1000 is default)
        //And a salt.
        Rfc2898DeriveBytes rfc = new Rfc2898DeriveBytes(password, salt, _PBKDF2Iterations);

        //Get 32 bytes (256 bits) from the derived key. A 256 bits key is required for AES.
        byte[] key = rfc.GetBytes(32);

        return key;
    }

    /// <summary>
    /// Retrieves the salt from the encrypted file
    /// </summary>
    /// <param name="encryptedContent">An array of bytes containing the cipher text</param>
    /// <returns>An array of bytes(32) containing a salt</returns>
    private byte[] GetSaltFromCiphertext(byte[] ciphertext)
    {

        byte[] salt = new byte[_saltSizeBytes];
        //Get the salt from the encrypted file content
        for (int i = 0; i < _saltSizeBytes; i++)
        {
            salt[i] = ciphertext[i];
        }

        return salt;
    }

    /// <summary>
    /// Retrieves the initialization vector from the encrypted file
    /// </summary>
    /// <param name="encryptedContent">An array of bytes containing the cipher text</param>
    /// <returns>An array of bytes(16) containing an initialization vector</returns>
    private byte[] GetInitializationVectorFromCiphertext(byte[] ciphertext)
    {
        byte[] initializationVector = new byte[_IVSizeBytes];
        //Get the initialization vector from the encrypted file content
        for (int i = 0; i < _IVSizeBytes; i++)
        {
            initializationVector[i] = ciphertext[i + _saltSizeBytes];
        }

        return initializationVector;
    }

    /// <summary>
    /// Gets the cipher text from an encrypted file
    /// </summary>
    /// <param name="encryptedContent">An array of bytes containing the cipher text</param>
    /// <returns>An array of bytes containing the encrypted content</returns>
    private byte[] GetContentFromCiphertext(byte[] ciphertext)
    {
        byte[] fileContentToDecrypt = new byte[ciphertext.Length - _saltSizeBytes - _IVSizeBytes];

        //Get the readl content to decrypt
        for (int i = 0; i < fileContentToDecrypt.Length; i++)
        {
            fileContentToDecrypt[i] = ciphertext[i + _saltSizeBytes + _IVSizeBytes];
        }

        return fileContentToDecrypt;
    }
}
\$\endgroup\$
4
\$\begingroup\$

Style

private const int _saltSizeBytes = 32;
private const int _IVSizeBytes = 16;
private const int _PBKDF2Iterations = 10000;

Initially, I wanted to comment that consts should be written in all caps, e.g. SALT_SIZE_BYTES. However, this answer disagrees based on Microsoft's StyleCop rules.

The recommended naming and capitalization convention is to use Pascal casing for constants (Microsoft has a tool named StyleCop that documents all the preferred conventions and can check your source for compliance - though it is a little bit too anally retentive for many people's tastes). e.g.

private const int TheAnswer = 42;

I'm still used to the all caps, but pascal casing seems to be a better option based on what I find online.

The style you're currently using is used for private fields; which is not what you want to use here.


Comments

The method summaries are great to have. Even if a bit obvious at times, it's still good to have them in regards to Intellisense tooltips.

You put a lot of effort in writing comments, but you went a bit overboard with it. Obvious comments that rephrase a line of code should be avoided unless they meaningfully add an explanation. Some examples below:

//Set ciphermode for the AES algoritm (CBC, cipher block chaining, by default)
aesManaged.Mode = cipherMode;

There's no need to list the options. That's what the enum is for. Now you have two places to updates if the enum changes: the enum and this comment. That's more work than you want.
Furthermore, Set ciphermode for the AES algoritm is obvious when you look at the code: aesManaged.Mode = cipherMode.

The comment doesn't add much explanation, so it can be removed.

//Generate a random salt
byte[] salt = GenerateRandomSalt();

The comment is already explained by the method name.

//Open filestream
using (FileStream outputFileStream = new FileStream("../../Files/" + fileNameAndExtension, FileMode.Create))

The comment doesn't add something that wasn't already clear.

//Append salt to filestream
outputFileStream.Write(salt, 0, salt.Length);

The comment is just a rephrase of outputFileStream.Write(salt.

//Initialize byte array to store salt, the salt is 32 bytes (256 bits) long
byte[] salt = new byte[32];

The comment lists what the code tells us. From the code, I can see that you're making a byte array that represents the salt and is 32 bytes in size.

As a counterexample, let's say that you commonly refer to the salt as "a 256 bit salt" in the entire application. Adding a comment that reflects that can be meaningful, as the relation between 32 and 256 is not immediately apparent:

    byte[] salt = new byte[32]; //256 bits

This comment adds something that was not already obvious.


I'm not all that experience in cryptography so I can't comment on the intention of the code.

\$\endgroup\$
4
\$\begingroup\$

var

AesManaged aesManaged = new AesManaged()

You can make you code less verbose by using var instead of explicit type everywhere. Consider this:

var aesManaged = new AesManaged()

EncryptFile & DecryptFile

I find those methods confusing. Their names suggest to encrypt/decrypt files but all they do is to save data as files, they don't read them. Extracting a SaveAs method for any stream would be much cleaner.


Single Responsibility Principle (SRP)

From the single-resposibility point of view both EncryptFile and DecryptFile are doing to much and will lead in duplicated code later if you decide to use other output streams like MemoryStream or compression streams.

This means you should extract the encrpytion and decryption parts into separate methods that work with any Stream for example:

public void Encrypt(Stream stream, byte[] data, string password, CipherMode cipherMode = CipherMode.CBC)
{
    using (AesManaged aesManaged = new AesManaged())
    {
        //Set ciphermode for the AES algoritm (CBC, cipher block chaining, by default)
        aesManaged.Mode = cipherMode;

        //Generate initialization vector, IV is 16 bytes (128 bits) long 
        aesManaged.GenerateIV();

        //Generate a random salt
        byte[] salt = GenerateRandomSalt();

        //Generate a 256 bits key using the password and the salt
        aesManaged.Key = GenerateKey(password, salt);

        //Append salt to filestream
        stream.Write(salt, 0, salt.Length);

        //Append initialization vector to filestream
        stream.Write(aesManaged.IV, 0, aesManaged.IV.Length);

        //Link the filestream to a Cryptostream(which handles cryptographic transformations, such as AES).
        using (CryptoStream cryptoStream = new CryptoStream(stream, aesManaged.CreateEncryptor(), CryptoStreamMode.Write))
        {
            //Write the salt, initialization vector and encrypted content to a file.
            cryptoStream.Write(data, 0, data.Length);
        }
    }
}

public void SaveAs(Stream stream, string fileName)
{
    using (FileStream outputFileStream = new FileStream(fileName, FileMode.Create))
    {
        stream.CopyTo(outputFileStream);
    }
}

Now you have two methods. A general one Encrypt and a specialized one SaveAsEncrypted that reuses it. If you now decide to add compression to it, it'll be much easier to add another stream to that chain another stream.

You would use them like this:

using(var memoryStream = new MemoryStream())
{
    var crypto = new Crypto();
    crypto.Encrypt(memoryStream, data, password);
    crypto.SaveAs(memoryStream, fileName);
}

which you can again encapsulate in another helper if you want to.


Having multiple methods that have only one responsibility has the benefit that you can test then independantly and one feature at a time.

\$\endgroup\$
  • 1
    \$\begingroup\$ unfortunately the solution you proposed will not work as CryptoStream will close any underlying stream (MemoryStream in this case) when it closes itself. That means that when you call SaveAs() it will throw an exception as you tried to access a closed stream. \$\endgroup\$ – Mick May 23 '18 at 14:14
  • \$\begingroup\$ @Mick oops, I now see my mistake. I'll need to fix that later. \$\endgroup\$ – t3chb0t May 23 '18 at 14:20
4
\$\begingroup\$

In addition to the other reviews, here are my points:

Crypto notes:

  1. The salt size is 32 bytes, which is fine if you can spare that amount of bytes. However, a size of 16 bytes is more than plenty for PBKDF2. The salt is only required to avoid collisions, and I presume you're not going to reuse the password more than 2^64 times.
  2. If you generate a random key then you don't need a random IV, not even for CBC mode.
  3. Not all ciphers are created equal; you might as well standardize on one, e.g. AES-GCM which adds authentication (AES-CTR is unfortunately not available by default). However, that uses an IV of 12 bytes and adds a tag.
  4. CBC is vulnerable to plaintext / padding oracle attacks, so don't use this for transport mode security.
  5. GetContentFromCiphertext is incorrect, it should be called GetCiphertextFromContext.
  6. You should define your protocol and add a protocol version, so you can change your protocol later on. For instance, you may want to re-encrypt using a different hash or iteration count in the future. This can be represented using the protocol version.
  7. You may want to validate that the derived key is correct before decryption. Note that CBC doesn't add integrity protection. This also means that the decryption may not fail even if you supply the wrong key (resulting in garbage plaintext).
  8. You extract 256 bits from PBKDF2 which defaults to SHA-1 with an output size of 160 bits. That means that the entire function is repeated for the last 256 - 160 = 96 bits. This will double the amount of work on your server. Better specify SHA-256 or SHA-512 as hash.
  9. Why not use new Aes() instead of new AesManaged()? Then you can use hardware acceleration in native code when available. Also see this question. Note that I haven't tried the speed of either of the mentioned classes, but you may want to give it a try for large files.

Other remarks:

  1. The order of your calls is a bit weird, you can create salt and key outside the using code block.
  2. "../../Files/" is a magic string - it is probably better.
  3. Your encrypt and decrypt functions aren't symmetric, even though they seem to be. They should be byte array to file, then file to byte array.
\$\endgroup\$
  • \$\begingroup\$ Extremely late to the party, I know... \$\endgroup\$ – Maarten Bodewes May 28 '18 at 23:44
  • \$\begingroup\$ Thank you for the feedback! All feedback is welcome, at any time. I can totally see whe're you're coming from with your points and i am going to take them to improve my code. I just got a few questions related to the points: 1) If CBC is vulnerable, which ciphermode would you suggest? I thought CBC was the safest currently available. 2) What makes the random IV obsolete when i use a random key? 3) What would you recommend for checking derived key validity? I would like to prevent creating a database \$\endgroup\$ – Mick May 30 '18 at 9:10
  • \$\begingroup\$ @Mick 1) CTR, or better use authenticated encryption such as GCM. Run away from anything with padding, it's a common source of subtle but sometimes devastating attacks. 2) Using repeated or related IVs with the same key can leak information about the ciphertexts. When a key is only used once, there aren't two ciphertexts to relate. But I'd still play it safe and use a random IV unless you're absolutely sure you won't ever want to reuse the key (say, when you do a format upgrade?) and you sorely need to save a few bytes of storage. 3) Start the ciphertext with a known string (magic header). \$\endgroup\$ – Gilles Jul 1 '18 at 22:33

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.