Skip to content

Make secret-key cleanup and replacement exception-safe #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AKG451 wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Make secret-key cleanup and replacement exception-safe #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
873b9f5
Cleanse secret keys before replacement
AKG451 Nov 3, 2026
0e80fcd
Refactor secret key handling in KeyEncapsulation and Signature
AKG451 Nov 6, 2026
fbc376e
Refactor KEM unit tests for better memory handling
AKG451 Nov 6, 2026
783be1b
Refactor signature tests for clarity and efficiency
AKG451 Nov 6, 2026
cfdc4c8
Refactor KEM tests and add unsupported KEM case
AKG451 Nov 6, 2026
4f724c6
Refactor signature tests for clarity and error logging
AKG451 Nov 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
200 changes: 127 additions & 73 deletions include/oqs_cpp.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <cstdlib>
#include <iostream>
#include <memory>
#include <new>
#include <ostream>
#include <stdexcept>
#include <string>
Expand All @@ -35,6 +36,22 @@ extern "C" {
}
} // namespace C

namespace internal {
// Guards temporary secret storage, including partially generated keys.
class SecretKeyGuard {
bytes& key_;

public:
explicit SecretKeyGuard(bytes& key) noexcept : key_{key} {}
SecretKeyGuard(const SecretKeyGuard&) = delete;
SecretKeyGuard& operator=(const SecretKeyGuard&) = delete;
~SecretKeyGuard() noexcept {
if (!key_.empty())
C::OQS_MEM_cleanse(key_.data(), key_.size());
}
};
} // namespace internal

/**
* \class oqs::MechanismNotSupportedError
* \brief Cryptographic scheme not supported
Expand Down Expand Up @@ -164,10 +181,13 @@ class KEMs final : public internal::Singleton<const KEMs> {
* \brief Key encapsulation mechanisms
*/
class KeyEncapsulation {
std::shared_ptr<C::OQS_KEM> kem_{nullptr, [](C::OQS_KEM* p) {
C::OQS_KEM_free(p);
}}; ///< liboqs smart pointer to C::OQS_KEM
bytes secret_key_{}; ///< secret key
std::shared_ptr<C::OQS_KEM> kem_{}; ///< liboqs object

static void cleanse_secret_key(bytes& secret_key) noexcept {
if (!secret_key.empty())
C::OQS_MEM_cleanse(secret_key.data(), secret_key.size());
}

public:
/**
* \brief KEM algorithm details
Expand All @@ -185,6 +205,16 @@ class KeyEncapsulation {

private:
KeyEncapsulationDetails alg_details_{}; ///< KEM algorithm details
// Keep secret storage last: default copying must finish all potentially
// throwing metadata copies before copying any secret bytes.
bytes secret_key_{}; ///< secret key

void swap(KeyEncapsulation& other) noexcept {
kem_.swap(other.kem_);
using std::swap;
swap(alg_details_, other.alg_details_);
secret_key_.swap(other.secret_key_);
}

public:
/**
Expand All @@ -194,7 +224,7 @@ class KeyEncapsulation {
*/
explicit KeyEncapsulation(const std::string& alg_name,
bytes secret_key = {})
: secret_key_{std::move(secret_key)} {
try : secret_key_{} {
// KEM not enabled
if (!KEMs::is_KEM_enabled(alg_name)) {
// Perhaps it's supported
Expand All @@ -206,6 +236,8 @@ class KeyEncapsulation {

kem_.reset(C::OQS_KEM_new(alg_name.c_str()),
[](C::OQS_KEM* p) { C::OQS_KEM_free(p); });
if (!kem_)
throw std::bad_alloc{};

alg_details_.name = kem_->method_name;
alg_details_.version = kem_->alg_version;
Expand All @@ -215,6 +247,11 @@ class KeyEncapsulation {
alg_details_.length_secret_key = kem_->length_secret_key;
alg_details_.length_ciphertext = kem_->length_ciphertext;
alg_details_.length_shared_secret = kem_->length_shared_secret;

secret_key_.swap(secret_key);
} catch (...) {
cleanse_secret_key(secret_key);
throw;
}

/**
Expand All @@ -223,51 +260,47 @@ class KeyEncapsulation {
KeyEncapsulation(const KeyEncapsulation&) = default;

/**
* \brief Default copy assignment operator
* \brief Copy assignment operator
* \return Reference to the current instance
*/
KeyEncapsulation& operator=(const KeyEncapsulation&) = default;
KeyEncapsulation& operator=(const KeyEncapsulation& rhs) {
if (this != &rhs) {
KeyEncapsulation replacement{rhs};
swap(replacement);
// replacement now owns and cleanses our previous secret key.
}
return *this;
}

/**
* \brief Move constructor, guarantees that the rvalue secret key is always
* zeroed
* \param rhs oqs::KeyEncapsulation instance
* \brief Move constructor; transfers key ownership without allocation.
* The source no longer owns any secret bytes after the move.
* \param rhs KeyEncapsulation instance
*/
KeyEncapsulation(KeyEncapsulation&& rhs) noexcept
: kem_{std::move(rhs.kem_)}, alg_details_{std::move(rhs.alg_details_)} {
// Paranoid move via copy/clean/resize, see
// https://stackoverflow.com/questions/55054187/can-i-resize-a-vector-that-was-moved-from
secret_key_ = rhs.secret_key_; // copy
// Clean (zero)
C::OQS_MEM_cleanse(rhs.secret_key_.data(), rhs.secret_key_.size());
rhs.secret_key_.resize(0); // resize
: kem_{std::move(rhs.kem_)},
alg_details_{std::move(rhs.alg_details_)} {
secret_key_.swap(rhs.secret_key_);
}

/**
* \brief Move assignment operator, guarantees that the rvalue secret key is
* always zeroed
* \param rhs oqs::KeyEncapsulation instance
* \brief Move assignment; cleanses the replaced key and transfers ownership.
* \param rhs KeyEncapsulation instance
* \return Reference to the current instance
*/
KeyEncapsulation& operator=(KeyEncapsulation&& rhs) noexcept {
kem_ = std::move(rhs.kem_);
alg_details_ = std::move(rhs.alg_details_);

// Paranoid move via copy/clean/resize, see
// https://stackoverflow.com/questions/55054187/can-i-resize-a-vector-that-was-moved-from
secret_key_ = rhs.secret_key_; // copy
// Clean (zero)
C::OQS_MEM_cleanse(rhs.secret_key_.data(), rhs.secret_key_.size());
rhs.secret_key_.resize(0); // resize

if (this != &rhs) {
KeyEncapsulation replacement{std::move(rhs)};
swap(replacement);
}
return *this;
}

/**
* \brief Virtual default destructor
* \brief Virtual destructor; cleanses the owned secret key
*/
virtual ~KeyEncapsulation() {
if (!secret_key_.empty())
C::OQS_MEM_cleanse(secret_key_.data(), secret_key_.size());
cleanse_secret_key(secret_key_);
}

/**
Expand All @@ -288,13 +321,16 @@ class KeyEncapsulation {
*/
bytes generate_keypair() {
bytes public_key(alg_details_.length_public_key, 0);
secret_key_ = bytes(alg_details_.length_secret_key, 0);
bytes new_secret_key(alg_details_.length_secret_key, 0);
internal::SecretKeyGuard guard{new_secret_key};

OQS_STATUS rv_ = C::OQS_KEM_keypair(kem_.get(), public_key.data(),
secret_key_.data());
OQS_STATUS rv_ = C::OQS_KEM_keypair(
kem_.get(), public_key.data(), new_secret_key.data());
if (rv_ != OQS_STATUS::OQS_SUCCESS)
throw std::runtime_error("Can not generate keypair");

// Commit only after success. The guard then cleanses the old key.
secret_key_.swap(new_secret_key);
return public_key;
}

Expand Down Expand Up @@ -481,10 +517,12 @@ class Sigs final : public internal::Singleton<const Sigs> {
* \brief Signature mechanisms
*/
class Signature {
std::shared_ptr<C::OQS_SIG> sig_{nullptr, [](C::OQS_SIG* p) {
C::OQS_SIG_free(p);
}}; ///< liboqs smart pointer to C::OQS_SIG
bytes secret_key_{}; ///< secret key
std::shared_ptr<C::OQS_SIG> sig_{}; ///< liboqs object

static void cleanse_secret_key(bytes& secret_key) noexcept {
if (!secret_key.empty())
C::OQS_MEM_cleanse(secret_key.data(), secret_key.size());
}

public:
/**
Expand All @@ -503,6 +541,16 @@ class Signature {

private:
SignatureDetails alg_details_{}; ///< Signature algorithm details
// Keep secret storage last: default copying must finish all potentially
// throwing metadata copies before copying any secret bytes.
bytes secret_key_{}; ///< secret key

void swap(Signature& other) noexcept {
sig_.swap(other.sig_);
using std::swap;
swap(alg_details_, other.alg_details_);
secret_key_.swap(other.secret_key_);
}

public:
/**
Expand All @@ -511,7 +559,7 @@ class Signature {
* \param secret_key Secret key (optional)
*/
explicit Signature(const std::string& alg_name, bytes secret_key = {})
: secret_key_{std::move(secret_key)} {
try : secret_key_{} {
// signature not enabled
if (!Sigs::is_sig_enabled(alg_name)) {
// perhaps it's supported
Expand All @@ -523,6 +571,8 @@ class Signature {

sig_.reset(C::OQS_SIG_new(alg_name.c_str()),
[](C::OQS_SIG* p) { C::OQS_SIG_free(p); });
if (!sig_)
throw std::bad_alloc{};

alg_details_.name = sig_->method_name;
alg_details_.version = sig_->alg_version;
Expand All @@ -532,6 +582,11 @@ class Signature {
alg_details_.length_public_key = sig_->length_public_key;
alg_details_.length_secret_key = sig_->length_secret_key;
alg_details_.max_length_signature = sig_->length_signature;

secret_key_.swap(secret_key);
} catch (...) {
cleanse_secret_key(secret_key);
throw;
}

/**
Expand All @@ -540,51 +595,47 @@ class Signature {
Signature(const Signature&) = default;

/**
* \brief Default copy assignment operator
* \brief Copy assignment operator
* \return Reference to the current instance
*/
Signature& operator=(const Signature&) = default;
Signature& operator=(const Signature& rhs) {
if (this != &rhs) {
Signature replacement{rhs};
swap(replacement);
// replacement now owns and cleanses our previous secret key.
}
return *this;
}

/**
* \brief Move constructor, guarantees that the rvalue secret key is always
* zeroed
* \param rhs oqs::Signature instance
* \brief Move constructor; transfers key ownership without allocation.
* The source no longer owns any secret bytes after the move.
* \param rhs Signature instance
*/
Signature(Signature&& rhs) noexcept
: sig_{std::move(rhs.sig_)}, alg_details_{std::move(rhs.alg_details_)} {
// Paranoid move via copy/clean/resize, see
// https://stackoverflow.com/questions/55054187/can-i-resize-a-vector-that-was-moved-from
secret_key_ = rhs.secret_key_; // copy
// Clean (zero)
C::OQS_MEM_cleanse(rhs.secret_key_.data(), rhs.secret_key_.size());
rhs.secret_key_.resize(0); // resize
: sig_{std::move(rhs.sig_)},
alg_details_{std::move(rhs.alg_details_)} {
secret_key_.swap(rhs.secret_key_);
}

/**
* \brief Move assignment operator, guarantees that the rvalue secret key is
* always zeroed
* \param rhs oqs::Signature instance
* \brief Move assignment; cleanses the replaced key and transfers ownership.
* \param rhs Signature instance
* \return Reference to the current instance
*/
Signature& operator=(Signature&& rhs) noexcept {
sig_ = std::move(rhs.sig_);
alg_details_ = std::move(rhs.alg_details_);

// Paranoid move via copy/clean/resize, see
// https://stackoverflow.com/questions/55054187/can-i-resize-a-vector-that-was-moved-from
secret_key_ = rhs.secret_key_; // copy
// Clean (zero)
C::OQS_MEM_cleanse(rhs.secret_key_.data(), rhs.secret_key_.size());
rhs.secret_key_.resize(0); // resize

if (this != &rhs) {
Signature replacement{std::move(rhs)};
swap(replacement);
}
return *this;
}

/**
* \brief Virtual default destructor
* \brief Virtual destructor; cleanses the owned secret key
*/
virtual ~Signature() {
if (!secret_key_.empty())
C::OQS_MEM_cleanse(secret_key_.data(), secret_key_.size());
cleanse_secret_key(secret_key_);
}

/**
Expand All @@ -604,14 +655,17 @@ class Signature {
* \return Public key
*/
bytes generate_keypair() {
bytes public_key(get_details().length_public_key, 0);
secret_key_ = bytes(alg_details_.length_secret_key, 0);
bytes public_key(alg_details_.length_public_key, 0);
bytes new_secret_key(alg_details_.length_secret_key, 0);
internal::SecretKeyGuard guard{new_secret_key};

OQS_STATUS rv_ = C::OQS_SIG_keypair(sig_.get(), public_key.data(),
secret_key_.data());
OQS_STATUS rv_ = C::OQS_SIG_keypair(
sig_.get(), public_key.data(), new_secret_key.data());
if (rv_ != OQS_STATUS::OQS_SUCCESS)
throw std::runtime_error("Can not generate keypair");

// Commit only after success. The guard then cleanses the old key.
secret_key_.swap(new_secret_key);
return public_key;
}

Expand Down
Loading
Loading