Hashing a SecureString using Cryptography Next Generation

Question

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);
}

Answer 1

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);
}