HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Rijndael compression/encryption class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
compressionclassencryptionrijndael

Problem

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;

///
/// compress and encrypt file into target with password protection
///
///
///
///
///
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

Solution

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.

Code Snippets

Rfc2898DeriveBytes keyGenerator = new Rfc2898DeriveBytes(password, SALTSIZE, ITERCOUNT);
Rijndael rijndael = Rijndael.Create();
throw new Exception("File extension is too long for allocated memory. " +
                    "Consider compressing the file to a zip archive prior to encryption.");
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."
}
throw new Exception("Incorrect password entered.");

Context

StackExchange Code Review Q#158632, answer score: 5

Revisions (0)

No revisions yet.