7
\$\begingroup\$

Goal:

I needed to calculate the salted hash of a password, where the password is stored as a SecureString. There doesn't seem to be much .NET framework support for SecureString. In particular, none of the crypto functions have overloads that take a SecureString.

The goal is to provide a way to salt and hash a SecureString without creating a managed string or byte array that could be moved by the GC or paged to disk. This is hard to do because Encoding.GetBytes doesn't have a secure input or secure output, and SHA1Cng.HashCore takes the input byte array and clones it. We can't control the cloned copy, so we'll want to shortcut that part and call BCryptHashData ourselves.

I'm also aware that SHA1 is being deprecated in favor of SHA256. Doesn't seem like that long ago that SHA256 was considered overkill, so perhaps I'll skip another upgrade step and get permission to go to SHA512. Regardless, this method can easily be adapted to work with any CNG hash algorithm.

Explanation:

The SecureString stores char (string-type) data that will need to be encoded to byte (binary-type) data. Secure encoding and appending the binary salt would be better as separate steps if there was a SecureByteArray type. Since there isn't, the method takes a SecureString together with an Encoding and a salt. (Edit: Oh hey, look what I found: SafeBuffer. Wonder if it's worth trying to implement a SafeByteArray?)

The method is unsafe because our two options for Encoding.GetBytes take char[] and char*. char[] would have to be created in managed memory, GC pinned and copied from insecureUnmanagedBStr, which made char* the more straightforward option. It would not be hard to make a safe version if that was preferable.

public static class CryptographicExtensions
{
    public unsafe static byte[] ComputeHash(this SHA1Cng implementation, SecureString secureString, Encoding encoding, byte[] salt)
    {
        if (implementation == null) throw new ArgumentNullException("implementation");
        if (secureString == null) throw new ArgumentNullException("secureString");

        var hashAlgorithm = typeof(SHA1Cng).GetField("m_hashAlgorithm", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(implementation);
        var hashHandle = (SafeHandle)hashAlgorithm.GetType().GetField("m_hashHandle", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(hashAlgorithm);

        var insecureUnmanagedBStr = Marshal.SecureStringToBSTR(secureString);
        try
        {
            var numBytes = encoding.GetByteCount((char*)insecureUnmanagedBStr, secureString.Length);
            var insecureUnmanagedBytes = Marshal.AllocHGlobal(numBytes + salt.Length);
            try
            {
                Marshal.Copy(salt, 0, insecureUnmanagedBytes + numBytes, salt.Length);
                encoding.GetBytes((char*)insecureUnmanagedBStr, secureString.Length, (byte*)insecureUnmanagedBytes, numBytes);
                Marshal.ZeroFreeBSTR(insecureUnmanagedBStr);
                insecureUnmanagedBStr = IntPtr.Zero;

                // We call BCryptHashData ourselves instead of using SHA1Cng.HashCore because HashCode requires a byte array and it creates a cloned byte array which we can't control.
                var error = BCryptHashData(hashHandle, insecureUnmanagedBytes, numBytes + salt.Length, 0);
                if (error != 0) throw new CryptographicException(error);
            }
            finally
            {
                Kernel32.ZeroMemory(insecureUnmanagedBytes, (IntPtr)numBytes);
                Marshal.FreeHGlobal(insecureUnmanagedBytes);
            }
        }
        finally
        {
            if (insecureUnmanagedBStr != IntPtr.Zero) Marshal.ZeroFreeBSTR(insecureUnmanagedBStr);
        }

        var hash = (byte[])((byte[])typeof(SHA1Cng).GetMethod("HashFinal", BindingFlags.NonPublic | BindingFlags.Instance).Invoke(implementation, null)).Clone();
        implementation.Initialize();
        return hash;
    }

    [DllImport("bcrypt.dll")]
    private static extern int BCryptHashData(SafeHandle hHash, IntPtr pbInput, int cbInput, int dwFlags);
}
\$\endgroup\$
3
\$\begingroup\$

I'm not 100% sure about it, but this doesn't look right to me.

var insecureUnmanagedBStr = Marshal.SecureStringToBSTR(secureString);
try
{

You're converting to a string outside of the try...finally block. Like I said, I'm not absolutely certain, but it at least looks potentially problematic. Are there any crazy edge cases where insecureUnmanagedBStr won't get cleaned up in the finally? I don't know, but why take the chance?

var insecureUnmanagedBStr = IntPtr.Zero;
try
{
    insecureUnmanagedBStr = Marshal.SecureStringToBSTR(secureString);

    //...
}
finally
{
     if (insecureUnmanagedBStr != IntPtr.Zero) Marshal.ZeroFreeBSTR(insecureUnmanagedBStr);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Now I'm wondering too. I'm not sure if that changes anything. If an exception occurs before the assignment to insecureUnmanagedBStr, finally won't help. Take a look at the way CNG classes set hashObjectBuffer. I saw it earlier today and it's making me think. \$\endgroup\$ – jnm2 Jun 9 '15 at 17:12
  • 4
    \$\begingroup\$ Sigh..... why can I suddenly see myself submitting pull requests to .Net Core for better SecureString support? \$\endgroup\$ – RubberDuck Jun 9 '15 at 17:14
  • 2
    \$\begingroup\$ Most people wouldn't care and a few of us would love you to death. Seriously, if I had the expertise... \$\endgroup\$ – jnm2 Jun 9 '15 at 17:21
  • 1
    \$\begingroup\$ While I'm on a SecureString kick, PasswordEdit might be interesting to you. It's pretty niche, since you have to have the DevExpress WinForms suite and be interested in secure passwords to really care. It could be simplified for the TextBox. \$\endgroup\$ – jnm2 Jun 9 '15 at 18:00
  • 2
    \$\begingroup\$ There is absolutely no problem with having insecureUnmanagedBStr = Marshal.SecureStringToBSTR(secureString); before the try..finally. That is precisely how the idiom is supposed to work. Either it will fail exceptionally, in which case, the finally code is definitely not needed, or it will succeed and enter the try and the guarantee is that the finally code shall execute. \$\endgroup\$ – Jesse C. Slicer Jun 9 '15 at 23:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.