Encryption/decryption of data

Question

I'm trying to work with some encryption/decryption of data. It was some hard work for me to get it working with some buffers and what not.

This is the code I came up with:

public static string Encrypt(string dataToEncrypt, byte[] publicKeyInfo)
{
    //// Our bytearray to hold all of our data after the encryption
    byte[] encryptedBytes = new byte[0];
    using (var rsa = new RSACryptoServiceProvider())
    {
        try
        {
            var encoder = new UTF8Encoding();

            byte[] encryptThis = encoder.GetBytes(dataToEncrypt);

            //// Importing the public key
            rsa.ImportCspBlob(publicKeyInfo);

            int blockSize = (rsa.KeySize / 8) - 32;

            //// buffer to write byte sequence of the given block_size
            byte[] buffer = new byte[blockSize];

            byte[] encryptedBuffer = new byte[blockSize];

            //// Initializing our encryptedBytes array to a suitable size, depending on the size of data to be encrypted
            encryptedBytes = new byte[encryptThis.Length + blockSize - (encryptThis.Length % blockSize) + 32];

            for (int i = 0; i < encryptThis.Length; i += blockSize)
            {
                //// If there is extra info to be parsed, but not enough to fill out a complete bytearray, fit array for last bit of data
                if (2 * i > encryptThis.Length && ((encryptThis.Length - i) % blockSize != 0))
                {
                    buffer = new byte[encryptThis.Length - i];
                    blockSize = encryptThis.Length - i;
                }

                //// If the amount of bytes we need to decrypt isn't enough to fill out a block, only decrypt part of it
                if (encryptThis.Length < blockSize)
                {
                    buffer = new byte[encryptThis.Length];
                    blockSize = encryptThis.Length;
                }

                //// encrypt the specified size of data, then add to final array.
                Buffer.BlockCopy(encryptThis, i, buffer, 0, blockSize);
                encryptedBuffer = rsa.Encrypt(buffer, false);
                encryptedBuffer.CopyTo(encryptedBytes, i);
            }
        }
        catch (CryptographicException e)
        {
            Console.Write(e);
        }
        finally
        {
            //// Clear the RSA key container, deleting generated keys.
            rsa.PersistKeyInCsp = false;
        }
    }
    //// Convert the byteArray using Base64 and returns as an encrypted string
    return Convert.ToBase64String(encryptedBytes);
}




public static string Decrypt(string dataToDecrypt, byte[] privateKeyInfo)
{
    //// The bytearray to hold all of our data after decryption
    byte[] decryptedBytes;

    //Create a new instance of RSACryptoServiceProvider.
    using (var rsa = new RSACryptoServiceProvider())
    {
        try
        {
            byte[] bytesToDecrypt = Convert.FromBase64String(dataToDecrypt);

            //// Import the private key info
            rsa.ImportCspBlob(privateKeyInfo);

            //// No need to subtract padding size when decrypting
            int blockSize = rsa.KeySize / 8;

            //// buffer to write byte sequence of the given block_size
            byte[] buffer = new byte[blockSize];

            //// buffer containing decrypted information
            byte[] decryptedBuffer = new byte[blockSize];

            //// Initializes our array to make sure it can hold at least the amount needed to decrypt.
            decryptedBytes = new byte[dataToDecrypt.Length];

            for (int i = 0; i < bytesToDecrypt.Length; i += blockSize)
            {
                if (2 * i > bytesToDecrypt.Length && ((bytesToDecrypt.Length - i) % blockSize != 0))
                {
                    buffer = new byte[bytesToDecrypt.Length - i];
                    blockSize = bytesToDecrypt.Length - i;
                }

                //// If the amount of bytes we need to decrypt isn't enough to fill out a block, only decrypt part of it
                if (bytesToDecrypt.Length < blockSize)
                {
                    buffer = new byte[bytesToDecrypt.Length];
                    blockSize = bytesToDecrypt.Length;
                }

                Buffer.BlockCopy(bytesToDecrypt, i, buffer, 0, blockSize);
                decryptedBuffer = rsa.Decrypt(buffer, false);
                decryptedBuffer.CopyTo(decryptedBytes, i);
            }
        }
        finally
        {
            //// Clear the RSA key container, deleting generated keys.
            rsa.PersistKeyInCsp = false;
        }
    }

    //// We encode each byte with UTF8 and then write to a string while trimming off the extra empty data created by the overhead.
    var encoder = new UTF8Encoding();
    return encoder.GetString(decryptedBytes).TrimEnd(new[] { '\0' });

}

I tried to see if I could split it up, but in my effort to do so, I came up with what I believed would be a more complex method (higher cyclomatic complexity). I had a lot of if statements all of the sudden, having to take care of the encryption mode but also handling the encoding from UTF8 or BASE64.

Answer 1

I don't have any comments on the cryptography side of it, but I have a few general programming points:

• It is not necessary to have four forward slash characters for inline comments, you only need two.
• There is no need to initialize your encryptedBytes array as an empty array since you re-initialize it later.
• You should use System.Text.Encoding.UTF8 instead of instantiating UTF8Encoding manually.
• It might be worth splitting the inline calculations inside your conditional and array building for ease of reading (lines 24, when initializing the encryptedBytes array, and line 29, when checking for 'extra info to be parsed').
• While I encourage the use of well places comments to explain the algorithm or program steps (particularly when it's a complicated algorithm), I think you might have gone a little overkill in some places. As such, it might be an idea to remove the more obvious comments, such as "Our bytearray to hold all of our data after the encryption" - the name of the variable indicates this already. It's not a major point, but it will help with the length of your method.
• With regards to your exception handling, I would suggest ending code execution when you reach your catch clause as you wouldn't really want the program to continue with this algorithm if it has failed. Also, I would suggest breaking down the code inside the try clause a little more so you only have try-catch blocks surrounding the exact lines that may cause a CryptographicException. It's not of major importance, but it helps another reader to easily identify the line(s) that could the issue.

Other than the above points, the code itself looks to be fairly well structured and logical.

Answer 2

You're using RSA as a block cipher in ECB mode. That's not how it's usually used. Unless you have a very good reason to do otherwise, you should just do one RSA encryption, of a secure random key for a block cipher (i.e. AES), and then use that key to encrypt your message.

Since you're converting string to string, it's apparent that neither memory nor latency is an issue, so you could avoid a lot of messing around with byte[]s by using MemoryStream and CryptoStream. You can also use Encoding.UTF8 to simplify the en/decoding step slightly.