Hacking a SecureString based on std::basic_string for C++

Question

Inspired by reading "How-to write a password-safe class?", I tried some clever (or dumb-fool) hack to create a widely-useable secure string using the std::basic_string-template, which does not need to be explicitly securely erased itself.
At least gcc and clang seem not to choke on it (coliru):

#include <string>
namespace my_secure {
void SecureZeroMemory(void* p, std::size_t n) {
 for(volatile char* x = static_cast<char*>(p); n; --n)
 *x++ = 0;
}
// Minimal allocator zeroing on deallocation
template <typename T> struct secure_allocator {
 using value_type = T;
 secure_allocator() = default;
 template <class U> secure_allocator(const secure_allocator<U>&) {}
 T* allocate(std::size_t n) { return new T[n]; }
 void deallocate(T* p, std::size_t n) {
 SecureZeroMemory(p, n * sizeof *p);
 delete [] p;
 }
};
template <typename T, typename U>
inline bool operator== (const secure_allocator<T>&, const secure_allocator<U>&) {
 return true;
}
template <typename T, typename U>
inline bool operator!= (const secure_allocator<T>&, const secure_allocator<U>&) {
 return false;
}
using secure_string = std::basic_string<char, std::char_traits<char>,
 secure_allocator<char>>;
}
namespace std {
// Zero the strings own memory on destruction
template<> my_secure::secure_string::~basic_string() {
 using X =std::basic_string<char, std::char_traits<char>,
 my_secure::secure_allocator<float>>;
 ((X*)this)->~X();
 my_secure::SecureZeroMemory(this, sizeof *this);
}
}

And a short program using it to do nothing much:

//#include "my_secure.h"
using my_secure::secure_string;
#include <iostream>
int main() {
 secure_string s = "Hello World!";
 std::cout << s << '\n';
}

Some specific concerns:

1. How badly did I break the standard?

   • Does the fact that one of the template-arguments is my own type heal the fact that I added my own explicit specialization to ::std?
   • Are the two types actually guaranteed to be similar enough that my bait-and-switch in the destructor is ok?

2. Is there any actual implementation where the liberties I took with the standard will come back to haunt me?

3. Did I miss any place where I should zero memory after use? Or is there any chance that anything will slip by?

• Gabriel Bercea: have you created a final version of this small class. This is something I'm interested in and would appreciate it if you shared it.

Answer 1

The only thing special about SecureZeroMemory (the Windows version) is that it uses volatile to prevent optimization. Therefore there's no reason to write your own. Just defer to standard algorithms:

std::fill_n((volatile char*)p, n*sizeof(T), 0);

You will probably want to call the global delete:

::operator delete [] p;

Add noexcept:

template <class U> secure_allocator(const secure_allocator<U>&) noexcept {}

• Deduplicator: Will use the algorithm, <string> is probably including it already anyway. Further consideration led me to the conclusion that calling new and delete the way I did is a bug (it should not construct/destroy the elements), and your suggestion doesn't actually correct that. Adding noexcept, though not only there. Also constexpr...

Answer 2

Well, by now I've found some things to correct too:

1. Make everything you can noexcept, so everything (including templates and you yourself) can easier reason about exception-safety and knows which shortcuts are safe: SecureZeroMemory, secure_allocator::secure_allocator<U>, secure_allocator::deallocate, operator==, operator!=

2. Mark all constructors for secure_allocator constexpr to make it a literal type. While at it, do the same for operator== and operator!=.

3. Add secure_allocator::propagate_on_container_move_assignment mirroring std::allocator to have the same noexcept-guarantees. See: Can I force a default special member function to be noexcept?

4. Make all member-functions of secure_allocator static, because none actually needs an instance.

5. Do not use a new-expression in allocate nor a delete-expression in deallocate, in order to avoid constructing respective destroying the (de-)allocated elements there. Delegate to std::allocator<T> instead.

6. Mark SecureZeroMemory inline to avoid problems with the ODR.

7. Don't depend on structural equivalence and reinterpret_cast, use the proper members.
   Thus, the only member which must be specialized is the destructor.

The modified sources after applying all the above can be seen live on coliru.

#include <algorithm>
#include <memory>
#include <string>
namespace my_secure {
inline void SecureZeroMemory(void* p, std::size_t n) noexcept {
 std::fill_n(static_cast<volatile char*>(p), n, 0);
}
// Minimal allocator zeroing on deallocation
template <typename T> struct allocator {
 using value_type = T;
 using propagate_on_container_move_assignment =
 typename std::allocator_traits<std::allocator<T>>
 ::propagate_on_container_move_assignment;
 constexpr allocator() = default;
 constexpr allocator(const allocator&) = default;
 template <class U> constexpr allocator(const allocator<U>&) noexcept {}
 static T* allocate(std::size_t n) { return std::allocator<T>{}.allocate(n); }
 static void deallocate(T* p, std::size_t n) noexcept {
 SecureZeroMemory(p, n * sizeof *p);
 std::allocator<T>{}.deallocate(p, n);
 }
};
template <typename T, typename U>
constexpr bool operator==(allocator<T>, allocator<U>) noexcept { return true; }
template <typename T, typename U>
constexpr bool operator!= (allocator<T>, allocator<U>) noexcept { return false; }
using secure_string = std::basic_string<char, std::char_traits<char>, allocator<char>>;
}
// Zero the strings own memory on destruction
template<> my_secure::secure_string::~basic_string() {
 clear();
 shrink_to_fit();
 ::my_secure::SecureZeroMemory(this, sizeof *this);
}

Answer 3

FYI,

_N4659 24.3.2.1 [string.require]/3 requires that the supplied char_traits character type match the string's character type._

similarly, N4659 26.2.1 say the same about the allocator's value type.

Thus, that last section should read

// Zero the strings own memory on destruction
template<> my_secure::secure_string::~basic_string() {
 using X = basic_string<unsigned char, char_traits<unsigned char>,
 ::my_secure::allocator<unsigned char>>;
 ((X*)this)->~X();
 ::my_secure::SecureZeroMemory(this, sizeof *this);
}

Also, SecureZeroMemory is a macro in the MSVC (windows) runtime, so you should either defer to that, temporarily undefine it, or call your function something else entirely. Unless you don't mind your function being renamed to RtlSecureZeroMemory.

// WinBase.h
#define SecureZeroMemory RtlSecureZeroMemory

Answer 4

1. Turns out this point is invalid. Your SecureZeroMemory is called twice in case of regular allocation. First time, by destructor of your string: ((X*)this)->~X();. Second time, explicitly: my_secure::SecureZeroMemory(this, sizeof *this); As you can guess, this leaves you with undefined behavior.
2. Older (and more common) versions of gcc and clang (4.9 and 3.6, for example) will not compile your code, since older versions of libstdc++ and libc++ don't use std::allocator_traits. You have to explicitly define implementations of optional allocator requirements if you want portability.

• Nikita Kakuev: Oh, you're right. Then my first point is invalid. Sorry for the trouble.