4
\$\begingroup\$

Out of personal interest and as a learning exercise I've written a C# class (.NET 4) to perform encryption/decryption of a file along with some compression upon encryption. Most of my understanding of this has been based on web research so I'd like some feedback on coding style, optimizations and, if any cryptography experts out there, the security of this approach.

using System;
using System.IO;
using System.IO.Compression;
using System.Security.Cryptography;
using System.Text;

namespace FileCrypt {
    class Cypto {

        private const int SALTSIZE = 16;
        private const int HASHSIZE = 20;
        private const int EXTENSIONSIZE = 32;
        private const int BITSPERBYTE = 8;
        private const int ITERCOUNT = 32767;

        /// <summary>
        /// compress and encrypt file into target with password protection
        /// </summary>
        /// <param name="inFile"></param>
        /// <param name="outFile"></param>
        /// <param name="password"></param>
        /// <returns></returns>
        public static void Encrypt( FileInfo inFile, FileInfo outFile, string password ) {

            try{
                Rfc2898DeriveBytes keyGenerator = new Rfc2898DeriveBytes(password, SALTSIZE, ITERCOUNT);
                Rijndael rijndael = Rijndael.Create();
                rijndael.Padding = PaddingMode.PKCS7;
                rijndael.Mode = CipherMode.CBC;
                rijndael.IV = keyGenerator.GetBytes(rijndael.BlockSize / BITSPERBYTE);
                rijndael.Key = keyGenerator.GetBytes(rijndael.KeySize / BITSPERBYTE);

                using(var fileStream = outFile.Create()) {
                    byte[] salt = keyGenerator.Salt;
                    byte[] fileExtension = Encoding.ASCII.GetBytes(Convert.ToBase64String(Encoding.ASCII.GetBytes(inFile.Extension)));
                    byte[] paddedExtension = new byte[EXTENSIONSIZE];

                    if(fileExtension.Length > EXTENSIONSIZE) {
                        throw new Exception("File extension is too long for allocated memory. " +
                            "Consider compressing the file to a zip archive prior to encryption.");
                    }

                    Array.Copy(fileExtension, paddedExtension, fileExtension.Length);

                    // write random salt, hash and file extension to output file
                    fileStream.Write(salt, 0, SALTSIZE);
                    fileStream.Write(Hash(salt, password), 0, HASHSIZE);
                    fileStream.Write(paddedExtension, 0, EXTENSIONSIZE);

                    using(CryptoStream cryptoStream = new CryptoStream(fileStream, rijndael.CreateEncryptor(), CryptoStreamMode.Write)) {
                        using(GZipStream gzStream = new GZipStream(cryptoStream, CompressionMode.Compress)) {
                            using(FileStream sourceStream = inFile.OpenRead()) {
                                sourceStream.CopyTo(gzStream);
                            }
                        }
                    }
                }
            } catch {
                throw;
            }
        }

        /// <summary>
        /// decompress and decrypt file with password validation
        /// </summary>
        /// <param name="sourceFile"></param>
        /// <param name="outDir"></param>
        /// <param name="password"></param>
        /// <returns></returns>
        public static void Decrypt( FileInfo sourceFile, DirectoryInfo outDir, string password ) {

            byte[] salt = new byte[SALTSIZE];
            byte[] hash = new byte[HASHSIZE];
            byte[] fileextension = new byte[EXTENSIONSIZE];

            try {
                using(FileStream sourceStream = sourceFile.OpenRead()) {

                    // read salt, hash and file extension from source file
                    sourceStream.Read(salt, 0, SALTSIZE);
                    sourceStream.Read(hash, 0, HASHSIZE);
                    sourceStream.Read(fileextension, 0, EXTENSIONSIZE);

                    // check hashed password
                    if(!ByteCompare(Hash(salt, password), hash)) {
                        throw new Exception("Incorrect password entered.");
                    }

                    // build output file path and create FileInfo object for it
                    fileextension = Convert.FromBase64String(Encoding.ASCII.GetString(fileextension).TrimEnd('\0'));
                    string filepath = Path.Combine(outDir.FullName,
                                                    Path.GetFileNameWithoutExtension(sourceFile.FullName) +
                                                    Encoding.ASCII.GetString(fileextension).Trim());
                    FileInfo outFile = new FileInfo(filepath);

                    // generate derived key
                    Rfc2898DeriveBytes keyGenerator = new Rfc2898DeriveBytes(password, salt, ITERCOUNT);
                    Rijndael rijndael = Rijndael.Create();
                    rijndael.Padding = PaddingMode.PKCS7;
                    rijndael.Mode = CipherMode.CBC;
                    rijndael.IV = keyGenerator.GetBytes(rijndael.BlockSize / BITSPERBYTE);
                    rijndael.Key = keyGenerator.GetBytes(rijndael.KeySize / BITSPERBYTE);

                    using(FileStream outStream = new FileStream(outFile.FullName, FileMode.OpenOrCreate, FileAccess.Write)) {
                        using(CryptoStream cryptoStream = new CryptoStream(sourceStream, rijndael.CreateDecryptor(), CryptoStreamMode.Read)) {
                            using(GZipStream gzipStream = new GZipStream(cryptoStream, CompressionMode.Decompress)) {
                                gzipStream.CopyTo(outStream);
                            }
                        }
                    }
                }
            } catch {
                throw;
            }
        }

        /// <summary>
        /// compute hash from salt and password
        /// </summary>
        /// <param name="salt"></param>
        /// <param name="password"></param>
        /// <returns></returns>
        private static byte[] Hash( byte[] salt, string password ) {

            byte[] pass = Encoding.ASCII.GetBytes(password);
            byte[] saltedpass = new byte[salt.Length + pass.Length];
            Array.Copy(salt, saltedpass, salt.Length);
            Array.Copy(pass, 0, saltedpass, salt.Length, pass.Length);

            return new SHA1CryptoServiceProvider().ComputeHash(saltedpass);
        }

        /// <summary>
        /// compare byte arrays for equality
        /// </summary>
        /// <param name="b1"></param>
        /// <param name="b2"></param>
        /// <returns></returns>
        public static bool ByteCompare( byte[] b1, byte[] b2 ) {
            return Encoding.ASCII.GetString(b1) == Encoding.ASCII.GetString(b2);
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ You shouldn't use the Rijndael class, use Aes. With a 128-bit block size they are the same, but saying "Aes" makes your interoperability clearer. \$\endgroup\$
    – bartonjs
    Commented Apr 3, 2017 at 3:49

1 Answer 1

5
\$\begingroup\$

Overall it looks fine. You use constants and meainingful names and you encapsulate features properly but here and there, there are some small flaws. Let's take a look...


Rfc2898DeriveBytes keyGenerator = new Rfc2898DeriveBytes(password, SALTSIZE, ITERCOUNT);

This needs to be disposed.


Rijndael rijndael = Rijndael.Create();

This needs to be disposed too. Actually you can test everthing for the Dispose method when working with encryptions and streams. There is much more then that which is currently not disposed.


throw new Exception("File extension is too long for allocated memory. " +
                    "Consider compressing the file to a zip archive prior to encryption.");

For this you should create a new exception like

class FileExtensionLengthOutOfRangeException : Exception
{
    public override string Message => "File extension is too long for allocated memory. Consider compressing the file to a zip archive prior to encryption."
}

You can then put the message there and just throw it without writing the message every time. You should avoid throwing the Exception.

throw new Exception("Incorrect password entered.");

The same here. InvalidPasswordException would be much more appropriate and alone by the name of the exception you'd know what's wrong. What more you'll be able to catch it with catch(InvalidPasswordException ex) if you need to.


using (FileStream outStream = new FileStream(outFile.FullName, FileMode.OpenOrCreate, FileAccess.Write))
{
  using (CryptoStream cryptoStream = new CryptoStream(sourceStream, rijndael.CreateDecryptor(), CryptoStreamMode.Read))
  {
      using (GZipStream gzipStream = new GZipStream(cryptoStream, CompressionMode.Decompress))
      {
          gzipStream.CopyTo(outStream);
      }
  }
}

You can throw away the {} of adjacent usings:

using (FileStream outStream = new FileStream(outFile.FullName, FileMode.OpenOrCreate, FileAccess.Write))
using (CryptoStream cryptoStream = new CryptoStream(sourceStream, rijndael.CreateDecryptor(), CryptoStreamMode.Read))
using (GZipStream gzipStream = new GZipStream(cryptoStream, CompressionMode.Decompress))
{
    gzipStream.CopyTo(outStream);
}

catch
{
  throw;
}

This is useless. You can remove the entire try/catch block and nothing would change.

\$\endgroup\$
5
  • \$\begingroup\$ Great, thanks for the useful feedback. It all makes sense and helps the code to read better. One question I do have; why do I need to dispose of the Rfc2898DeriveBytes and Rijndael objects explicitly? Are they not disposed once they go out of scope? \$\endgroup\$
    – T_Bacon
    Commented Mar 24, 2017 at 8:57
  • 1
    \$\begingroup\$ @T_Bacon nope, they won't be autodisposed. Let me ask you another quesiton: why do you dispose the FileStream or the CryptoStream or other things explicitly and the other ones not? ;-) \$\endgroup\$
    – t3chb0t
    Commented Mar 24, 2017 at 8:59
  • \$\begingroup\$ Ok, did a bit of reading on this. So, it sounds like anything that implements IDisposable should be disposed; never knew that. One of those things that as a self-taught programmer can slip through the net! Thanks for pointing it out. :-) \$\endgroup\$
    – T_Bacon
    Commented Mar 24, 2017 at 9:20
  • 1
    \$\begingroup\$ @T_Bacon when objects get out of scope they are still alive and are going to be garbage collected at some undefinite point in future. The GC however does not track unmanaged resources - that are usually used by disposable objects - and that's how memory leaks occur. The object might be never disposed and the memory is never freed. You might find the article about Fundamentals of Garbage Collection interesting. \$\endgroup\$
    – t3chb0t
    Commented Mar 24, 2017 at 10:06
  • \$\begingroup\$ Thanks for the link, interesting stuff. Can't say I understood everything but I certainly understand the process more than I did before. I had always assumed that as a managed language the garbage collection would deal with all this so had ignored the Dispose() method and had actually questioned it's purpose given my assumption. This clears things up quite a bit and I won't be ignoring it any more! \$\endgroup\$
    – T_Bacon
    Commented Mar 24, 2017 at 11:01

Not the answer you're looking for? Browse other questions tagged or ask your own question.