3
\$\begingroup\$

When dealing with sensitive data, it should be protected from modification and unauthorized viewing as much as possible. This is what this ProtectedPtr class aims to do: encrypt your data when you aren't using it, and decrypt your data when you need access to it, using Windows CryptProtectMemory API.

The goals for this class are:

  • Encrypt memory as long as possible
  • Automatically decrypt data when it needs to be accessed
  • Automatically re-encrypt data when possible
  • Securely wipe all copies of unencrypted data at the end of their scope
  • Manage protected data securely

I provide a constant time comparison function in the operator==, but for objects to be compared in constant time (for at least the only method I know of) they need to be in byte arrays. This is why the Serializer classes exist, to convert the data type being used by the ProtectedPtr into byte arrays, and to return a reference to the raw data. In a primitive type this is trivial, but for a user-created object, dereferencing the object itself may not return the data needed. For example in a std::pair, the pair::first and/or pair::second functions would need to be called in order to return the data that needs to be encrypted/decrypted/converted to byte arrays.


As the class already decrypts the data automatically, I was originally going to attempt to automatically encrypt the protected data after it is returned. So this would change the const operator* to:

const T& operator*() const
{
    ProtectMemory(false);
    T temp = *protectedData;
    ProtectMemory(true);
    return temp;
} 

Because the data returned can't be modified, I figured I can return a copy of the unencrypted data and encrypted the original data.

My problem with this is that I have no control over the 'temp' variable. The user could keep it unencrypted for the length of the client program, making all the work of the ProtectedPtr class for naught.


Is there any security or usability problems or fixes I can apply? Any other areas of concern for this class?

#include "Windows.h"
#include "Wincrypt.h"
#include <string>
#include <memory>

#pragma comment(lib, "crypt32.lib")
#pragma once

namespace Protected_Ptr
{
//base class that converts data of type T to a byte array, 
//gets size of data, and returns reference to raw data
template <class T>
class Serializer
{
public:
    //return size of data
    virtual std::size_t getSize(const T& obj) const = 0;
    //return reference to raw data
    virtual T* getRawData(T& obj) const = 0;
    //convert data into byte array
    virtual byte* serialize(T& obj) const = 0;
};

template <class T>
class PrimitiveSerializer : public Serializer<T>
{
public:
    virtual std::size_t getSize(const T& obj) const { return sizeof(obj); }
    virtual T* getRawData(T& obj) const { return &obj; }
    virtual byte* serialize(T& obj) const
    {
        const size_t size = getSize(obj);
        byte* out = new byte[size];
        memcpy(out, getRawData(obj), size);
        return out;
    }
};

class StringSerializer : public Serializer<std::string>
{
public:
    virtual std::size_t getSize(const std::string& str) const { return str.length(); }
    virtual std::string* getRawData(std::string& str) const { return &str; }    
    virtual byte* serialize(std::string& str) const
    {
        const std::size_t size = getSize(str);
        byte* out = new byte[size];
        memcpy(out, str.c_str(), size);
        return out;
    }

};

template <class T, class S = PrimitiveSerializer<T>>
class ProtectedPtr
{
public:
    explicit ProtectedPtr(bool wipeOnExit = true) noexcept
        : protectedData(nullptr), overwriteOnExit(wipeOnExit) {};
    explicit ProtectedPtr(T *obj, bool wipeOnExit = true)
        : overrideOnExit(wipeOnExit)
    {
        assign(obj);
        ProtectMemory(true);
    }
    explicit ProtectedPtr(ProtectedPtr other) : protectedData(nullptr)
    {
        other.swap(*this);
    }
    explicit ProtectedPtr(ProtectedPtr&& other) : protectedData(nullptr) noexcept
    {
        other.swap(*this);
    }
    ~ProtectedPtr()
    {
        ProtectMemory(false);
        SecureWipeData();
    }

    void SetWipeOnExit(bool wipe) { overwriteOnExit = wipe; }
    bool IsProtected() const { return isEncrypted };
    void ProtectMemory(bool encrypt)
    {
        size_t mod;
        size_t dataBlockSize;
        size_t dataSize = sizeof(*protectedData);

        //CryptProtectMemory requires data to be a multiple of its block size
        if (mod = dataSize % CRYPTPROTECTMEMORY_BLOCK_SIZE)
            dataBlockSize = dataSize + (CRYPTPROTECTMEMORY_BLOCK_SIZE - mod);
        else
            dataBlockSize = dataSize;

        if (encrypt && !isEncrypted)
        {
            isEncrypted = true;
            if (!CryptProtectMemory(getRawPtr(), dataBlockSize,
                CRYPTPROTECTMEMORY_SAME_PROCESS))
            {
                cerr << "CryptProtectMemory failed: " << GetLastError() << endl;
            }
        }
        else if (!encrypt && isEncrypted)
        {
            isEncrypted = false;
            if (!CryptUnprotectMemory(getRawPtr(), dataBlockSize,
                CRYPTPROTECTMEMORY_SAME_PROCESS))
            {
                cerr << "CryptProtectMemory failed: " << GetLastError() << endl;
            }
        }

        SecureZeroMemory(&mod, sizeof(mod));
        SecureZeroMemory(&dataSize, sizeof(dataSize));
        SecureZeroMemory(&dataBlockSize, sizeof(dataBlockSize));
    }
    void SecureWipeData()
    {
        if (overwriteOnExit)
            SecureZeroMemory(getRawPtr(), sizeof(*protectedData));
    }

    void swap(ProtectedPtr& other) noexcept
    {
        using std::swap;

        //make sure data is encrypted
        ProtectMemory(true);
        other.ProtectMemory(true);

        swap(*this->protectedData, other.protectedData);
        swap(*this->isEncrypted, other.isEncrypted);
        swap(*this->overwriteOnExit, other.overwriteOnExit);
    }

    T& operator*()
    {
        ProtectMemory(false);
        return *protectedData;
    }
    const T& operator*() const
    {
        ProtectMemory(false);
        return *protectedData;
    }

    T* const operator->()
    {
        ProtectMemory(false);
        return protectedData.operator->();
    }
    const T* const operator->() const
    {
        ProtectMemory(false);
        return protectedData.operator->();
    }

    ProtectedPtr& operator=(ProtectedPtr rhs)
    {
        rhs.swap(*this);
        return *this;
    }
    ProtectedPtr& operator=(ProtectedPtr&& rhs) noexcept
    {
        rhs.swap(*this);
        return *this;
    }

    //constant time comparison 
    bool operator==(ProtectedPtr& other)
    {
        if (sizeof(*protectedData) != sizeof(*other))
            return false;

        volatile byte* thisData = serializeData();
        ProtectMemory(true);

        volatile byte* otherData = other.serializeData();
        other.ProtectMemory(true);

        volatile byte result = 0;
        for (int i = 0; i < sizeof(*protectedData); i++)
        {
            result |= thisData[i] ^ otherData[i];
            //securely wipe unencrypted copies of data
            thisData[i] = 0;
            otherData[i] = 0;
        }

        return result == 0;
    }
    bool operator!=(ProtectedPtr& other)
    {
        return !(*this == other);
    }

    explicit operator bool() const { return (bool)protectedData; }

    T& get() { return this->operator*(); }
    const T& data() const { return this->operator*(); }
    void assign(T *obj)
    {
        //if protectedData is already pointing to something,
        //securely overwrite and delete it
        if (protectedData)
        {
            ProtectMemory(false);
            SecureWipeData();
            protectedData.release();
        }

        //point to copy of data, encrypt it, and overwrite
        //original unencrypted data
        protectedData = std::make_unique<T>(*obj);
        ProtectMemory(true);
        SecureZeroMemory(obj, sizeof(obj));
    }
    bool empty() const { return (bool)*this; }

private:
    //returns reference to data pointed to
    T* getRawPtr() { return serializer.getRawData(*protectedData); }
    byte* serializeData()
    {
        ProtectMemory(false);
        return serializer.serialize(*protectedData);
    }

    S serializer;
    std::unique_ptr<T> protectedData;
    bool isEncrypted = false;
    bool overwriteOnExit;
};

template <class T>
void swap(ProtectedPtr<T>& lhs, ProtectedPtr<T>& rhs) noexcept { lhs.swap(rhs); }

}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! It seems fishy to me to mess with bytes at this level. I’m afraid at -O3 or similar this might explode. Have you tried to thoroughly test this? \$\endgroup\$ – Incomputable Nov 30 '17 at 22:27
  • \$\begingroup\$ Thanks! The Visual Studio compiler at least, with all it's various optimization options enabled, does not optimize the constant time of the == operator away or break anything. I haven't checked g++ though. As for why I'm comparing this way, see stackoverflow.com/questions/25373767/… \$\endgroup\$ – Andrew LeFevre Dec 1 '17 at 1:17
4
\$\begingroup\$

This is a very interesting idea! I have to say your implementation is definitely different from how I would have tackled that.

There are two main axes of improvement that I see:

1: Why Pointers?

I don't think you should be treating this as a pointer, but as an object wrapper instead. There is no reason to not be able to apply this to a stack-based object.

If someone wants pointer-like functionality, nothing is stopping them from creating a std::unique_ptr<ProtectedObject<T>> or std::shared_ptr<ProtectedObject<T>>

2: RAII, please!

If the unprotect -> reprotect flow really should be handled through RAII. Think std::mutex and std::unique_lock as references.

The idea with this is to make it impossible to accidentally leave an object unprotected because of an early return or thrown exception.


A few notes on the code itself while I'm at it:

Duck typing or type erasure, choose one.

When taking a duck-typed operation as template parameter, creating a polymorphic base class is just redundant, and extra weight. Eventually, you want to use concepts instead, but for now, documentation is enough.

Spelling:

it's Primitive, not Primative.

Broken code:

Your copy constructor won't compile: swap() requires non-const operands.

Do not encrypt in swap

swap should leave the objects in swapped state. Forcefully encrypting the objects will just lead to surprises.

At worse, throw an exception if one of them is currently unencrypted, but don't surprise users like that.

Why is overwriteOnExit even an option?

You zero the data whenever you encrypt, so I fail to see a scenario where you would ever want that set to false.

If anything, should you move to RAII semantics, it should actually be illegal to destroy the object when it is in an unencrypted state,

followup feedback:

Thanks for the input! First, the reason I'm implementing this as a pointer class is because for my use cases, I need to protect dynamically allocated data. Wrapping the class around the unique_ptr makes this easy.

This kinda violates the separation of concerns principle. Have a class/function do 1 thing and do it well is generally preferable.

I'm a little confused on what you're saying about choosing between duck-typing and type erasure. I'm pretty new to programming, and while I did some research, I'm not really sure what you mean, you think I should get rid of the polymorphic base Serializer?

Basically, virtual functions (type erasure) are only needed when you need to access an instance through a pointer to the base. Since you are instantiating the subclass, and accessing the methods through it directly, you don't need the virtual interface. You can simply use the object as if it had the interface (duck typing).

Data is automatically unencrypted whenever the user requests it, so I don't see how encrypting in swap is an issue. I'm just making sure when copying or moving, the other ProtectedPtr that may never be used again is safely encrypted.

The thing with this is that swap(a, b) has the implicit contract that b will be in the same state a was, and vice-versa. Doing otherwise can lead to surprises, and surprises are never good.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the input! First, the reason I'm implementing this as a pointer class is because for my use cases, I need to protect dynamically allocated data. Wrapping the class around the unique_ptr makes this easy. \$\endgroup\$ – Andrew LeFevre Dec 1 '17 at 1:06
  • \$\begingroup\$ I'm a little confused on what you're saying about choosing between duck-typing and type erasure. I'm pretty new to programming, and while I did some research, I'm not really sure what you mean, you think I should get rid of the polymorphic base Serializer? \$\endgroup\$ – Andrew LeFevre Dec 1 '17 at 1:10
  • \$\begingroup\$ Data is automattically unencrypted whenever the user requests it, so I don't see how encrypting in swap is an issue. I'm just making sure when copying or moving, the other ProtectedPtr that may never be used again is safely encrypted. \$\endgroup\$ – Andrew LeFevre Dec 1 '17 at 1:11
  • \$\begingroup\$ OverwriteOnExit is an option because some classes already do so, and sometimes overwriting twice can throw errors. Also, the data is not zeroed out upon encryption; it wouldn't be able to be decrypted if that was the case. I overwrite the data in the destructor by default because I don't want the unencrypted data to linger. Deleting encrypted data, at least after using CryptProtectMemory, throws major errors. \$\endgroup\$ – Andrew LeFevre Dec 1 '17 at 1:15
  • \$\begingroup\$ @AndrewLeFevre I've put most answers in my post directly, as for that last point: I was referring to zeroing the unencrypted data after encrypting it. \$\endgroup\$ – Frank Dec 1 '17 at 1:21

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.