10
\$\begingroup\$

This code is a Frankenstein. I found ten examples done in ten different ways and my requirements were slightly different so even if they were reliable they weren't a copy and paste solution.

Encrypt takes in a byte[] and password and returns the byte[] encrypted with the password. It has to tack on the length of the original byte[] and the initialization vector used by the encryption algorithm.

Decrypt knows how to read the encrypted byte[] and will return the original message.

Note: One thing about naming conventions - Since a lot of this is copy and paste from different sources I tended to keep the original authors' conventions so that the code would match up. That's why some variables start with caps and some don't. Not looking to have this post turn into a tangent about that low hanging fruit though. But since it's obvious I thought I should point it out.

Encrypt

// https://social.msdn.microsoft.com/Forums/vstudio/en-US/eab7d698-2340-4ba0-a91c-da6fae06963c/aes-encryption-encrypting-byte-array?forum=csharpgeneral
// https://crypto.stackexchange.com/questions/2280/why-is-the-iv-passed-in-the-clear-when-it-can-be-easily-encrypted
public static byte[] Encrypt(byte[] bytesToEncrypt, string password)
{
    byte[] ivSeed = Guid.NewGuid().ToByteArray();

    var rfc = new Rfc2898DeriveBytes(password, ivSeed);
    byte[] Key = rfc.GetBytes(16);
    byte[] IV = rfc.GetBytes(16);

    byte[] encrypted;
    using (MemoryStream mstream = new MemoryStream())
    {
        using (AesCryptoServiceProvider aesProvider = new AesCryptoServiceProvider())
        {
            using (CryptoStream cryptoStream = new CryptoStream(mstream, aesProvider.CreateEncryptor(Key, IV), CryptoStreamMode.Write))
            {
                cryptoStream.Write(bytesToEncrypt, 0, bytesToEncrypt.Length);
            }
        }
        encrypted = mstream.ToArray();
    }

    var messageLengthAs32Bits = Convert.ToInt32(bytesToEncrypt.Length);
    var messageLength = BitConverter.GetBytes(messageLengthAs32Bits);

    encrypted = encrypted.Prepend(ivSeed);
    encrypted = encrypted.Prepend(messageLength);

    return encrypted;
}

Decrypt

public static byte[] Decrypt(byte[] bytesToDecrypt, string password)
{
    (byte[] messageLengthAs32Bits, byte[] bytesWithIv) = bytesToDecrypt.Shift(4); // get the message length
    (byte[] ivSeed, byte[] encrypted) = bytesWithIv.Shift(16);                    // get the initialization vector

    var length = BitConverter.ToInt32(messageLengthAs32Bits, 0);

    var rfc = new Rfc2898DeriveBytes(password, ivSeed);
    byte[] Key = rfc.GetBytes(16);
    byte[] IV = rfc.GetBytes(16);

    byte[] decrypted;
    using (MemoryStream mStream = new MemoryStream(encrypted))
    {
        using (AesCryptoServiceProvider aesProvider = new AesCryptoServiceProvider())
        {
            aesProvider.Padding = PaddingMode.None;
            using (CryptoStream cryptoStream = new CryptoStream(mStream,aesProvider.CreateDecryptor(Key, IV), CryptoStreamMode.Read))
            {
                cryptoStream.Read(encrypted, 0, length);
            }
        }
        decrypted = mStream.ToArray().Take(length).ToArray();
    }
    return decrypted;
}

Relevant Extension Methods (i'm more worried about security than these)

    public static byte[] Prepend(this byte[] bytes, byte[] bytesToPrepend)
    {
        var tmp = new byte[bytes.Length + bytesToPrepend.Length];
        bytesToPrepend.CopyTo(tmp, 0);
        bytes.CopyTo(tmp, bytesToPrepend.Length);
        return tmp;
    }

    public static (byte[] left, byte[] right) Shift(this byte[] bytes, int size)
    {
        var left = new byte[size];
        var right = new byte[bytes.Length - size];

        Array.Copy(bytes, 0, left, 0, left.Length);
        Array.Copy(bytes, left.Length, right, 0, right.Length);

        return (left, right);
    }
\$\endgroup\$
  • 6
    \$\begingroup\$ Guids are designed to be unique, but not necessary random. If you need a random salt, you should use random number generators the were designed for cryptography (such as RNGCryptoServiceProvider). Take a look at example on msdn: msdn.microsoft.com/en-us/library/zhe81fz4(v=vs.110).aspx \$\endgroup\$ – Nikita B Jun 8 '18 at 6:50
  • 1
    \$\begingroup\$ Rfc2898DeriveBytes is an IDisposable-implementing class, so you should wrap its lifetime in a using statement as you have the others. \$\endgroup\$ – Jesse C. Slicer Jun 8 '18 at 14:37
  • \$\begingroup\$ Come by to 2nd Monitor and we can discuss this issue there. \$\endgroup\$ – t3chb0t Nov 29 '18 at 13:52
5
\$\begingroup\$

in this portion of your encryption method you can actually stack the using statements so that you don't have to indent them so much

using (MemoryStream mstream = new MemoryStream())
{
    using (AesCryptoServiceProvider aesProvider = new AesCryptoServiceProvider())
    {
        using (CryptoStream cryptoStream = new CryptoStream(mstream, aesProvider.CreateEncryptor(Key, IV), CryptoStreamMode.Write))
        {
            cryptoStream.Write(bytesToEncrypt, 0, bytesToEncrypt.Length);
        }
    }
    encrypted = mstream.ToArray();
}

And you can actually put that assignment to the encrypted variable inside the inner most using statement, it won't harm anything and will allow you to make this stack of usings the least indented possible, which lends itself to easier reading.

Now it looks like this:

using (MemoryStream mstream = new MemoryStream())
using (AesCryptoServiceProvider aesProvider = new AesCryptoServiceProvider())
using (CryptoStream cryptoStream = new CryptoStream(mstream, aesProvider.CreateEncryptor(Key, IV), CryptoStreamMode.Write))
{  
    cryptoStream.Write(bytesToEncrypt, 0, bytesToEncrypt.Length);
    encrypted = mstream.ToArray();
}

and your Decrypt statement can be made a little cleaner as well because we can return directly from the using blocks.

a feature of the using block is that no matter what happens when the scope leaves the using block it will automatically call the dispose method of those objects before scope moves onto something else, kind of like a Try/Finally.

you can also get rid of the Decrypted Variable because we are going to return inside of the using statement.

I also moved the Padding Change into a Property initializer

so instead of this:

public static byte[] Decrypt(byte[] bytesToDecrypt, string password)
{
    (byte[] messageLengthAs32Bits, byte[] bytesWithIv) = bytesToDecrypt.Shift(4); // get the message length
    (byte[] ivSeed, byte[] encrypted) = bytesWithIv.Shift(16);                    // get the initialization vector

    var length = BitConverter.ToInt32(messageLengthAs32Bits, 0);

    var rfc = new Rfc2898DeriveBytes(password, ivSeed);
    byte[] Key = rfc.GetBytes(16);
    byte[] IV = rfc.GetBytes(16);

    byte[] decrypted;
    using (MemoryStream mStream = new MemoryStream(encrypted))
    {
        using (AesCryptoServiceProvider aesProvider = new AesCryptoServiceProvider())
        {
            aesProvider.Padding = PaddingMode.None;
            using (CryptoStream cryptoStream = new CryptoStream(mStream,aesProvider.CreateDecryptor(Key, IV), CryptoStreamMode.Read))
            {
                cryptoStream.Read(encrypted, 0, length);
            }
        }
        decrypted = mStream.ToArray().Take(length).ToArray();
    }
    return decrypted;
}

you get this:

public static byte[] Decrypt(byte[] bytesToDecrypt, string password)
{
    (byte[] messageLengthAs32Bits, byte[] bytesWithIv) = bytesToDecrypt.Shift(4); // get the message length
    (byte[] ivSeed, byte[] encrypted) = bytesWithIv.Shift(16);                    // get the initialization vector

    var length = BitConverter.ToInt32(messageLengthAs32Bits, 0);

    var rfc = new Rfc2898DeriveBytes(password, ivSeed);
    byte[] Key = rfc.GetBytes(16);
    byte[] IV = rfc.GetBytes(16);

    using (MemoryStream mStream = new MemoryStream(encrypted))
    using (AesCryptoServiceProvider aesProvider = new AesCryptoServiceProvider() { Padding = PaddingMode.None })
    using (CryptoStream cryptoStream = new CryptoStream(mStream, aesProvider.CreateDecryptor(Key, IV), CryptoStreamMode.Read))
    {
        cryptoStream.Read(encrypted, 0, length);
        return mStream.ToArray().Take(length).ToArray();
    }
}

Kudos:

I like your Comments on the Message Length and Initialization Vector

\$\endgroup\$
9
\$\begingroup\$

Standard disclaimer: in a production setting it is overwhelmingly better to use somebody else's crypto.

I don't have the expertise to certify "Yes, this is secure" (and as I'm some randomer online, you should assume that even if I was actually a world expert!) There are, however, a few things that strike me as red flags.

byte[] ivSeed = Guid.NewGuid().ToByteArray();

In crypto, if you need a random number, you basically always need a cryptographically secure random number. NewGuid() is not cryptographically secure.

var messageLength = BitConverter.GetBytes(messageLengthAs32Bits);

Block ciphers like AES do not claim to obscure the approximate message length, so this is not strictly a vulnerability. Nevertheless if you are vulnerable to any attacks based on the length, then passing the precise length in plaintext makes those attacks a whole bunch quicker and easier to execute.

byte[] Key = rfc.GetBytes(16);
byte[] IV = rfc.GetBytes(16);

The derivation of your key should not be tied to the derivation of your IV.

\$\endgroup\$
  • \$\begingroup\$ Be careful with your statements about cryptographic randomness. For initialisation vectors and salts, uniqueness is usually sufficient and monotonically increasing counters are often better than pseudorandom generators, especially when little to no entropy is available. \$\endgroup\$ – Rainer P. Jun 8 '18 at 16:56
  • \$\begingroup\$ I would stand by my claim as written. If you need a random number, it should be from a cryptographically secure source. Salts I agree can be deterministic. IVs depend on the protocol, e.g. I recall the beast attack on TLS exploited a predictabe (even though unique) IV. \$\endgroup\$ – Josiah Jun 8 '18 at 17:52
  • 1
    \$\begingroup\$ I think in a well-designed cryptosystem, the IV's only purpose is to produce distinct cyphertexts even when multiple messages have the same payload. Though I agree that a good random IV can sometimes save the day in a not-so-well-designed cryptosystem, a bad RNG (like the debian bug some years ago) can certainly make it worse compared to a deterministc counter. \$\endgroup\$ – Rainer P. Jun 8 '18 at 21:17
  • \$\begingroup\$ Thanks :) Get random number ✓ Don't include message length (figure it out programatically) ✓ The key and iv one I updated based on @Nikita B's comment. \$\endgroup\$ – user875234 Jun 9 '18 at 0:53
3
\$\begingroup\$

Algorithm bugged

One machine could encrypt the password and another could decrypt it. You would think that this should not be a problem.

However, BitConverter uses the endianess of the machine. This means your encryption strategy is computer architecture-sensitive, which is a huge weakness and frankly a bug.

Encryptor (using big endian):

var messageLength = BitConverter.GetBytes(messageLengthAs32Bits);

Decryptor (using little endian):

var length = BitConverter.ToInt32(messageLengthAs32Bits, 0);
\$\endgroup\$
2
\$\begingroup\$

You should add cryptoStream.Close() inside your CryptoStream() when you are finished with it. Otherwise you may end up with mismatching byte[] size when decrypting and you may get an exception on cryptoStream.Read(encrypted, 0 , length) and/or a corrupted result.

\$\endgroup\$

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.