Security of ProtectedObject

Posted on

Problem

Is ProtectedObject really secure? Otherwise, do you have any advice?

public class ProtectedBinary : IDisposable
{
    private byte[] pBArray;
    private int originPBArraySize;

    public ProtectedBinary(byte[] data)
    {
        originPBArraySize = data.Length;
        Pad(ref data);
        ProtectedMemory.Protect(data, MemoryProtectionScope.SameProcess);

        pBArray = new byte[data.Length];
        Array.ConstrainedCopy(data, 0, pBArray, 0, data.Length);
    }

    public void Get(out byte[] outArray)
    {
        ProtectedMemory.Unprotect(pBArray, MemoryProtectionScope.SameProcess);

        outArray = new byte[originPBArraySize];
        Array.Copy(pBArray, outArray, originPBArraySize);
        Unpad(ref outArray);

        ProtectedMemory.Protect(pBArray, MemoryProtectionScope.SameProcess);
    }

    public void Clear()
    {
        pBArray.Clear(); // set all bytes to 0x00
        originPBArraySize = 0;
    }

    public void Dispose()
    {
        Clear();
    }

    private void Pad(ref byte[] bytes)
    {
        Array.Resize(ref bytes, 16 * ((bytes.Length + 15) / 16));
    }

    private void Unpad(ref byte[] bytes)
    {
        Array.Resize(ref bytes, originPBArraySize);
    }
}

public class ProtectedObject<T> : IDisposable
{
    private ProtectedBinary pBin;

    private bool isDisposed;

    public ProtectedObject(T obj)
    {
        byte[] serializedObject = ULBinConvert.Serialize(obj);
        pBin = new ProtectedBinary(serializedObject);
    }

    public void Get(out T obj)
    {
        if (isDisposed)
            throw new ObjectDisposedException("Protected object is disposed");
        byte[] serializedObject;
        pBin.Get(out serializedObject);
        obj = ULBinConvert.Deserialize<T>(serializedObject);
        serializedObject.Clear();
    }

    public void Dispose()
    {
        if (!isDisposed)
        {
            pBin.Dispose();
            isDisposed = true;
        }
    }
}

Solution

Most of it looks fine to me, but why do you have void methods with out parameters? This makes no sense to me, and only serves to make usage more awkward. As such I would recommend:

public T Get()
{
    if (isDisposed)
        throw new ObjectDisposedException("Protected object is disposed");
    byte[] serializedObject = pBin.Get();
    T result = ULBinConvert.Deserialize<T>(serializedObject);
    serializedObject.Clear();
    return result;
}

Secondly, I don’t like a lot of your variable names. You’re not paying by the character, so there’s no real benefit in shortening them at the cost of making it a slower read. pBin is significantly less readable than protectedBinary

You should also refrain from putting things like array into the variable name unless there is no better way to describe the variable. Most of the time it is unnecessary, as it is in pBArray or outArray.

Leave a Reply

Your email address will not be published. Required fields are marked *