Hashing a SecureString in .NET

Question

In .NET, we have the SecureString class, which is all very well until you come to try and use it, as to (for example) hash the string, you need the plaintext. I've had a go here at writing a function that will hash a SecureString, given a hash function that takes a byte array and outputs a byte array.

private static byte[] HashSecureString(SecureString ss, Func<byte[], byte[]> hash)
{
    // Convert the SecureString to a BSTR
    IntPtr bstr = Marshal.SecureStringToBSTR(ss);

    // BSTR contains the length of the string in bytes in an
    // Int32 stored in the 4 bytes prior to the BSTR pointer
    int length = Marshal.ReadInt32(bstr, -4);

    // Allocate a byte array to copy the string into
    byte[] bytes = new byte[length];

    // Copy the BSTR to the byte array
    Marshal.Copy(bstr, bytes, 0, length);

    // Immediately destroy the BSTR as we don't need it any more
    Marshal.ZeroFreeBSTR(bstr);

    // Hash the byte array
    byte[] hashed = hash(bytes);

    // Destroy the plaintext copy in the byte array
    for (int i = 0; i < length; i++) { bytes[i] = 0; }

    // Return the hash
    return hashed;
}

I believe this will correctly hash the string, and will correctly scrub any copies of the plaintext from memory by the time the function returns, assuming the provided hash function is well behaved and doesn't make any copies of the input that it doesn't scrub itself. Have I missed anything here?

Answer 1

Have I missed anything here?

Yes, you have, a rather fundamental one at that. You cannot scrub the copy of the array left behind when the garbage collector compacts the heap. Marshal.SecureStringToBSTR(ss) is okay because a BSTR is allocated in unmanaged memory so will have a reliable pointer that won't change. In other words, no problem scrubbing that one.

Your byte[] bytes array however contains the copy of the string and is allocated on the GC heap. You make it likely to induce a garbage collection with the hashed[] array. Easily avoided but of course you have little control over other threads in your process allocating memory and inducing a collection. Or for that matter a background GC that was already in progress when your code started running.

The point of SecureString is to never have a cleartext copy of the string in garbage collected memory. Copying it into a managed array violated that guarantee. If you want to make this code secure then you are going to have to write a hash() method that takes the IntPtr and only reads through that pointer.

Beware that if your hash needs to match a hash computed on another machine then you cannot ignore the Encoding that machine would use to turn the string into bytes.

Answer 2

There's always the possibility of using the unmanaged CryptoApi or CNG functions. Bear in mind that SecureString was designed with an unmanaged consumer which has full control over memory management in mind.

If you want to stick to C#, you should pin the temporary array to prevent the GC from moving it around before you get a chance to scrub it:

private static byte[] HashSecureString(SecureString input, Func<byte[], byte[]> hash)
{
    var bstr = Marshal.SecureStringToBSTR(input);
    var length = Marshal.ReadInt32(bstr, -4);
    var bytes = new byte[length];

    var bytesPin = GCHandle.Alloc(bytes, GCHandleType.Pinned);
    try {
        Marshal.Copy(bstr, bytes, 0, length);
        Marshal.ZeroFreeBSTR(bstr);

        return hash(bytes);
    } finally {
        for (var i = 0; i < bytes.Length; i++) { 
            bytes[i] = 0; 
        }

        bytesPin.Free();
    }
}

Answer 3

As a complement to Hans’ answer here’s a suggestion how to implement the hasher. Hans suggests passing the pointer to the unmanaged string to the hash function but that means that client code (= the hash function) needs to deal with unmanaged memory. That’s not ideal.

On the other hand, you can replace the callback by an instance of the following interface:

interface Hasher {
    void Reinitialize();
    void AddByte(byte b);
    byte[] Result { get; }
}

That way the hasher (although it becomes slightly more complex) can be implemented wholly in managed land without leaking secure information. Your HashSecureString would then look as follows:

private static byte[] HashSecureString(SecureString ss, Hasher hasher) {
    IntPtr bstr = Marshal.SecureStringToBSTR(ss);
    try {
        int length = Marshal.ReadInt32(bstr, -4);

        hasher.Reinitialize();

        for (int i = 0; i < length; i++)
            hasher.AddByte(Marshal.ReadByte(bstr, i));

        return hasher.Result;
    }
    finally {
        Marshal.ZeroFreeBSTR(bstr);
    }
}

Note the finally block to make sure that the unmanaged memory is zeroed, no matter what shenanigans the hasher instance does.

Here’s a simple (and not very useful) Hasher implementation to illustrate the interface:

sealed class SingleByteXor : Hasher {
    private readonly byte[] data = new byte[1];

    public void Reinitialize() {
        data[0] = 0;
    }

    public void AddByte(byte b) {
        data[0] ^= b;
    }

    public byte[] Result {
        get { return data; }
    }
}

Answer 4

As a further complement, could you not wrap the logic @KonradRudolph and @HansPassant supplied into a custom Stream implementation?

This would allow you to use the HashAlgorithm.ComputeHash(Stream) method, which would keep the interface managed (although it would be down to you to dispose the stream in good time).

Of course, you are at the mercy of the HashAlgorithm implementation as to how much data ends up in memory at a time (but, of course, that's what the reference source is for!)

Just an idea...

public class SecureStringStream : Stream
{
    public override bool CanRead { get { return true; } }
    public override bool CanWrite { get { return false; } }
    public override bool CanSeek { get { return false; } }

    public override long Position
    {
        get { return _pos; }
        set { throw new NotSupportedException(); }
    }

    public override void Flush() { throw new NotSupportedException(); }
    public override long Seek(long offset, SeekOrigin origin) { throw new NotSupportedException(); }
    public override void SetLength(long value) { throw new NotSupportedException(); }
    public override void Write(byte[] buffer, int offset, int count) { throw new NotSupportedException(); }

    private readonly IntPtr _bstr = IntPtr.Zero;
    private readonly int _length;
    private int _pos;

    public SecureStringStream(SecureString str)
    {
        if (str == null) throw new ArgumentNullException("str");
        _bstr = Marshal.SecureStringToBSTR(str);

        try
        {
            _length = Marshal.ReadInt32(_bstr, -4);
            _pos = 0;
        }
        catch
        {
            if (_bstr != IntPtr.Zero) Marshal.ZeroFreeBSTR(_bstr);
            throw;
        }
    }

    public override long Length { get { return _length; } }

    public override int Read(byte[] buffer, int offset, int count)
    {
        if (buffer == null) throw new ArgumentNullException("buffer");
        if (offset < 0) throw new ArgumentOutOfRangeException("offset");
        if (count < 0) throw new ArgumentOutOfRangeException("count");
        if (offset + count > buffer.Length) throw new ArgumentException("offset + count > buffer");

        if (count > 0 && _pos++ < _length) 
        {
            buffer[offset] = Marshal.ReadByte(_bstr, _pos++);
            return 1;
        }
        else return 0;
    }

    protected override void Dispose(bool disposing)
    {
        try { if (_bstr != IntPtr.Zero) Marshal.ZeroFreeBSTR(_bstr); }
        finally { base.Dispose(disposing); }
    }
}

void RunMe()
{
    using (SecureString s = new SecureString())
    {
        foreach (char c in "jimbobmcgee") s.AppendChar(c);
        s.MakeReadOnly();

        using (SecureStringStream ss = new SecureStringStream(s))
        using (HashAlgorithm h = MD5.Create())
        {
            Console.WriteLine(Convert.ToBase64String(h.ComputeHash(ss)));
        }
    }
}