Try/Catch/Finally with Encryption Resources

Question

I have the following code on my website. It takes a URL query as a string named commandencrypted. I have a try/catch/finally block that will copy the encrypted command to a memorystream, then decrypt it back to a string. If any errors occur in the try block, they are handled in the catch block. However, I want to make sure any resources that are used are closed in the finally block. When an invalid URL query command is provided, I receive an exception while trying to close the csencryptedcommand CryptoStream. The exception details follow the code.

    // Resources used for storing and decrypting the encrypted client command
    MemoryStream msencryptedcommand = null;
    RijndaelManaged rmencryptedcommand = null;
    CryptoStream csencryptedcommand = null;
    StreamReader srdecryptedcommand = null;
    MemoryStream msdecryptedcommand = null;

    try
    {
        // Copy the encrypted client command (where two characters represent a byte) to a memorystream
        msencryptedcommand = new MemoryStream();
        for (int i = 0; i < encryptedcommand.Length; )
        {
            msencryptedcommand.WriteByte(Byte.Parse(encryptedcommand.Substring(i, 2), NumberStyles.HexNumber));
            i = i + 2;
        }
        msencryptedcommand.Flush();
        msencryptedcommand.Position = 0;

        // Define parameters used for decryption
        byte[] key = new byte[] { //bytes hidden// };
        byte[] iv = new byte[] { //bytes hidden// };
        rmencryptedcommand = new RijndaelManaged();
        csencryptedcommand = new CryptoStream(msencryptedcommand, rmencryptedcommand.CreateDecryptor(key, iv), CryptoStreamMode.Read);
        msdecryptedcommand = new MemoryStream();

        // Decrypt the client command
        int decrytptedbyte;
        while ((decrytptedbyte = csencryptedcommand.ReadByte()) != -1)
        {
            msdecryptedcommand.WriteByte((byte)decrytptedbyte);
        }

        // Store the decrypted client command as a string
        srdecryptedcommand = new StreamReader(msdecryptedcommand);
        srdecryptedcommand.BaseStream.Position = 0;
        string decryptedcommand = srdecryptedcommand.ReadToEnd();
    }
    catch (Exception ex)
    {
        ErrorResponse("Invalid URL Query", context, ex.ToString());
        return;
    }
    finally
    {
        // If any resources were used, close or clear them
        if (srdecryptedcommand != null)
        {
            srdecryptedcommand.Close();
        }

        if (msdecryptedcommand != null)
        {
            msdecryptedcommand.Close();
        }

        if (csencryptedcommand != null)
        {
            csencryptedcommand.Close();
        }

        if (rmencryptedcommand != null)
        {
            rmencryptedcommand.Clear();
        }

        if (msencryptedcommand != null)
        {
            msencryptedcommand.Close();
        }
    }

The following unhandled exception occurred: System.Security.Cryptography.CryptographicException: Length of the data to decrypt is invalid. at System.Security.Cryptography.RijndaelManagedTransform.TransformFinalBlock(Byte[] inputBuffer, Int32 inputOffset, Int32 inputCount) at System.Security.Cryptography.CryptoStream.FlushFinalBlock() at System.Security.Cryptography.CryptoStream.Dispose(Boolean disposing) at System.IO.Stream.Close() at dongleupdate.ProcessRequest(HttpContext context) in update.ashx:line 92 at System.Web.HttpApplication.CallHandlerExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() at System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) URL Referrer: User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.69 Safari/537.36 Request URL: update.ashx?aa

EDIT:

I changed the code to use using statements. Is this also a valid way to ensure that all resources are closed?

    try
    {
        // Copy the encrypted client command (where two characters represent a byte) to a memorystream
        using (MemoryStream msencryptedcommand = new MemoryStream())
        {
            for (int i = 0; i < encryptedcommand.Length; )
            {
                msencryptedcommand.WriteByte(Byte.Parse(encryptedcommand.Substring(i, 2), NumberStyles.HexNumber));
                i = i + 2;
            }
            msencryptedcommand.Flush();
            msencryptedcommand.Position = 0;

            // Define parameters used for decryption
            byte[] key = new byte[] { //bytes hidden// };
            byte[] iv = new byte[] { //bytes hidden// };
            using (RijndaelManaged rmencryptedcommand = new RijndaelManaged())
            {
                using (CryptoStream csencryptedcommand = new CryptoStream(msencryptedcommand, rmencryptedcommand.CreateDecryptor(key, iv), CryptoStreamMode.Read))
                {
                    using (MemoryStream msdecryptedcommand = new MemoryStream())
                    {

                        // Decrypt the client command
                        int decrytptedbyte;
                        while ((decrytptedbyte = csencryptedcommand.ReadByte()) != -1)
                        {
                            msdecryptedcommand.WriteByte((byte)decrytptedbyte);
                        }

                        // Store the decrypted client command as a string
                        using (StreamReader srdecryptedcommand = new StreamReader(msdecryptedcommand))
                        {
                            srdecryptedcommand.BaseStream.Position = 0;
                            string decryptedcommand = srdecryptedcommand.ReadToEnd();
                        }
                    }
                }
            }
        }
    }
    catch (Exception ex)
    {
        ErrorResponse("Invalid URL Query", context, ex.ToString());
        return;
    }

I find coding styles like that largely unreadable.. either way you should be doing input validation on any inputs that you know could be invalid before even submitting them to the method above. I'd also consider 'using' statements to ensure proper disposal and release of resources. — Dave Lawrence, Oct 16 '13 at 16:19
Blam: As noted above, I receive an exception while trying to close the csencryptedcommand CryptoStream. csencryptedcommand.Close(); — bbs_chad, Oct 16 '13 at 16:22
daveL: How do you perform input validation on encrypted data before attempting to decrypt it? — bbs_chad, Oct 16 '13 at 16:24
For your updated code in the spot where you have three using statements right after each other you don't need the `{` after the first two using statements, you can chain them all and just use a single `{` at the end. — Scott Chamberlain, Oct 16 '13 at 17:50
See [Nested using statements in C#](http://stackoverflow.com/q/1329739/18192) for discussion of Scott's point. A minority of developers really hate stacked using statements, but personally I think it's much cleaner; it sharply reduces indentation. — Brian, Oct 16 '13 at 18:19

Karl Anderson · Accepted Answer · 2013-10-16T17:52:27.747

Wrap your CryptoStream in a using block and then it will be automatically closed and disposed of for you via the Dispose Pattern, like this:

using(csencryptedcommand = new CryptoStream(msencryptedcommand, 
    rmencryptedcommand.CreateDecryptor(key, iv), CryptoStreamMode.Read))
{
    // Do things with crypto stream here
}

Read using Statement (C# Reference) documentation for more information.

UPDATE:

Using Reflector, here is the code for CryptoStream's Dispose method:

protected override void Dispose(bool disposing)
{
    try
    {
        if (disposing)
        {
            if (!this._finalBlockTransformed)
            {
                this.FlushFinalBlock();
            }
            this._stream.Close();
        }
    }
    finally
    {
        try
        {
            this._finalBlockTransformed = true;
            if (this._InputBuffer != null)
            {
                Array.Clear(this._InputBuffer, 0, this._InputBuffer.Length);
            }
            if (this._OutputBuffer != null)
            {
                Array.Clear(this._OutputBuffer, 0, this._OutputBuffer.Length);
            }
            this._InputBuffer = null;
            this._OutputBuffer = null;
            this._canRead = false;
            this._canWrite = false;
        }
        finally
        {
            base.Dispose(disposing);
        }
    }
}

Note: As you can see there is an explicit call to close the stream via this line:

this._stream.Close();

Karl: Thanks. That worked. Would you recommend creating using blocks for the other resources (srdecryptedcommand, msdecryptedcommand, rmencryptedcommand, msencryptedcommand) and removing the finally block? — bbs_chad, Oct 16 '13 at 16:41
I changed the code. Is this also a valid way to do it where we are ensured that the resources are closed? See the new code in the above question. — bbs_chad, Oct 16 '13 at 17:06
@bbs_chad - yes, I recommend implementing a `using` block around any object that implements the `IDisposable` interface, which all of the types you have in your code do, either directly or via their inheritance chain. — Karl Anderson, Oct 16 '13 at 17:47
@bbs_chad - see `UPDATE` in answer for whether or not the `using` block ensures the resources are closed. — Karl Anderson, Oct 16 '13 at 17:52

score 0 · Answer 2 · answered Oct 16 '13 at 16:20

0

I think all of the objects you're using implement IDisposable so if I were you I'd wrap them all in using statements so they'll automatically be cleaned up for you, i.e.

using(msencryptedcommand = new MemoryStream())
{
    ....
}

answered Oct 16 '13 at 16:20

DoctorMick

6,703
28
26

Try/Catch/Finally with Encryption Resources

2 Answers2