trjhtr

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;
 

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.

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.

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.