test(secure_buffer): verify wipe and lock ranges

Author: LimiNode

CMakeLists.txt

@@ -129,16 +129,26 @@ if(HMACCPP_BUILD_TESTS)
         DOWNLOAD_EXTRACT_TIMESTAMP TRUE
     )
     FetchContent_MakeAvailable(googletest)
-    find_package(OpenSSL REQUIRED)
-    add_executable(test_all test_all.cpp)
-    target_link_libraries(test_all PRIVATE hmac_cpp gtest_main OpenSSL::Crypto)
-    target_include_directories(test_all PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include)
-    add_test(NAME test_all COMMAND test_all)
+
+    add_executable(test_secure_buffer test_secure_buffer.cpp)
+    target_link_libraries(test_secure_buffer PRIVATE hmac_cpp gtest_main)
+    target_include_directories(test_secure_buffer PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include)
+    add_test(NAME test_secure_buffer COMMAND test_secure_buffer)
 
     add_executable(test_totp test_totp.cpp)
     target_link_libraries(test_totp PRIVATE hmac_cpp gtest_main)
     target_include_directories(test_totp PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include)
     add_test(NAME test_totp COMMAND test_totp)
+
+    find_package(OpenSSL QUIET)
+    if(OpenSSL_FOUND)
+        add_executable(test_all test_all.cpp)
+        target_link_libraries(test_all PRIVATE hmac_cpp gtest_main OpenSSL::Crypto)
+        target_include_directories(test_all PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include)
+        add_test(NAME test_all COMMAND test_all)
+    else()
+        message(STATUS "OpenSSL not found; skipping test_all")
+    endif()
 endif()
 
 export(EXPORT hmac_cppTargets

include/hmac_cpp/secure_buffer.hpp

@@ -1,14 +1,15 @@
 #ifndef HMAC_CPP_SECURE_BUFFER_HPP
 #define HMAC_CPP_SECURE_BUFFER_HPP
 
-#include <atomic>
 #include <algorithm>
+#include <atomic>
 #include <cstddef>
 #include <cstdint>
 #include <cstring>
-#include <vector>
-#include <type_traits>
+#include <memory>
 #include <string>
+#include <type_traits>
+#include <vector>
 #include "hmac_cpp/memlock.hpp"
 
 #if defined(HAVE_EXPLICIT_BZERO)
@@ -28,6 +29,12 @@
 
 namespace hmac_cpp {
 
+/// \brief Default page-locking policy used by secure_buffer.
+struct page_locker {
+    static bool lock(void* ptr, size_t len) noexcept { return lock_pages(ptr, len); }
+    static bool unlock(void* ptr, size_t len) noexcept { return unlock_pages(ptr, len); }
+};
+
 /// \brief Securely zeroes a memory region.
 /// \param ptr Pointer to the memory to wipe.
 /// \param len Number of bytes to set to zero.
@@ -48,81 +55,71 @@ static inline void secure_zero(void* ptr, size_t len) {
 /// \brief Vector-like buffer that zeroizes its contents on destruction.
 /// \tparam T Trivial value type stored in the buffer (defaults to uint8_t).
 /// \tparam LockOnAlloc Lock pages in memory on allocation.
-template<class T = uint8_t, bool LockOnAlloc = false>
+/// \tparam Allocator Allocator used for the backing storage.
+/// \tparam PageLocker Page-locking policy used when LockOnAlloc is true.
+template<class T = uint8_t,
+         bool LockOnAlloc = false,
+         class Allocator = std::allocator<T>,
+         class PageLocker = page_locker>
 struct secure_buffer {
     static_assert(std::is_trivial<T>::value, "secure_buffer requires trivial type");
 
+    using vector_type = std::vector<T, Allocator>;
+
     secure_buffer() {
-        if (LockOnAlloc && !buf.empty()) {
-            locked_ = lock_pages(buf.data(), buf.size() * sizeof(T));
-        }
+        lock_current_range();
     }
 
     /// \brief Construct with n default-initialized elements.
     /// \param n Element count.
     explicit secure_buffer(size_t n) : buf(n) {
-        if (LockOnAlloc && !buf.empty()) {
-            locked_ = lock_pages(buf.data(), buf.size() * sizeof(T));
-        }
+        lock_current_range();
     }
 
     /// \brief Construct from vector, moving its contents.
     /// \param v Source vector.
-    explicit secure_buffer(std::vector<T>&& v) : buf(std::move(v)) {
-        if (LockOnAlloc && !buf.empty()) {
-            locked_ = lock_pages(buf.data(), buf.size() * sizeof(T));
-        }
+    explicit secure_buffer(vector_type&& v) : buf(std::move(v)) {
+        lock_current_range();
     }
 
     /// \brief Construct from std::string rvalue and zeroize the source.
     /// \param s Source string.
     template<class U = T, typename std::enable_if<std::is_same<U, uint8_t>::value, int>::type = 0>
     explicit secure_buffer(std::string&& s) : buf(s.begin(), s.end()) {
-        if (LockOnAlloc && !buf.empty()) {
-            locked_ = lock_pages(buf.data(), buf.size() * sizeof(T));
-        }
+        lock_current_range();
         if (!s.empty()) {
             secure_zero(&s[0], s.size());
             s.clear();
         }
     }
 
     secure_buffer(const secure_buffer& other) : buf(other.buf) {
-        if (LockOnAlloc && !buf.empty()) {
-            locked_ = lock_pages(buf.data(), buf.size() * sizeof(T));
-        }
+        lock_current_range();
     }
 
     secure_buffer& operator=(const secure_buffer& other) {
         if (this != &other) {
-            secure_zero(buf.data(), buf.size() * sizeof(T));
-            if (locked_) {
-                unlock_pages(buf.data(), buf.size() * sizeof(T));
-            }
+            clear();
             buf = other.buf;
-            if (LockOnAlloc && !buf.empty()) {
-                locked_ = lock_pages(buf.data(), buf.size() * sizeof(T));
-            } else {
-                locked_ = false;
-            }
+            lock_current_range();
         }
         return *this;
     }
 
     secure_buffer(secure_buffer&& other) noexcept
-        : buf(std::move(other.buf)), locked_(other.locked_) {
-        other.locked_ = false;
+        : buf(std::move(other.buf)),
+          locked_ptr_(other.locked_ptr_),
+          locked_bytes_(other.locked_bytes_) {
+        other.clear_locked_range();
     }
 
     secure_buffer& operator=(secure_buffer&& other) noexcept {
         if (this != &other) {
-            secure_zero(buf.data(), buf.size() * sizeof(T));
-            if (locked_) {
-                unlock_pages(buf.data(), buf.size() * sizeof(T));
-            }
+            clear();
             buf = std::move(other.buf);
-            locked_ = other.locked_;
-            other.locked_ = false;
+            locked_ptr_ = other.locked_ptr_;
+            locked_bytes_ = other.locked_bytes_;
+            other.clear_locked_range();
         }
         return *this;
     }
@@ -131,17 +128,14 @@ struct secure_buffer {
     ~secure_buffer() noexcept { clear(); }
 
     /// \brief Check whether pages are locked.
-    bool is_locked() const noexcept { return locked_; }
+    bool is_locked() const noexcept { return locked_ptr_ != nullptr; }
 
     /// \brief Clear and deallocate the buffer.
     void clear() noexcept {
         secure_zero(buf.data(), buf.size() * sizeof(T));
-        if (locked_) {
-            unlock_pages(buf.data(), buf.size() * sizeof(T));
-            locked_ = false;
-        }
-        buf.clear();
-        buf.shrink_to_fit();
+        unlock_locked_range();
+        vector_type empty(buf.get_allocator());
+        buf.swap(empty);
     }
 
     /// \brief Resize the buffer, zeroizing data before a reallocation releases it.
@@ -156,35 +150,26 @@ struct secure_buffer {
         }
 
         // std::vector::resize may release the old allocation before we can
-        // wipe it. Allocate the replacement first so the old secret remains
-        // reachable until it has been zeroized and unlocked.
-        std::vector<T> replacement(n);
+        // wipe it. Allocate and lock the replacement first so the old secret
+        // remains reachable until it has been zeroized and unlocked.
+        vector_type replacement(n, T{}, buf.get_allocator());
+        replacement_guard guard(replacement.data(), replacement.size() * sizeof(T));
+        guard.lock();
         std::copy(buf.begin(), buf.end(), replacement.begin());
 
         secure_zero(buf.data(), old_sz * sizeof(T));
-        if (locked_) {
-            unlock_pages(buf.data(), old_sz * sizeof(T));
-            locked_ = false;
-        }
+        unlock_locked_range();
 
         buf.swap(replacement);
-        if (LockOnAlloc && !buf.empty()) {
-            locked_ = lock_pages(buf.data(), buf.size() * sizeof(T));
-        }
+        set_locked_range(buf.data(), buf.size() * sizeof(T), guard.release());
     }
 
     /// \brief Assign from raw pointer.
     void assign(const T* p, size_t n) {
         secure_zero(buf.data(), buf.size() * sizeof(T));
-        if (locked_) {
-            unlock_pages(buf.data(), buf.size() * sizeof(T));
-        }
+        unlock_locked_range();
         buf.assign(p, p + n);
-        if (LockOnAlloc && !buf.empty()) {
-            locked_ = lock_pages(buf.data(), buf.size() * sizeof(T));
-        } else {
-            locked_ = false;
-        }
+        lock_current_range();
     }
 
     /// \brief Assign from std::string rvalue and zeroize the source.
@@ -204,14 +189,73 @@ struct secure_buffer {
     T& operator[](size_t i) { return buf[i]; }
     const T& operator[](size_t i) const { return buf[i]; }
 
-    typename std::vector<T>::iterator begin() { return buf.begin(); }
-    typename std::vector<T>::iterator end() { return buf.end(); }
-    typename std::vector<T>::const_iterator begin() const { return buf.begin(); }
-    typename std::vector<T>::const_iterator end() const { return buf.end(); }
+    typename vector_type::iterator begin() { return buf.begin(); }
+    typename vector_type::iterator end() { return buf.end(); }
+    typename vector_type::const_iterator begin() const { return buf.begin(); }
+    typename vector_type::const_iterator end() const { return buf.end(); }
 
 private:
-    std::vector<T> buf;
-    bool locked_{};
+    struct replacement_guard {
+        replacement_guard(void* ptr, size_t bytes) noexcept : ptr_(ptr), bytes_(bytes) {}
+
+        ~replacement_guard() {
+            if (!active_) {
+                return;
+            }
+            secure_zero(ptr_, bytes_);
+            if (locked_) {
+                PageLocker::unlock(ptr_, bytes_);
+            }
+        }
+
+        void lock() noexcept {
+            if (LockOnAlloc && ptr_ != nullptr && bytes_ != 0) {
+                locked_ = PageLocker::lock(ptr_, bytes_);
+            }
+        }
+
+        bool release() noexcept {
+            active_ = false;
+            return locked_;
+        }
+
+        void* ptr_{};
+        size_t bytes_{};
+        bool locked_{};
+        bool active_{true};
+    };
+
+    void lock_current_range() noexcept {
+        if (LockOnAlloc && !buf.empty()) {
+            set_locked_range(buf.data(), buf.size() * sizeof(T),
+                             PageLocker::lock(buf.data(), buf.size() * sizeof(T)));
+        }
+    }
+
+    void unlock_locked_range() noexcept {
+        if (locked_ptr_ != nullptr) {
+            PageLocker::unlock(locked_ptr_, locked_bytes_);
+        }
+        clear_locked_range();
+    }
+
+    void set_locked_range(void* ptr, size_t bytes, bool locked) noexcept {
+        if (locked) {
+            locked_ptr_ = ptr;
+            locked_bytes_ = bytes;
+        } else {
+            clear_locked_range();
+        }
+    }
+
+    void clear_locked_range() noexcept {
+        locked_ptr_ = nullptr;
+        locked_bytes_ = 0;
+    }
+
+    vector_type buf;
+    void* locked_ptr_{};
+    size_t locked_bytes_{};
 };
 
 } // namespace hmac_cpp

test_all.cpp

@@ -12,7 +12,6 @@
 #include "hmac_cpp/hmac.hpp"
 #include "hmac_cpp/hmac_utils.hpp"
 #include "hmac_cpp/encoding.hpp"
-#include "hmac_cpp/secure_buffer.hpp"
 #include "hmac_cpp/secret_string.hpp"
 
 static std::time_t mock_time_value = 0;
@@ -33,20 +32,6 @@ static std::vector<uint8_t> from_hex(const std::string& hex) {
     return out;
 }
 
-TEST(SecureBufferTest, ResizeGrowthPreservesContents) {
-    hmac_cpp::secure_buffer<uint8_t, true> buffer(8);
-    for (size_t i = 0; i < buffer.size(); ++i) {
-        buffer[i] = static_cast<uint8_t>(i + 1);
-    }
-
-    buffer.resize(4096);
-
-    ASSERT_EQ(buffer.size(), 4096u);
-    for (size_t i = 0; i < 8; ++i) {
-        EXPECT_EQ(buffer[i], static_cast<uint8_t>(i + 1));
-    }
-}
-
 TEST(HashTest, SHA1) {
     EXPECT_EQ(hmac_hash::sha1("grape"),
               "bc8a2f8cdedb005b5c787692853709b060db75ff");