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
: https://waybackassets.bk21.net/20090928112609/http://dotnet.org.za/markn/archive/2008/10/04/handling-passwords.aspx