\$\begingroup\$

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, < 0 to indicate an error, > 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.