Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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);
    }
}
share|improve this question
    
Relevant reading. TL;DR: Nothing is safe when the OS writes your RAM to disk. – RubberDuck Oct 17 '15 at 14:24

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

share|improve this answer
    
This is pretty much the "by the book" implementation. It hangs on to the string for the absolute minimum amount of time. +1. – RubberDuck Oct 17 '15 at 11:58
    
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. – Artem Sokolov Oct 17 '15 at 12:03
    
Since C# internally stores strings as UTF-16, wouldn't it be better to use that instead of UTF-8? – Ismael Miguel Oct 17 '15 at 22:00
    
Shouldn't you clear the bptr too? Or FreeHGlobal will clear its content? – treaschf Jun 11 at 18:23

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.

share|improve this answer
3  
The finally block is always reached wether there is an exception thrown or a return statement before. – Heslacher Oct 17 '15 at 11:33
1  
Not in case of an an asynchronous exception happening on the thread (e.g. OutOfMemoryException, StackOverflowException). – Kai Oct 17 '15 at 11:34
1  
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. – craftworkgames Oct 17 '15 at 22:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.