locked
Protecting sensitive data in managed memory (pin, GC) RRS feed

  • Question

  • Hi everyone,

    A couple of months ago, I made a blog post about how to encrypt and decrypt sensitive data in PowerShell using AES and RSA.  I thought I had done a good job of cleaning up any sensitive data in memory in this code, but now I'm not so sure.  Here's a snippet of equivalent C# code:

    /*
    * Assume that rsa already refers to an RsaCryptoServiceProvider object, specifically the
    * PrivateKey of an X509Certificate2.
    */
    
    byte[] plainText = null;
    
    try
    {
        plainText = rsa.Decrypt(cipherText, true);
        
        // Do something with plainText.
    }
    finally
    {
        if (plainText != null) { Array.Clear(plainText, 0, plainText.Length); }
    }    

    So that looks pretty straightforward, I used try/finally to make sure the plainText array is zeroed out before letting go of the reference, ideally ensuring that sensitive data isn't left in memory any longer than necessary.  However, I wasn't taking the GC into account at the time.  If it copies the plainText array around in managed memory, does it leave copies of the decrypted data around that don't get cleaned up later?

    I later learned that you can temporarily pin objects in managed memory, which would allow me to ensure that the cleanup code is effective, but RsaCryptoServiceProvider.Decrypt() throws a bit of a wrench into this idea:  it allocates the array and returns it, rather than accepting a caller-defined buffer.  If I do this:

    byte[] plainText = null;
    GCHandle gch = null;
    
    try
    {
        plainText = rsa.Decrypt(cipherText, true);
        gch = GCHandle.Alloc(plainText, GCHandleType.Pinned);
    
        // Do something with plainText
    }
    finally
    {
        if (plainText != null) { Array.Clear(plainText, 0, plainText.Length); }
        if (gch != null) { gch.Free(); }
    }    

    My understanding is that the pinning of the plainText array may already be too late; at any time between when the Decrypt() method allocates and populates the array and when GCHandle.Alloc() executes, the GC may already have copied the array around, leaving bits of uncleared data behind.  Is this correct?  If so, what can be done to make this code secure?

    Thursday, April 10, 2014 1:48 AM

Answers

  • "While that's true, I don't think it's an excuse for having insecure code in the first place."

    The code is "insecure" to begin with because it places decrypted data in memory. If someone manages to dump the process at the right time then the contents of the plaintext array will be visible no matter what you do. This isn't about security, it's about minimizing risks. And if you look at the problem from the risks angle then pinning the array still minimizes the risks even if pinning is done after Decrypt.

    "After all, the SecureString class was added for exactly this reason."

    Sort of, strings have a different problem, unlike arrays they cannot be cleared. Anyway, SecureString is IMO a failure. I've never seen anyone actually using it much less using it correctly. It's kind of impossible after all, for example try using SecureString to store the password provided in a WinForms TextBox control to a database.


    • Edited by Mike Danes Thursday, April 10, 2014 12:08 PM
    • Marked as answer by Caillen Thursday, April 17, 2014 8:00 AM
    Thursday, April 10, 2014 12:08 PM

All replies

  • "My understanding is that the pinning of the plainText array may already be too late; ..."

    Indeed, it's too late and there's nothing you can do about that. Well, except not using the RSA class and using the native crypto library directly.

    Anyway, the primary use of zeroing out that array is to prevent someone from getting the secure information from a memory dump of a process. But if someone is able to dump the process then it means that the environment is not secure to begin with so...

    Thursday, April 10, 2014 6:02 AM
  • Anyway, the primary use of zeroing out that array is to prevent someone from getting the secure information from a memory dump of a process. But if someone is able to dump the process then it means that the environment is not secure to begin with so...

    While that's true, I don't think it's an excuse for having insecure code in the first place. After all, the SecureString class was added for exactly this reason. If it's no big deal to have sensitive data hanging around in memory, why not just use Strings everywhere?  :)


    Edit:  I decided to post this on the Connect site for .NET:  https://connect.microsoft.com/VisualStudio/feedback/details/850077/sensitive-data-returned-by-rsacryptoserviceprovider-decrypt-cannot-be-reliably-cleared-from-memory .  If it gets enough "up" votes, perhaps they'll make a change to a future version of the framework.
    • Edited by David Wyatt Thursday, April 10, 2014 11:23 AM
    Thursday, April 10, 2014 11:00 AM
  • "While that's true, I don't think it's an excuse for having insecure code in the first place."

    The code is "insecure" to begin with because it places decrypted data in memory. If someone manages to dump the process at the right time then the contents of the plaintext array will be visible no matter what you do. This isn't about security, it's about minimizing risks. And if you look at the problem from the risks angle then pinning the array still minimizes the risks even if pinning is done after Decrypt.

    "After all, the SecureString class was added for exactly this reason."

    Sort of, strings have a different problem, unlike arrays they cannot be cleared. Anyway, SecureString is IMO a failure. I've never seen anyone actually using it much less using it correctly. It's kind of impossible after all, for example try using SecureString to store the password provided in a WinForms TextBox control to a database.


    • Edited by Mike Danes Thursday, April 10, 2014 12:08 PM
    • Marked as answer by Caillen Thursday, April 17, 2014 8:00 AM
    Thursday, April 10, 2014 12:08 PM
  • The code is "insecure" to begin with because it places decrypted data in memory. If someone manages to dump the process at the right time then the contents of the plaintext array will be visible no matter what you do. This isn't about security, it's about minimizing risks. And if you look at the problem from the risks angle then pinning the array still minimizes the risks even if pinning is done after Decrypt.


    I sort of agree with you there. You're right that sensitive data can always be exposed, if the memory is somehow examined at exactly the right moment.  I would say that "insecure" code (in the context of an information disclosure vulnerability) is code that hasn't taken all reasonable precautions to prevent the data from being exposed.

    However, the current implementation of RsaCryptoServiceProvider takes some of this ability away from the calling code.  Instead of being able to control exactly how long the sensitive data is stored in memory, you're left with a luck factor of "how long before the GC happens to copy over any old memory that it left behind?"  Pinning after the call to Decrypt() appears to be as good as it gets for now, but they could do better, in this case.

    Thursday, April 10, 2014 12:25 PM