Converting a SecureString to a byte array

Question

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

• RubberDuck: Relevant reading. TL;DR: Nothing is safe when the OS writes your RAM to disk.
• treaschf: 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/…

Answer 1

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

• RubberDuck: This is pretty much the "by the book" implementation. It hangs on to the string for the absolute minimum amount of time. +1.
• Artem Sokolov: 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.
• Ismael Miguel: Since C# internally stores strings as UTF-16, wouldn't it be better to use that instead of UTF-8?
• H B: 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.
• tomasz_kajetan_stanczak: seems that the first code sample should use ZeroFreeGlobalAllocUnicode instead of ZeroFreeBSTR

Answer 2

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.

• Heslacher: The finally block is always reached wether there is an exception thrown or a return statement before.
• Kai: Not in case of an an asynchronous exception happening on the thread (e.g. OutOfMemoryException, StackOverflowException).
• craftworkgames: 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.