SecureString as SqlParameter value without GC concerns

Question

The purpose here is to make it easy to use sensitive data that is already in the form of a SecureString (example) without converting it to a String object and risking more leaks than necessary.

SecureString isn't about total security, but it is about reducing attack surface. For example, when you call SecureString.AppendChar there is a brief flash where it decrypts the contents, adds your character, and reencrypts. This is still better than storing your password in the clear on the heap for any amount of time.

So in a similar vein, if I'm to use a SecureString as a SqlParameter value, it's best to do as little as possible with the contents in the clear and erase it as soon as possible. This isn't about transport security to SQL server, just C# process memory that has the potential to be paged to disk and end up unerased, in the clear, for years.

Usage:

var secureString = new SecureString();
secureString.AppendChar('a');
secureString.AppendChar('q');
secureString.AppendChar('1');
using (var command = new SqlCommand("select case when @secureParam = 'aq1' then 'yes' else 'no' end", connection))
{
 object returnValue;
 using (command.Parameters.AddSecure("secureParam", secureString))
 {
 // At this point no copies exist in the clear
 returnValue = (string)command.ExecuteScalar();
 // Now one pinned String object exists in the clear (referenced at the internal property command.Parameters[0].CoercedValue)
 }
 // At this point no copies exist in the clear
}

Code:

public static class SecureSqlParameterExtensions
{
 [DllImport("kernel32.dll", EntryPoint = "CopyMemory")]
 private static extern void CopyMemory(IntPtr dest, IntPtr src, IntPtr count);
 [DllImport("kernel32.dll", EntryPoint = "RtlZeroMemory")]
 private static extern void ZeroMemory(IntPtr ptr, IntPtr count);
 /// <summary>
 /// You must dispose the return value as soon as SqlCommand.Execute* is called.
 /// </summary>
 public static IDisposable AddSecure(this SqlParameterCollection collection, string name, SecureString secureString)
 {
 var value = new SecureStringParameterValue(secureString);
 collection.Add(name, SqlDbType.NVarChar).Value = value;
 return value;
 }
 private sealed class SecureStringParameterValue : IConvertible, IDisposable
 {
 private readonly SecureString secureString;
 private int length;
 private string insecureManagedCopy;
 private GCHandle insecureManagedCopyGcHandle;
 public SecureStringParameterValue(SecureString secureString)
 {
 this.secureString = secureString;
 }
 #region IConvertible
 public TypeCode GetTypeCode()
 {
 return TypeCode.String;
 }
 public string ToString(IFormatProvider provider)
 {
 if (insecureManagedCopy != null) return insecureManagedCopy;
 if (secureString == null || secureString.Length == 0) return string.Empty;
 // We waited till the last possible minute.
 // Here's the plan:
 // 1. Create a new managed string initialized to zero
 // 2. Pin the managed string so the GC leaves it alone
 // 3. Copy the contents of the SecureString into the managed string
 // 4. Use the string as a SqlParameter
 // 5. Zero the managed string after Execute* is called and free the GC handle
 length = secureString.Length;
 insecureManagedCopy = new string('0円', length);
 insecureManagedCopyGcHandle = GCHandle.Alloc(insecureManagedCopy, GCHandleType.Pinned); // Do not allow the GC to move this around and leave copies behind
 try
 {
 // This is the only way to read the contents, sadly.
 // SecureStringToBSTR picks where to put it, so we have to copy it from there and zerofree the unmanaged copy as fast as possible.
 var insecureUnmanagedCopy = Marshal.SecureStringToBSTR(secureString);
 try
 {
 CopyMemory(insecureManagedCopyGcHandle.AddrOfPinnedObject(), insecureUnmanagedCopy, (IntPtr)(length * 2));
 }
 finally
 {
 if (insecureUnmanagedCopy != IntPtr.Zero) Marshal.ZeroFreeBSTR(insecureUnmanagedCopy);
 }
 // Now the string managed string has the contents in the clear.
 return insecureManagedCopy;
 }
 catch
 {
 Dispose();
 throw;
 }
 }
 public void Dispose()
 {
 if (insecureManagedCopy == null) return;
 insecureManagedCopy = null;
 ZeroMemory(insecureManagedCopyGcHandle.AddrOfPinnedObject(), (IntPtr)(length * 2));
 insecureManagedCopyGcHandle.Free();
 }
 public bool ToBoolean(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public char ToChar(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public sbyte ToSByte(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public byte ToByte(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public short ToInt16(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public ushort ToUInt16(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public int ToInt32(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public uint ToUInt32(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public long ToInt64(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public ulong ToUInt64(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public float ToSingle(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public double ToDouble(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public decimal ToDecimal(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public DateTime ToDateTime(IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 public object ToType(Type conversionType, IFormatProvider provider)
 {
 throw new NotImplementedException();
 }
 #endregion
 }
}

• jnm2: @200_success I don't think sql is a good tag for this. This has nothing to do with the language SQL or SQL Server. This is really about using a SecureString in a DbParameter, not necessarily a SqlParameter at all.
• Dan Lyons: If the string value is so sensitive that you need to take extreme steps to prevent it sitting in memory, you probably shouldn't be adding it to the database as plain text.
• jnm2: The database is not storing the password. It's salting and hashing it, comparing to a saved hash, and returning a signal.
• Dan Lyons: It may not ultimately store the value in a column as plain text, but your code above is passing the plain text value as a parameter to the database, which is pretty easy to capture. You could salt and hash it first, then pass the value as a SqlBinary parameter, though.

Answer 1

I don't think you should do this.

public string ToString(IFormatProvider provider)

SecureString doesn't override ToString. It just returns it's own name. I would recommend doing the same here. It seems to defeat the purpose of using it, if you're going to such extreme measures to then retrieve the value via a ToString method.

I mean, yes. I understand that you do eventually have to retrieve the actual string, but this is not the place to do that. There's a reason SecureString doesn't provide a method to get to the plain text value. It's supposed to be difficult to get out. Allowing people to call secureParameterValue.ToString() removes all of the benefits of using SecureString.

private sealed class SecureStringParameterValue : IConvertible, IDisposable
{
 private readonly SecureString secureString;
 private int length;
 private string insecureManagedCopy;
 private GCHandle insecureManagedCopyGcHandle;

I love that this is sealed. That's great. Security sensitive things should be sealed. But again, storing a insecureManagedCopy for the life of the class defeats the purpose. Now you're at the mercy of the garbage collector.

When you do need to access the actual value, you want to make sure that the unmanaged copy is removed as quickly as possible. Here is an example implementation that I submitted to the LibGit2Sharp project.

public sealed class SecureUsernamePasswordCredentials : Credentials
{
 /// <summary>
 /// Callback to acquire a credential object.
 /// </summary>
 /// <param name="cred">The newly created credential object.</param>
 /// <returns>0 for success, &lt; 0 to indicate an error, &gt; 0 to indicate no credential was acquired.</returns>
 protected internal override int GitCredentialHandler(out IntPtr cred)
 {
 if (Username == null || Password == null)
 {
 throw new InvalidOperationException("UsernamePasswordCredentials contains a null Username or Password.");
 }
 IntPtr passwordPtr = IntPtr.Zero;
 try
 {
 passwordPtr = Marshal.SecureStringToGlobalAllocUnicode(Password);
 return NativeMethods.git_cred_userpass_plaintext_new(out cred, Username, Marshal.PtrToStringUni(passwordPtr));
 }
 finally
 {
 Marshal.ZeroFreeGlobalAllocUnicode(passwordPtr);
 }
 }

Notice how the consumer is the one responsible for decoding and using it? Also note that the unmanaged string is always released from memory before the method exits. My implementation is faced with many of the same issues. It still has to decode the string, but it never exposes a manage string to anyplace it could be copied. It gets sent to the external library with no room for anyone to ever make copies of the password. You should strive a little more towards that goal.

To make things clear, there's no reason a developer couldn't do this with your implementation.

using (var command = new SqlCommand("select case when @secureParam = 'aq1' then 'yes' else 'no' end", connection))
{
 object returnValue;
 string pswd;
 using (command.Parameters.AddSecure("secureParam", secureString))
 {
 // At this point no copies exist in the clear
 returnValue = (string)command.ExecuteScalar();
 pswd = command.Parameters[0].CoercedValue;
 }
 // At this point the password is stored in a plain string.
}

Or, a developer doesn't use a using block. That would also lead to having the plain string in memory. Even worse, someone could forget to Dispose() it.

---

One last note. Once you've removed the ToString method, there's no longer a reason to implement IConvertible and those nasty unimplmented methods can be removed. Actually, I'm not sure why they were there to begin with. You didn't need to implement IConvertible in order to override ToString.

• jnm2: Thanks for the feedback! I'm struggling to understand how to apply these points. Could you clarify? ToString gets called at the last minute by SqlClient and that's the only reason it exists. If this is not the place to put the unencrypted string into SqlClient's hands, where would it be any safer? Also, I did need to implement IConvertible. That's necessary in order for SqlClient to coerce the value.
• jnm2: Also insecureManagedCopy is not stored for the life of the class, and I am not at the mercy of the garbage collector. During ToString when SqlClient asks for a value, insecureManagedCopy gets set to a zero filled string, pinned so that it cannot be moved by the GC, and then the sensitive data is written into that memory location. During Dispose, insecureManagedCopy is then zeroed and finally unpinned so that the GC can interact with it again. If there was a way to tell when SqlClient was finished reading the string I definitely wouldn't need the disposable pattern.
• RubberDuck: And if Dispose is never called?
• jnm2: The user is responsible. Code analyzers or a finalizer check could help, but that's like asking what if the user forgets to use try...finally with SecureStringToBSTR? At least the pattern is asking to be disposed. This is a better pit of success than manually decrypting every time to work around a lack of support in SqlClient. Plus it looks half decent. Again, if I could zero the memory the instant SqlClient was finished reading it I would.
• RubberDuck: I've dug into this a bit and here's what I think. You have the simplest solution here. I really think that this is the simplest thing that works. Your only option to make it better is to put a ton of work (and code) into creating a classes that wrap up SqlCommand, Parameter, and ParameterCollection. It's the only way I can really see to over come the deficiencies in the code. Even then, I'm not entirely positive that it would do a good job of it. Given the work involved, it's probably not worth it. Just be on the lookout for abuse of this functionality. Some custom diagnostics maybe.