0

I've spent the last few days creating a file encryption / decryption class based on the Rijndael encryption standard available through the RijndaelManaged class and have scoured all the resources and examples I could find. The examples were either outdated, broken or limited but have at least managed to learn a lot and thought that I'd post an up to date version of it after ensuring it was robust and past your critique.

The only gotcha I've found so far is that the salt needs to be known as there's no way of storing it within the encrypted file like you would do for a string unless you convert the per byte read/write to a buffer based read/write but then you'd need to cater for that on decryption and would also need at least 4 bytes of data to encrypt (though I don't really see this as an issue but does need to be mentioned).

I'm also not entirely sure whether 1 salt would suffice for both the key and initialisation vector or if two is better for security reasons?

Any other observations and / or optimisations would also be much appreciated

class FileEncDec
{
    private int keySize;
    private string passPhrase;

    internal FileEncDec( int keySize = 256, string passPhrase = @"This is pass phrase key to use for testing" )
    {
        this.keySize = keySize;
        this.passPhrase = passPhrase; // Can be user selected and must be kept secret
    }

    private static byte[] GenerateSalt( int length )
    {
        byte[] salt = new byte[ length ];

        // Populate salt with cryptographically strong bytes.
        RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider();

        rng.GetNonZeroBytes( salt );

        // Split salt length (always one byte) into four two-bit pieces and store these pieces in the first four bytes 
        // of the salt array.
        salt[ 0 ] = (byte)( ( salt[ 0 ] & 0xfc ) | ( length & 0x03 ) );
        salt[ 1 ] = (byte)( ( salt[ 1 ] & 0xf3 ) | ( length & 0x0c ) );
        salt[ 2 ] = (byte)( ( salt[ 2 ] & 0xcf ) | ( length & 0x30 ) );
        salt[ 3 ] = (byte)( ( salt[ 3 ] & 0x3f ) | ( length & 0xc0 ) );

        return salt;
    }

    internal bool EncryptFile( string inputFile, string outputFile )
    {
        try
        {
            byte[] salt = GenerateSalt( 16 ); // Salt needs to be known for decryption (can be safely stored in the file)
            Rfc2898DeriveBytes derivedBytes = new Rfc2898DeriveBytes( passPhrase, salt, 10000 );
            int bytesRead, bufferSize = keySize / 8;
            byte[] data = new byte[ bufferSize ];
            RijndaelManaged cryptor = new RijndaelManaged();
            cryptor.Key = derivedBytes.GetBytes( keySize / 8 );
            cryptor.IV = derivedBytes.GetBytes( cryptor.BlockSize / 8 );

            using ( var fsIn = new FileStream( inputFile, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, FileOptions.SequentialScan ) )
            {
                using ( var fsOut = new FileStream( outputFile, FileMode.Create, FileAccess.Write, FileShare.None, 4096, FileOptions.SequentialScan ) )
                {
                    // Add the salt to the file
                    fsOut.Write( salt, 0, salt.Length );

                    using ( CryptoStream cs = new CryptoStream( fsOut, cryptor.CreateEncryptor(), CryptoStreamMode.Write ) )
                    {
                        while ( ( bytesRead = fsIn.Read( data, 0, bufferSize ) ) > 0 )
                        {
                            cs.Write( data, 0, bytesRead );
                        }
                    }
                }
            }

            return true;
        }
        catch ( Exception )
        {
            return false;
        }
    }

    internal bool DecryptFile( string inputFile, string outputFile )
    {
        try
        {
            int bytesRead = 0, bufferSize = keySize / 8, saltLen;
            byte[] data = new byte[ bufferSize ], salt;
            Rfc2898DeriveBytes derivedBytes;
            RijndaelManaged cryptor = new RijndaelManaged();    // Create new cryptor so it's thread safe and don't need to use locks

            using ( var fsIn = new FileStream( inputFile, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, FileOptions.SequentialScan ) )
            {
                // Retrieve the salt length from the file
                fsIn.Read( data, 0, 4 );

                saltLen =   ( data[ 0 ] & 0x03 ) |
                            ( data[ 1 ] & 0x0c ) |
                            ( data[ 2 ] & 0x30 ) |
                            ( data[ 3 ] & 0xc0 );

                salt = new byte[ saltLen ];
                Array.Copy( data, salt, 4 );

                // Retrieve the remaining salt from the file and create the cryptor
                fsIn.Read( salt, 4, saltLen - 4 );
                derivedBytes = new Rfc2898DeriveBytes( passPhrase, salt, 10000 );
                cryptor.Key = derivedBytes.GetBytes( keySize / 8 );
                cryptor.IV = derivedBytes.GetBytes( cryptor.BlockSize / 8 );

                using ( var fsOut = new FileStream( outputFile, FileMode.Create, FileAccess.Write, FileShare.None, 4096, FileOptions.SequentialScan ) )
                {
                    using ( var cs = new CryptoStream( fsIn, cryptor.CreateDecryptor(), CryptoStreamMode.Read ) )
                    {
                        while ( ( bytesRead = cs.Read( data, 0, bufferSize ) ) > 0 )
                        {
                            fsOut.Write( data, 0, bytesRead );
                        }
                    }
                }
            }

            return true;
        }
        catch ( Exception )
        {
            return false;
        }
    }
}

Edit: 1. Added salt generator. 2. Refactored to single salt and Rfc2898DerivedBytes and now deduce IV from password + salt. 3. Made encryption / decryption thread safe (if I didn't do it correctly please let me know).

Edit 2: 1. Refactored so that reading / writing makes use of buffers instead of single byte read / write. 2. Embedded salt within the encrypted file and cleaned up variables (but still allows passPhrase default for "copy/paste" example. 3. Refactored file handles.

Storm
  • 1,848
  • 4
  • 20
  • 39
  • 2
    Code reviews should be posted on http://codereview.stackexchange.com. SO requires you to have a specific question. – Maarten Bodewes Jun 11 '15 at 12:34
  • @MaartenBodewes I did have a specific question, I had numerous questions in fact which as usr answered them I implemented his answers / suggestions (and ensured I referenced the changes in the edits) so that anyone else looking for the same thing would be able to get a proper example. Thanks for the link though, will remember in future – Storm Jun 11 '15 at 12:50

1 Answers1

1

You probably should use a different IV each time. If you use the same IV with the same data the result is the same. Attackers can now deduce that files are (partially) the same which is a leak. You can generate 16 strongly random bytes and use them as the salt for Rfc2898DeriveBytes. Prepend those bytes to the file. Use only one Rfc2898DeriveBytes to generate both IV and key. Alternatively, you can use no salt at all for the key and randomly generate the IV. The salt can be used to make the key derivation unique to your use case, or for example to give each user of your app a different key derivation algorithm.

Note, that processing streams byte-wise is extremely slow. Use buffers. Probably, you should use Stream.Copy.

usr
  • 168,620
  • 35
  • 240
  • 369
  • Thanks for those valuable insights. I'll update the source to reflect the single salt and `Rfc2898DeriveBytes`. With regards to the salt, it's my understanding that it helps when either the key / iv inputs are too short so I think that should always be there? – Storm Jun 11 '15 at 10:05
  • 1
    It only helps to make the output unique. If the password is one character there are still only 256 possible passwords no matter what salt you use. – usr Jun 11 '15 at 10:11
  • So is it completely safe to store the `salt` within the encrypted file? You can deduce the IV from `Rfc2898DeriveBytes` so long as you know the `password` and can retrieve the `salt` from the header of the encrypted file – Storm Jun 11 '15 at 10:31
  • 1
    Yes, only the key is secret and must not be derivable. The salt is optional. A warning: Attackers can change your data without you being able to find out. Use authenticated encryption to be sure that the contents were not changed. – usr Jun 11 '15 at 10:43
  • I've implemented your suggestions with exception to the authenticated encryption as there are a few ways of doing it and therefore beyond the scope of the OP. Appreciate your valuable insight! – Storm Jun 11 '15 at 12:58
  • "Alternatively, you can use no salt at all for the key and randomly generate the IV." Incorrect, that would always lead to the same key. This makes it easier to perform a dictionary attack if there are multiple passwords. – Maarten Bodewes Jun 11 '15 at 13:33
  • @MaartenBodewes that is true but that doesn't make the answer incorrect. If this this is OK for the OP then so be it. Update: How would you run that dictionary attack? I can't think of any way. I don't think it's possible. – usr Jun 11 '15 at 13:44
  • Let's just say that the salt is not an optional component for PBKDF2 for a good reason. – Maarten Bodewes Jun 11 '15 at 13:50
  • @MaartenBodewes I see no reason it would not be optional and so far you have not given one. – usr Jun 11 '15 at 14:09
  • With crypto you should be reasonably sure yourself. But OK, since you insist: if you have multiple passwords without salt then an attacker can perform a dictionary attack over all of the encrypted files. Although an attacker will still have to calculate the key using PBKDF2, it now only takes one AES decrypt over all of the ciphertext to test all of the passwords. That's a speed up of x where x is the amount of passwords. – Maarten Bodewes Jun 11 '15 at 16:28
  • Hm. It seems that he would need one full-file decryption per possible password that he is trying. Each password results in a different key which must be tested. Also, the random IV makes each file unique. I don't follow what you are proposing. – usr Jun 11 '15 at 17:06
  • You don't need to test the whole file to see if the key is correct. – Maarten Bodewes Jun 11 '15 at 23:42