5
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Hi, thank you for your suggestions, The reason I have four slashes is because of stylecop complaining about it otherwise, but thanks. Also, I did not want to initialize the encryptedBytes array, but if I don't it won't compile since the return it not sure if the array has been initialized. Also, sorry if this is a stupid question, but how do you mean, split my inline calculations? \$\endgroup\$ – SimonLanghoff Dec 10 '11 at 10:48
  • \$\begingroup\$ I think the compilation issue would be due to the fact an Exception can be thrown before initializing encryptedBytes and you continue execution after it occurs. Instead of allowing that, it might be worth either re-throwing the Exception in the catch clause of your try-catch-finally statement, or returning a null or empty string. That way, you should be able to omit the unnecessary initialization of encryptedBytes and better control the method execution. I have edited my answer to reflect this point. \$\endgroup\$ – Samuel Slade Dec 10 '11 at 10:55
  • \$\begingroup\$ I see, yes, thank you, I moved the lines outside the try/catch and that removed the error message. You say I should stop code execution in the catch clause, how would you say is the best way to do this? \$\endgroup\$ – SimonLanghoff Dec 10 '11 at 11:10
  • 1
    \$\begingroup\$ It depends on how your going to use the algorithm. You could let the CryptograpicException bubble up out of the method (but keeping the try-finally statement). Or, you could wrap the exception in your own exception that has a more explicit meaning (though this is probably better in larger systems and not here). Alternately, if you are wanting to prevent exceptions from being thrown from this method, I'd be tempted to return some result from the method. For example, return true or false to denote success and get the encrypted string as an out parameter. \$\endgroup\$ – Samuel Slade Dec 10 '11 at 11:47
  • \$\begingroup\$ Okay thanks a lot, as you can see, I'm not that experienced with coding and I'm really not at all that familiar with the out parameter, but hanks a lot. I'll look into it, thank you for your suggestions \$\endgroup\$ – SimonLanghoff Dec 10 '11 at 14:04
4
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ I was in fact not a favour of this approach, as everytime I read about doing it this way, I was told that RSA is usually used to encrypt a symmetric key. I'm doing this for a project in school and we pretty much specified that we wanted to use public/Private key encryption only. I might look into doing it with the RijndaelManaged class later on if I get the time, I'm just afraid it will be too much to change at the moment and I Don't want to break the build :) But I actually did not realize that I'm basically doing the insecure ECB mode which I now feel really bad about. \$\endgroup\$ – SimonLanghoff Dec 10 '11 at 10:56

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.