Converting a SecureString to a byte array

Does it allocates something that hasn't freed? Something that remains in memory? Except result byte array, ofc.

private unsafe byte[] ToByteArray(SecureString secureString, Encoding encoding = null)
{
    if (secureString == null)
        throw new ArgumentNullException(nameof(secureString));
    if (encoding == null)
        encoding = Encoding.UTF8;

    int maxLength = encoding.GetMaxByteCount(secureString.Length);

    IntPtr bytes = Marshal.AllocHGlobal(maxLength);
    IntPtr str   = Marshal.SecureStringToBSTR(secureString);

    try
    {
        char* chars = (char*)str.ToPointer();
        byte* bptr  = (byte*)bytes.ToPointer();
        int len     = encoding.GetBytes(chars, secureString.Length, bptr, maxLength);

        byte[] _bytes = new byte[len];
        for (int i = 0; i < len; ++i)
        {
            _bytes[i] = *bptr;
            bptr++;
        }

        return _bytes;
    }
    finally
    {
        Marshal.FreeHGlobal(bytes);
        Marshal.ZeroFreeBSTR(str);
    }
}

Although you have done a decent job, you could do this in a better way, without even to use unsafe.

First thing to mention, you should always use braces {} although they are optional for single lined if statements. This will just make your code less error prone which is in such a security context wanted.

Instead of using if (encoding==null) { encoding = Encoding.UTF8; } you can use the null coalescing operator ??.

How I use this usually (integrated in your method)

private byte[] ToByteArray(SecureString secureString, Encoding encoding = null)
{
    if (secureString == null)
    {
        throw new ArgumentNullException(nameof(secureString));
    }

    encoding = encoding ?? Encoding.UTF8;

    IntPtr unmanagedString = IntPtr.Zero;
    try
    {
        unmanagedString = Marshal.SecureStringToGlobalAllocUnicode(secureString);

        return encoding.GetBytes(Marshal.PtrToStringUni(unmanagedString));
    }
    finally
    {
        if (unmanagedString != IntPtr.Zero)
        {
            Marshal.ZeroFreeBSTR(unmanagedString);
        }
    }
}

Based on the valid comment


Marshal.PtrToStringUni allocates managed string so until GC clear this we will have unprotected string with sensitive data in memory.

I would like to suggest another aproach which depends on your implementation. The main problem I see is that with your implementation the returned byte[] needs to be cleaned up from the caller of the code.

How about having a class which implements IDisposable which is wrapping the shown method ?

public sealed class SecureStringWrapper : IDisposable  
{
    private readonly Encoding encoding;
    private readonly SecureString secureString;
    private byte[] _bytes = null;

    public SecureStringWrapper(SecureString secureString)
          : this(secureString, Encoding.UTF8)
    {}

    public SecureStringWrapper(SecureString secureString, Encoding encoding)
    {
        if (secureString == null)
        {
            throw new ArgumentNullException(nameof(secureString));
        }

        this.encoding = encoding ?? Encoding.UTF8
        this.secureString = secureString;
    }

    public unsafe byte[] ToByteArray()
    {

        int maxLength = encoding.GetMaxByteCount(secureString.Length);

        IntPtr bytes = IntPtr.Zero;
        IntPtr str   = IntPtr.Zero;

        try
        {
            bytes = Marshal.AllocHGlobal(maxLength);
            str   = Marshal.SecureStringToBSTR(secureString);

            char* chars = (char*)str.ToPointer();
            byte* bptr  = (byte*)bytes.ToPointer();
            int len     = encoding.GetBytes(chars, secureString.Length, bptr, maxLength);

            _bytes = new byte[len];
            for (int i = 0; i < len; ++i)
            {
                _bytes[i] = *bptr;
                bptr++;
            }

            return _bytes;
        }
        finally
        {
            if (bytes != IntPtr.Zero)
            {
                Marshal.FreeHGlobal(bytes);
            }
            if (str != IntPtr.Zero)
            {
                Marshal.ZeroFreeBSTR(str);
            }
        }
    }

    private bool _disposed = false;

    public void Dispose()
    {
        if (!_disposed)
        {
            Destroy();
            _disposed = true;
        }
        GC.SuppressFinalize(this);
    }

    private void Destroy()
    {
        if (_bytes == null) { return; }

        for (int i = 0; i < _bytes.Length; i++)
        {
            _bytes[i] = 0;
        }
        _bytes = null;
    }

    ~SecureStringWrapper()
    {
        Dispose();
    }
}

now the caller could use it like so

using (SecureStringWrapper wrapper = new SecureStringWrapper(secureString))
{

    byte[] _bytes = wrapper.ToByteArray();
    // use _bytes like wanted

}

A good read about the topic of SecureString: http://web.archive.org/web/20090928112609/http://dotnet.org.za/markn/archive/2008/10/04/handling-passwords.aspx

StackExchange Code Review Q#107860, answer score: 11