polycratia

Deleting == and << so a secret leak is a build error

· 8 min read

A key sitting in memory leaks three ways, and every one of them compiles without a warning: a comparison that returns as soon as two bytes differ, a print that lands in a log, and the buffer still holding bytes when the function returns. That is why review catches them unevenly. Two of those spellings can be deleted outright. The third one you can answer in a destructor.

The first leak is the one people write about. memcmp(mac, expected, 16) == 0 returns on the first differing byte, so whoever can time the call learns how many leading bytes they guessed right, and then recovers the tag one byte at a time. That is CWE-208, and a constant-time comparison answers it. The other two are duller, and in my experience building custodial wallet services for BTC and ETH and payment flows on card rails, considerably more common. A secret reaches a log because somebody added a debug line during an incident and never took it out. A secret survives a function because memset(key, 0, sizeof key) at the end of a scope is dead-store elimination bait (CWE-14), and at -O2 the compiler can see the buffer is dead afterwards and is entitled to delete the store entirely.

I wrote ctsafe for the first and the third: header-only C++17, no allocation, no exceptions. What I want to dissect here is the smallest type in the header, secret_span, because it is the one that changes what the compiler will accept rather than what the runtime does.

A guard, not a container#

secret_span holds two members and no memory:

cpp
private:
    std::uint8_t* data_ = nullptr;
    std::size_t size_ = 0;

The buffer belongs to whoever declared it. The guard names it, and erases it when the guard leaves scope, not when the buffer does:

cpp
~secret_span() { erase_now(); }

void erase_now() noexcept { erase(data_, size_); }

erase is the part that has to argue with the optimizer, and mostly it declines the argument: it calls the routine the platform already provides for exactly this (SecureZeroMemory, explicit_bzero, memset_s), and only falls back to stores through a volatile pointer when there is none. Every path ends in a compiler barrier so the buffer cannot be treated as dead across the call, and ctsafe::erase_backend_name() reports which row a given build took, because a guarantee you have to guess at is not one.

In use, the guard is three lines and a closing brace:

cpp
{
    ctsafe::secret_span secret(key);      // key is a std::array, vector, or C array

    sign(message, secret.data(), secret.size());
    if (ctsafe::equals(secret, presented)) { /* accept */ }
}   // key is zero here

Two operators that are deleted, not missing#

cpp
friend bool operator==(const secret_span&, const secret_span&) = delete;
friend bool operator!=(const secret_span&, const secret_span&) = delete;

template <typename Stream>
friend Stream& operator<<(Stream&, const secret_span&) = delete;

Deleted and absent are not the same thing here. If operator== were simply never declared, the expression would fail with a message about no viable operator, and someone would helpfully go add one. Declared and deleted, it says what it means: == on secrets is memcmp underneath, and the comparison that reads all the bytes has a different name. ctsafe::equals(secret, presented) is still right there, one identifier away.

The stream overload is the one I like more, because it is deleted for every Stream type at once. It is a friend template taking (Stream&, const secret_span&), found by argument-dependent lookup, and it is an exact match, so it wins against whatever templates the stream library brought along. std::cerr << secret; does not compile. Neither does the same line against a logging sink, a stringstream, or somebody's custom tracer. The accident was never specific to ostream, so the deletion should not be either.

One erase, exactly one owner#

A guard that erases in its destructor has a counting problem. Copy it and the buffer gets cleared twice, which is harmless, except that the type no longer tells you where the erase happens. Let a moved-from guard keep its pointer and you get a use-after-erase on a buffer somebody still holds.

cpp
secret_span(const secret_span&) = delete;
secret_span& operator=(const secret_span&) = delete;

secret_span(secret_span&& other) noexcept : data_(other.data_), size_(other.size_) {
    other.release();
}

secret_span& operator=(secret_span&& other) noexcept {
    if (this != &other) {
        erase_now();
        data_ = other.data_;
        size_ = other.size_;
        other.release();
    }
    return *this;
}

Copying is deleted. The move constructor takes the pointer and the length and calls other.release(), which sets data_ to nullptr and size_ to zero: the source stops naming the bytes and leaves them exactly as they are. Move assignment runs erase_now() on its own bytes first, and only then takes the other's. Skip that and the buffer the left-hand guard was responsible for is quietly abandoned, which is the original bug with extra steps.

The test that pins this down is the one with the interesting middle assertion:

cpp
void moving_a_secret_span_moves_the_duty() {
    std::array<std::uint8_t, 8> key{};
    key.fill(0x33);
    {
        ctsafe::secret_span outer;
        {
            ctsafe::secret_span inner(key);
            outer = std::move(inner);
            CHECK(inner.size() == 0);
            CHECK(inner.data() == nullptr);
        }
        CHECK(!ctsafe::is_zero(key.data(), key.size()));
        CHECK(outer.size() == key.size());
    }
    CHECK(ctsafe::is_zero(key.data(), key.size()));
}

inner dies and the key is still intact. That is the assertion worth writing, because it is the one that fails if the move constructor forgets to release. The buffer is cleared once, where the surviving guard dies.

erase_now() deliberately does not release. After an early erase the span keeps naming the bytes, so the destructor later zeroes a buffer that is already zero. That costs the same and promises no less. The alternative, making an early erase quietly disown the buffer, means the guard's behaviour depends on whether anyone called it, and that is a worse thing to reason about than a redundant store. Handing the duty back is a separate, named operation: release().

Explicit, because of the statement that ends#

cpp
template <typename T, std::size_t N,
          typename = std::enable_if_t<sizeof(T) == 1 && !std::is_const<T>::value>>
explicit secret_span(T (&array)[N]) noexcept
    : data_(reinterpret_cast<std::uint8_t*>(array)), size_(N) {}

template <typename Range,
          typename = std::enable_if_t<detail::is_mutable_byte_range<Range>::value>>
explicit secret_span(Range& range) noexcept
    : data_(reinterpret_cast<std::uint8_t*>(range.data())), size_(range.size()) {}

Both are explicit, and that keyword is doing real work rather than collecting style points. Without it, a function taking const secret_span& would happily accept a caller's std::array, materialise a temporary guard around it, and zero the caller's buffer at the end of the statement that passed it. The caller wrote a function call. What they got was an erase. A guard whose lifetime is a subexpression is not a guard.

The constraints are the other half. The array constructor takes only byte-sized, non-const element types, and the range constructor takes only what detail::is_mutable_byte_range admits, meaning size() and data() over non-const bytes. So a const std::vector or a std::string_view cannot be guarded at all. That looks restrictive until you remember the guard's job is to write zeros: offering to protect bytes you are not allowed to modify would be a promise the destructor cannot keep.

The pointer stays reachable, on purpose#

data() and size() are public, both const and non-const. A secret you cannot read cannot sign anything, verify anything, or be handed to a C API that wants a pointer and a length, which is every crypto library worth using. So memcmp and printf over secret.data() still compile. The guard does not close the hole, it narrows it: two specific accidents become build errors, one specific omission becomes impossible within the scope, and deliberately reaching for the raw pointer stays available because it has to.

Comparison survives the narrowing because of a lookup rather than a conversion operator. byte_view accepts the guard through detail::is_byte_range, so ctsafe::equals(secret, presented) reads every byte of it with both lengths travelling alongside their data. The size is never retyped at the call site, which removes the other classic failure around here: a 16 that used to be right.

Where this stops being enough#

The guard protects a buffer, not a value's history. If the key arrived as a std::string parsed out of a request body, got copied into a vector that reallocated once on the way, and only then got guarded, there are two abandoned copies on the heap that the destructor will never see. The type cannot help with those, and nothing header-only can. The only fix is to decide where in the request path the secret first lands in a buffer you own, and guard it there.

erase_object has a related edge, handled with a static_assert: erasing a polymorphic object by sizeof(T) would destroy its vtable pointer, so the type refuses rather than producing a zeroed object that crashes on the next virtual call.

The version of this I keep being tempted to write is an owning type, a secret buffer that allocates, zeroes and frees. It reads better in a header. In practice I think it is worse: the owning type forces a copy on the way in, the original buffer still needs erasing, so you end up with both problems and one more allocation. A non-owning guard is the honest shape for the thing I actually have, which is a buffer somebody else declared and a scope I want to be sure of.

react

$ new-project --brief

or email hey@polycratia.com