8

I've a function GetPassword, that returns a SecureString type.

When I pass this secure string to Rfc2898DeriveBytes to generate a key, Visual Studio shows an error. My limited knowledge tells me that it is because Rfc2898DeriveBytes accepts only a string and not a secure string. Is there a workaround to this?

//read the password from terminal
Console.Write("Insert password");
securePwd = myCryptography.GetPassword();

//dont know why the salt is initialized like this
byte[] salt = new byte[] { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0xF1, 0xF0, 0xEE, 0x21, 0x22, 0x45 };
 try
 {   //PBKDF2 standard 
     Rfc2898DeriveBytes key = new Rfc2898DeriveBytes(securePwd, salt, iterationsPwd);
CodesInChaos
  • 106,488
  • 23
  • 218
  • 262
NoobTom
  • 555
  • 1
  • 9
  • 21

4 Answers4

8

I found it interesting that the Rfc2898DeriveBytes class does not support a SecureString overload for passing the password used in deriving the key.

WPF allows for handling passwords as SecureString objects with the PasswordBox control. It seemed like such a waste that the added security that this control offers was lost due to the fact we could not pass in a SecureString to the constructor. However, erickson brought up the excellent point of using the byte[] instead of the string overload as it is relatively easier to properly manage the contents of a byte[] in memory than a string.

Using erickson's suggestion as inspiration I came up with the following wrapper which should allow for using the value of the password protected by SecureString with minimal exposure of the plaintext value in memory.

private byte[] DeriveKey(SecureString password, byte[] salt, int iterations, int keyByteLength)
{
    IntPtr ptr = Marshal.SecureStringToBSTR(password);
    byte[] passwordByteArray = null;
    try
    {
        int length = Marshal.ReadInt32(ptr, -4);
        passwordByteArray = new byte[length];
        GCHandle handle = GCHandle.Alloc(passwordByteArray, GCHandleType.Pinned);
        try
        {
            for (int i = 0; i < length; i++)
            {
                passwordByteArray[i] = Marshal.ReadByte(ptr, i);
            }

            using (var rfc2898 = new Rfc2898DeriveBytes(passwordByteArray, salt, iterations))
            {
                return rfc2898.GetBytes(keyByteLength);
            }
        }
        finally
        {
            Array.Clear(passwordByteArray, 0, passwordByteArray.Length);  
            handle.Free();
        }
    }
    finally
    {
        Marshal.ZeroFreeBSTR(ptr);
    }
}

This approach leverages the fact that BSTR is a pointer pointing to the first character of the data string with a four byte length prefix.

Important points:

  • By wrapping Rfc2898DeriveBytes in a using statement it ensures that it is disposed in a deterministic manner. This is important as it has a internal HMACSHA1 object which is a KeyedHashAlgorithm and needs to have the copy of the key (password) it possesses to be zeroed out in the call to Dispose. See Reference Source for full details.
  • As soon as we are done with the BSTR we zero it out and free it via ZeroFreeBSTR.
  • Lastly, we zero out (clear) of our copy of the password.
  • Update: Added pinning of the byte[]. As discussed in the comments of this answer, if the byte[] is not pinned then the garbage collector could relocate the object during collection and we would be left with no way to zero out the original copy.

This should keep the plaintext password in memory for the shortest amount of time and not weaken the gains of using SecureString too much. Although, if the attacker has access to RAM you probably have bigger problems. Another point is that we can only only manage our own copies of the password, the API we are using could very well mismanage (not zero out/clear) their copies. To the best of my knowledge this is not the case with Rfc2898DeriveBytes, although their copy of the byte[] key (password) is not pinned and therefore traces of the array may hang around if it was moved in the heap before being zeroed out. The message here is that code can look secure, but problems may lie underneath.

If anyone finds any serious holes in this implementation, please let me know.

Community
  • 1
  • 1
Derek W
  • 9,708
  • 5
  • 58
  • 67
  • This is exactly what I was hoping to find, and I believe this is as secure as it gets. In my short research, I believe you've covered every point. Because the implementation of `Rfc2898DeriveBytes` does not pin the `byte[]`, the memory may be available for a longer duration if the garbage collector relocates it, exposing an attack vector. Though, we know that if the attacker has access to RAM, this is the least of our concerns. Your solution limits the appearance of unpinned sensitive data to the behavior of the API. – Nicholas Miller Aug 27 '18 at 21:31
  • I was wondering if we could just return the `rfc2898` directly while still clearing the `passwordByteArray`. You could then have a `using` on the `DeriveKey` function call. It seems like you can do this since [`Rfc2898DeriveBytes` uses `HMAC`](https://referencesource.microsoft.com/#mscorlib/system/security/cryptography/rfc2898derivebytes.cs,112), which creates a [`.Clone()` of your byte array](https://referencesource.microsoft.com/#mscorlib/system/security/cryptography/hmac.cs,84). – dosentmatter Oct 04 '21 at 09:06
  • It also [clears the array on disposal](https://referencesource.microsoft.com/#mscorlib/system/security/cryptography/keyedhashalgorithm.cs,25) by [calling the base method](https://referencesource.microsoft.com/#mscorlib/system/security/cryptography/hmac.cs,175). The array isn't pinned though. By returning `DeriveKey`, you can generate as many bytes as needed. I tested returning `rfc2898` in powershell and it seems to work. Changing the password with fixed salt gives different keys, so that means the password is cloned and isn't stuck being zeroed out. – dosentmatter Oct 04 '21 at 09:07
6

Apparently, you can violate the protection afforded by SecureString and expose its internal state via the Marshal.SecureStringToBSTR() function.

Rather than creating a String out of the result, copy the content to a Byte[] to pass to Rfc2898DeriveBytes. Creating a String would prevent you from destroying the password information, allowing it to hang out in the heap indefinitely, or get paged to disk, which in turn increases the chances that an attacker can find it. Instead, you should destroy the password as soon as you are finished using it, by filling the array with zeros. For the same reason, you should also assign a zero to each element of the BSTR as you copy it to the Byte[].

Salt should be randomly selected for each hashed password, not a fixed, predictable value, otherwise a pre-computed dictionary attack is possible. You should iterate many tens of thousands of times in order to prevent brute force attacks.

erickson
  • 265,237
  • 58
  • 395
  • 493
  • thank you, but i need to encrypt a file in a machine with a password and decrypt it on a different machine with the same password. Therefore i think salt must be the same or the second machine would be unable to decrypt it. Am i correct? – NoobTom Mar 19 '12 at 08:56
  • I choose the other as answer because it is more "formal" but i really appreciate your tricky suggestion – NoobTom Mar 19 '12 at 08:57
  • @NoobTom Yes, of course the salt would have to be the same on both machines. When using password hashing for authentication purposes, the hash, the salt, and the number of iterations should be stored so that you can recompute the hash when presented with a password to see if the new hash matches the stored hash. You should never use a hard-coded hash for password authentication. – erickson Mar 19 '12 at 16:18
  • Note: It is also up to the API to responsibly manage and zero out it's own potential copy of the key. Looking at [Reference Source](https://referencesource.microsoft.com/#mscorlib/system/security/cryptography/rfc2898derivebytes.cs): As long as the password byte array is 64 bytes or less, then the `HMACSHA1` object internal to the `Rfc2898DeriveBytes` object has it's own cloned copy of the password byte array. Thankfully, `HMACSHA1` is a `KeyedHashAlgorithm` which provides a zeroing out of it's copy of the key in the call to *Dispose*. – Derek W May 08 '17 at 18:52
  • I would also add that pinning the `byte[]` is important to do as well. If the `byte[]` is not pinned then the GC could relocate it during collection and leave us with no method to zero out the original array. – Derek W May 09 '17 at 12:20
5

After doing some research and looking at previous answers on stackoverflow mentioning SecureString, that answer is almost certainly: "No". Only the creators of the API can accept SecureString and handle it correctly internally. And they can only do that with help of the platform.

If you - as a user - could retrieve the plain text String, you would have negated most of the advantages of using SecureString in the first place. It would even be a bit dangerous as you would create secure looking code, that would not actually be secure at all (edit: at least not when it comes to protecting in-memory data).

Maarten Bodewes
  • 90,524
  • 13
  • 150
  • 263
0
public static string GetSecureString(SecureString str) {
    System.Net.NetworkCredential cred = new System.Net.NetworkCredential(null, str);
    return cred.Password;             
}
Dave
  • 3,073
  • 7
  • 20
  • 33