8
\$\begingroup\$

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);
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Relevant reading. TL;DR: Nothing is safe when the OS writes your RAM to disk. \$\endgroup\$
    – RubberDuck
    Oct 17 '15 at 14:24
  • \$\begingroup\$ The encoding.GetBytes(char*, int, byte*, int) method allocates a managed char[] array and copies the string into it, and thus it voids all the security which was attempted to be preserved. See the source of the method here: referencesource.microsoft.com/#mscorlib/system/text/… \$\endgroup\$
    – treaschf
    Jun 15 '17 at 7:27
11
\$\begingroup\$

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

\$\endgroup\$
7
  • \$\begingroup\$ This is pretty much the "by the book" implementation. It hangs on to the string for the absolute minimum amount of time. +1. \$\endgroup\$
    – RubberDuck
    Oct 17 '15 at 11:58
  • \$\begingroup\$ Marshal.PtrToStringUni allocates managed string so until GC clear this we will have unprotected string with sensitive data in memory. I might be wrong, but it seems to be less secure. \$\endgroup\$ Oct 17 '15 at 12:03
  • \$\begingroup\$ Since C# internally stores strings as UTF-16, wouldn't it be better to use that instead of UTF-8? \$\endgroup\$ Oct 17 '15 at 22:00
  • 1
    \$\begingroup\$ Regarding your second solution: If someone calls wrapper.ToByteArray() multiple times, aren't all _bytes instances except the last one left for GC? Only the last instance will be cleaned up by Dispose. I added this first line to ToByteArray to check this: if (this.bytes != null) return this.bytes; correct me if I'm wrong about this. \$\endgroup\$
    – H B
    Nov 14 '16 at 15:42
  • 2
    \$\begingroup\$ seems that the first code sample should use ZeroFreeGlobalAllocUnicode instead of ZeroFreeBSTR \$\endgroup\$ Aug 30 '17 at 20:02
-3
\$\begingroup\$

I guess the Finally-block is not executed if the return statement makes it leave before then. Due to this the free is not called.

\$\endgroup\$
3
  • 4
    \$\begingroup\$ The finally block is always reached wether there is an exception thrown or a return statement before. \$\endgroup\$
    – Heslacher
    Oct 17 '15 at 11:33
  • 1
    \$\begingroup\$ Not in case of an an asynchronous exception happening on the thread (e.g. OutOfMemoryException, StackOverflowException). \$\endgroup\$
    – Kai
    Oct 17 '15 at 11:34
  • 1
    \$\begingroup\$ Exceptions like OutOfMemory are sometimes referred to as "fatal" exceptions and there's nothing you can do about them. The only thing really worth doing is logging that they happened. \$\endgroup\$ Oct 17 '15 at 22:00

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.