From dcbcd73fe640a8dfcb6ff0307bb4454fda530996 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Sun, 26 Apr 2026 22:02:46 -0400 Subject: [PATCH 1/4] feat(security): add validateUInt helper, apply to Argon2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `static_cast(NaN | +/-Infinity | -1)` is undefined behavior in C++. The audit found ~20 sites doing the cast naked at the JS↔C++ boundary; HybridArgon2.cpp:50 was the most prominent (parallelism, memory, passes, version, tagLength all unchecked). Add a templated `validateUInt` helper to QuickCryptoUtils.hpp that rejects NaN, +/-Infinity, negative values, fractional values, and out-of-range inputs BEFORE the cast and throws std::runtime_error with the parameter name on failure. Apply it as the reference site to HybridArgon2's hashImpl. The remaining ~19 sites (PBKDF2, Scrypt, HKDF, Hash, HMAC, KMAC, BLAKE3, RSA, ML-DSA, AES-CCM) get swept in the Phase 2 / Phase 3 follow-ups that consume this helper. Adds 7 regression tests covering NaN, +/-Infinity, negative, fractional, out-of-range parameters on the sync API and one async path proving the validation also rejects via the Promise rejection. Phase 1.1 of plans/todo/security-audit.md. --- example/src/tests/argon2/argon2_tests.ts | 66 +++++++++++++++++++ .../cpp/argon2/HybridArgon2.cpp | 13 +++- .../cpp/utils/QuickCryptoUtils.hpp | 35 ++++++++++ 3 files changed, 111 insertions(+), 3 deletions(-) diff --git a/example/src/tests/argon2/argon2_tests.ts b/example/src/tests/argon2/argon2_tests.ts index 0f59a730..cece1b98 100644 --- a/example/src/tests/argon2/argon2_tests.ts +++ b/example/src/tests/argon2/argon2_tests.ts @@ -136,3 +136,69 @@ test(SUITE, 'argon2Sync: deterministic with same inputs', () => { Buffer.from(r2).toString('hex'), ); }); + +// --- Numeric parameter validation (Phase 1.1: validateUInt) --- +// +// `static_cast(NaN | +/-Infinity | -1)` is undefined behavior in +// C++. The C++ layer used to do these casts naked; the validateUInt helper +// now rejects them with a descriptive error before the cast. + +const baseParams = { + message: Buffer.from('password'), + nonce: Buffer.from('somesalt0000'), + parallelism: 1, + tagLength: 32, + memory: 64, + passes: 3, +}; + +test(SUITE, 'argon2Sync: rejects NaN parallelism', () => { + assert.throws(() => { + argon2Sync('argon2id', { ...baseParams, parallelism: NaN }); + }, /parallelism.*NaN/i); +}); + +test(SUITE, 'argon2Sync: rejects +Infinity memory', () => { + assert.throws(() => { + argon2Sync('argon2id', { ...baseParams, memory: Infinity }); + }, /memory.*infinity/i); +}); + +test(SUITE, 'argon2Sync: rejects -Infinity passes', () => { + assert.throws(() => { + argon2Sync('argon2id', { ...baseParams, passes: -Infinity }); + }, /passes.*infinity/i); +}); + +test(SUITE, 'argon2Sync: rejects negative tagLength', () => { + assert.throws(() => { + argon2Sync('argon2id', { ...baseParams, tagLength: -1 }); + }, /tagLength.*non-negative/i); +}); + +test(SUITE, 'argon2Sync: rejects fractional passes', () => { + assert.throws(() => { + argon2Sync('argon2id', { ...baseParams, passes: 3.5 }); + }, /passes.*integer/i); +}); + +test(SUITE, 'argon2Sync: rejects out-of-range memory', () => { + // memory is uint32_t — anything beyond UINT32_MAX must be rejected. + assert.throws(() => { + argon2Sync('argon2id', { ...baseParams, memory: 2 ** 32 }); + }, /memory.*out of range/i); +}); + +test(SUITE, 'argon2: async path also rejects NaN parallelism', () => { + return new Promise((resolve, reject) => { + argon2('argon2id', { ...baseParams, parallelism: NaN }, err => { + try { + assert.isNotNull(err); + assert.match(err!.message, /parallelism.*NaN/i); + resolve(); + } catch (e) { + reject(e); + } + }); + }); +}); diff --git a/packages/react-native-quick-crypto/cpp/argon2/HybridArgon2.cpp b/packages/react-native-quick-crypto/cpp/argon2/HybridArgon2.cpp index 1a8f3a33..2163d7b8 100644 --- a/packages/react-native-quick-crypto/cpp/argon2/HybridArgon2.cpp +++ b/packages/react-native-quick-crypto/cpp/argon2/HybridArgon2.cpp @@ -30,6 +30,15 @@ static std::shared_ptr hashImpl(const std::string& algorithm, const auto type = parseAlgorithm(algorithm); + // Validate every numeric parameter before the cast. The previous code did + // `static_cast(parallelism)` etc. naked, which is undefined + // behavior for NaN, +/-Infinity, or negative input — see audit Phase 1.1. + uint32_t parallelismU = validateUInt(parallelism, "Argon2 parallelism"); + size_t tagLengthU = validateUInt(tagLength, "Argon2 tagLength"); + uint32_t memoryU = validateUInt(memory, "Argon2 memory"); + uint32_t passesU = validateUInt(passes, "Argon2 passes"); + uint32_t versionU = validateUInt(version, "Argon2 version"); + ncrypto::Buffer passBuf{message->size() > 0 ? reinterpret_cast(message->data()) : "", message->size()}; ncrypto::Buffer saltBuf{nonce->size() > 0 ? reinterpret_cast(nonce->data()) @@ -46,9 +55,7 @@ static std::shared_ptr hashImpl(const std::string& algorithm, const adBuf = {reinterpret_cast(associatedData.value()->data()), associatedData.value()->size()}; } - auto result = - ncrypto::argon2(passBuf, saltBuf, static_cast(parallelism), static_cast(tagLength), static_cast(memory), - static_cast(passes), static_cast(version), secretBuf, adBuf, type); + auto result = ncrypto::argon2(passBuf, saltBuf, parallelismU, tagLengthU, memoryU, passesU, versionU, secretBuf, adBuf, type); if (!result) { unsigned long err = ERR_peek_last_error(); diff --git a/packages/react-native-quick-crypto/cpp/utils/QuickCryptoUtils.hpp b/packages/react-native-quick-crypto/cpp/utils/QuickCryptoUtils.hpp index 998edc3f..b8b2edef 100644 --- a/packages/react-native-quick-crypto/cpp/utils/QuickCryptoUtils.hpp +++ b/packages/react-native-quick-crypto/cpp/utils/QuickCryptoUtils.hpp @@ -2,11 +2,13 @@ #include #include +#include #include #include #include #include #include +#include #include #include "Macros.hpp" @@ -68,6 +70,39 @@ inline bool CheckIsInt32(double value) { return (value >= std::numeric_limits::lowest() && value <= std::numeric_limits::max()); } +// Validate a JS-side `double` intended to be an unsigned integer in +// [minValue, maxValue], then cast it to T. Rejects NaN, +/-Infinity, negative +// values, and fractional values BEFORE the cast — `static_cast(NaN)` +// and friends are undefined behavior in C++, and the audit found ~20 sites +// that did the cast naked. Throws `std::runtime_error` carrying `paramName` +// on any failure so JS callers see a descriptive, actionable message. +// +// The helper is templated so callers pick the destination type +// (uint32_t, uint64_t, size_t, ...). T must be an unsigned integer type. +template +T validateUInt(double value, const char* paramName, T minValue = 0, T maxValue = std::numeric_limits::max()) { + static_assert(std::is_integral_v && std::is_unsigned_v, "validateUInt: T must be an unsigned integer type"); + + if (std::isnan(value)) { + throw std::runtime_error(std::string(paramName) + " must be a finite number, got NaN"); + } + if (std::isinf(value)) { + throw std::runtime_error(std::string(paramName) + std::string(" must be a finite number, got ") + + (value > 0 ? "+Infinity" : "-Infinity")); + } + if (value < 0) { + throw std::runtime_error(std::string(paramName) + " must be non-negative, got " + std::to_string(value)); + } + if (value != std::floor(value)) { + throw std::runtime_error(std::string(paramName) + " must be an integer, got " + std::to_string(value)); + } + if (value < static_cast(minValue) || value > static_cast(maxValue)) { + throw std::runtime_error(std::string(paramName) + " out of range [" + std::to_string(minValue) + ", " + std::to_string(maxValue) + + "], got " + std::to_string(value)); + } + return static_cast(value); +} + // Function to convert a string to lowercase inline std::string toLower(std::string s) { std::transform(s.begin(), s.end(), s.begin(), [](unsigned char c) { return static_cast(std::tolower(c)); }); From 2fae6bd7a3bc6d81d10f48529f90a15ceb7c61a2 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Sun, 26 Apr 2026 22:13:02 -0400 Subject: [PATCH 2/4] feat(security): add secureZero helper, apply to XSalsa20 destructors `std::memset` is the wrong tool for wiping secret memory: under -O2 the compiler will see that the writes are dead (the buffer is freed or leaving scope right after) and elide them, leaving keys / nonces / plaintext sitting on the heap. The audit found ~30 sites that need a guaranteed-not-elided zeroize. Add a `secureZero()` helper to QuickCryptoUtils.hpp that wraps `OPENSSL_cleanse` (designed for exactly this purpose, available unconditionally on OpenSSL 3.x). Provide overloads for the common shapes: raw pointer + size, std::vector, std::string, fixed-size uint8_t arrays. Apply to two reference sites: - XSalsa20Cipher destructor: previously did NOT zero key/nonce at all (audit HIGH finding), and Phase 0.2 added a leftover_keystream buffer that also needs wiping. - XSalsa20Poly1305Cipher destructor: had a `#ifdef BLSALLOC_SODIUM` fork that fell through to plain `std::memset` when sodium was off (audit MEDIUM finding); collapse both branches to secureZero. The remaining ~28 sites (XChaCha20-Poly1305, all KDFs derived bits, DH/ECDH shared secrets, RSA/EC/Ed/DSA DER private-key strings) get swept in Phase 2 alongside the unique_ptr replacements. Phase 1.2 of plans/todo/security-audit.md. --- .../cpp/cipher/XSalsa20Cipher.hpp | 7 ++++ .../cpp/cipher/XSalsa20Poly1305Cipher.cpp | 19 ++++------ .../cpp/utils/QuickCryptoUtils.hpp | 37 +++++++++++++++++++ 3 files changed, 51 insertions(+), 12 deletions(-) diff --git a/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Cipher.hpp b/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Cipher.hpp index 098a5f96..d39fdf06 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Cipher.hpp +++ b/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Cipher.hpp @@ -13,6 +13,7 @@ #include "HybridCipher.hpp" #include "NitroModules/ArrayBuffer.hpp" +#include "QuickCryptoUtils.hpp" namespace margelo::nitro::crypto { @@ -20,6 +21,12 @@ class XSalsa20Cipher : public HybridCipher { public: XSalsa20Cipher() : HybridObject(TAG) {} ~XSalsa20Cipher() { + // Wipe key material and any cached keystream bytes before the heap is + // returned. Without this the secret-bearing bytes persist on the heap + // until overwritten — see audit HIGH finding (XSalsa20Cipher.hpp:19-22). + secureZero(key); + secureZero(nonce); + secureZero(leftover_keystream); // Let parent destructor free the context ctx = nullptr; } diff --git a/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Poly1305Cipher.cpp b/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Poly1305Cipher.cpp index c5bad00a..da6b5f69 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Poly1305Cipher.cpp +++ b/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Poly1305Cipher.cpp @@ -9,18 +9,13 @@ namespace margelo::nitro::crypto { XSalsa20Poly1305Cipher::~XSalsa20Poly1305Cipher() { -#ifdef BLSALLOC_SODIUM - sodium_memzero(key_, kKeySize); - sodium_memzero(nonce_, kNonceSize); - sodium_memzero(auth_tag_, kTagSize); - if (!data_buffer_.empty()) { - sodium_memzero(data_buffer_.data(), data_buffer_.size()); - } -#else - std::memset(key_, 0, kKeySize); - std::memset(nonce_, 0, kNonceSize); - std::memset(auth_tag_, 0, kTagSize); -#endif + // Always wipe via OPENSSL_cleanse (even when libsodium is enabled) so the + // non-sodium `std::memset` fallback can't be optimized away by the + // compiler. Audit MEDIUM finding (XSalsa20Poly1305Cipher.cpp:20-22). + secureZero(key_); + secureZero(nonce_); + secureZero(auth_tag_); + secureZero(data_buffer_); data_buffer_.clear(); } diff --git a/packages/react-native-quick-crypto/cpp/utils/QuickCryptoUtils.hpp b/packages/react-native-quick-crypto/cpp/utils/QuickCryptoUtils.hpp index b8b2edef..1adc15eb 100644 --- a/packages/react-native-quick-crypto/cpp/utils/QuickCryptoUtils.hpp +++ b/packages/react-native-quick-crypto/cpp/utils/QuickCryptoUtils.hpp @@ -3,8 +3,11 @@ #include #include #include +#include +#include #include #include +#include #include #include #include @@ -103,6 +106,40 @@ T validateUInt(double value, const char* paramName, T minValue = 0, T maxValue = return static_cast(value); } +// Securely zero a memory range using OPENSSL_cleanse, which the compiler is +// guaranteed not to optimize away even when the buffer is about to leave +// scope. Use this for any memory that held secrets — keys, derived bits, +// shared secrets, plaintext, PEM/DER private-key strings, IV/nonce material. +// +// Plain std::memset is unsafe for this purpose: under -O2 the compiler will +// see that the memset writes are dead (the memory is freed or going out of +// scope right after) and elide them, leaving the secret on the heap. +// +// Overloads cover the common shapes: raw pointer + size, vector, string, +// fixed-size array. The audit found ~30 sites that need this — XSalsa20, +// XChaCha20-Poly1305, all KDFs, DH/ECDH shared secrets, RSA/EC/Ed/DSA DER +// private-key strings — and they get swept in Phase 2. +inline void secureZero(void* ptr, std::size_t size) { + if (ptr != nullptr && size > 0) { + OPENSSL_cleanse(ptr, size); + } +} + +inline void secureZero(std::vector& vec) { + secureZero(vec.data(), vec.size()); +} + +inline void secureZero(std::string& s) { + if (!s.empty()) { + secureZero(s.data(), s.size()); + } +} + +template +inline void secureZero(uint8_t (&arr)[N]) { + secureZero(static_cast(arr), N); +} + // Function to convert a string to lowercase inline std::string toLower(std::string s) { std::transform(s.begin(), s.end(), s.begin(), [](unsigned char c) { return static_cast(std::tolower(c)); }); From 73ea9f336dd8122c08b4d75a9af6efa69e883099 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Sun, 26 Apr 2026 22:20:10 -0400 Subject: [PATCH 3/4] feat(security): own EVP_CIPHER_CTX via unique_ptr in HybridCipher base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three subclasses (CCMCipher, ChaCha20Cipher, ChaCha20Poly1305Cipher) implemented destructors as `~SubClass() { ctx = nullptr; }` — they nulled the parent's raw pointer without freeing it first, expecting the parent destructor to do the cleanup. The parent then saw `ctx == nullptr` and skipped the EVP_CIPHER_CTX_free call entirely. Result: every cipher object of those three types leaked its OpenSSL context. Move ownership into the base class with a unique_ptr that has the EVP_CIPHER_CTX_free deleter baked in. Subclasses can no longer mismanage ctx because the destruction order (subclass → base) makes the parent's unique_ptr destructor run automatically and free the context exactly once. The buggy three subclass destructors are removed (defaulted), and XSalsa20Cipher's destructor drops its now- redundant `ctx = nullptr` line while keeping the secureZero calls from Phase 1.2. Internally every `EVP_*Foo*(ctx, ...)` OpenSSL call now uses `ctx.get()`; every `ctx = EVP_CIPHER_CTX_new()` becomes `ctx.reset(EVP_CIPHER_CTX_new())`; every manual cleanup of `EVP_CIPHER_CTX_free(ctx); ctx = nullptr;` collapses to `ctx.reset()`. Touched files: HybridCipher, CCMCipher, ChaCha20Cipher, ChaCha20Poly1305Cipher, GCMCipher, OCBCipher, XSalsa20Cipher. No new tests — the existing AEAD round-trip tests exercise both the constructor (ctx.reset(EVP_CIPHER_CTX_new())) and the destructor (unique_ptr cleanup) in every test that uses these ciphers. Phase 1.3 of plans/todo/security-audit.md. --- .../cpp/cipher/CCMCipher.cpp | 20 +++---- .../cpp/cipher/CCMCipher.hpp | 6 +- .../cpp/cipher/ChaCha20Cipher.cpp | 21 +++---- .../cpp/cipher/ChaCha20Cipher.hpp | 6 +- .../cpp/cipher/ChaCha20Poly1305Cipher.cpp | 29 ++++------ .../cpp/cipher/ChaCha20Poly1305Cipher.hpp | 6 +- .../cpp/cipher/GCMCipher.cpp | 24 +++----- .../cpp/cipher/HybridCipher.cpp | 58 ++++++++----------- .../cpp/cipher/HybridCipher.hpp | 12 +++- .../cpp/cipher/OCBCipher.cpp | 6 +- .../cpp/cipher/XSalsa20Cipher.hpp | 5 +- 11 files changed, 86 insertions(+), 107 deletions(-) diff --git a/packages/react-native-quick-crypto/cpp/cipher/CCMCipher.cpp b/packages/react-native-quick-crypto/cpp/cipher/CCMCipher.cpp index 8b0491ff..ec54cbbf 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/CCMCipher.cpp +++ b/packages/react-native-quick-crypto/cpp/cipher/CCMCipher.cpp @@ -22,7 +22,7 @@ void CCMCipher::init(const std::shared_ptr cipher_key, const std::s size_t iv_len = native_iv->size(); // Set the IV length using CCM-specific control - if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_CCM_SET_IVLEN, iv_len, nullptr) != 1) { + if (EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_CCM_SET_IVLEN, iv_len, nullptr) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); @@ -31,7 +31,7 @@ void CCMCipher::init(const std::shared_ptr cipher_key, const std::s // Set the expected/output tag length using CCM-specific control. // auth_tag_len should have been defaulted or set via setArgs in the base init. - if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_CCM_SET_TAG, auth_tag_len, nullptr) != 1) { + if (EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_CCM_SET_TAG, auth_tag_len, nullptr) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); @@ -44,7 +44,7 @@ void CCMCipher::init(const std::shared_ptr cipher_key, const std::s const unsigned char* iv_ptr = reinterpret_cast(native_iv->data()); // The last argument (is_cipher) should be consistent with the initial setup call. - if (EVP_CipherInit_ex(ctx, nullptr, nullptr, key_ptr, iv_ptr, is_cipher) != 1) { + if (EVP_CipherInit_ex(ctx.get(), nullptr, nullptr, key_ptr, iv_ptr, is_cipher) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); @@ -66,7 +66,7 @@ std::shared_ptr CCMCipher::update(const std::shared_ptr CCMCipher::update(const std::shared_ptr(native_data->data()); int actual_out_len = 0; - int ret = EVP_CipherUpdate(ctx, out_buf.get(), &actual_out_len, in, in_len); + int ret = EVP_CipherUpdate(ctx.get(), out_buf.get(), &actual_out_len, in, in_len); if (!is_cipher) { // Decryption: Check for tag verification failure @@ -115,14 +115,14 @@ std::shared_ptr CCMCipher::final() { } // Proceed only for encryption - int block_size = EVP_CIPHER_CTX_block_size(ctx); + int block_size = EVP_CIPHER_CTX_block_size(ctx.get()); if (block_size <= 0) { throw std::runtime_error("Invalid block size"); } auto out_buf = std::make_unique(block_size); int out_len = 0; - if (!EVP_CipherFinal_ex(ctx, out_buf.get(), &out_len)) { + if (!EVP_CipherFinal_ex(ctx.get(), out_buf.get(), &out_len)) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); @@ -133,7 +133,7 @@ std::shared_ptr CCMCipher::final() { auth_tag_len = sizeof(auth_tag); } - if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_CCM_GET_TAG, auth_tag_len, auth_tag) != 1) { + if (EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_CCM_GET_TAG, auth_tag_len, auth_tag) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); @@ -179,7 +179,7 @@ bool CCMCipher::setAAD(const std::shared_ptr& data, std::optional 0; if (should_set_total_length) { - if (EVP_CipherUpdate(ctx, nullptr, &out_len, nullptr, data_len) != 1) { + if (EVP_CipherUpdate(ctx.get(), nullptr, &out_len, nullptr, data_len) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); @@ -190,7 +190,7 @@ bool CCMCipher::setAAD(const std::shared_ptr& data, std::optionaldata(), aad_len) != 1) { + if (EVP_CipherUpdate(ctx.get(), nullptr, &out_len, native_aad->data(), aad_len) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); diff --git a/packages/react-native-quick-crypto/cpp/cipher/CCMCipher.hpp b/packages/react-native-quick-crypto/cpp/cipher/CCMCipher.hpp index e084544e..1a34697a 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/CCMCipher.hpp +++ b/packages/react-native-quick-crypto/cpp/cipher/CCMCipher.hpp @@ -7,10 +7,8 @@ namespace margelo::nitro::crypto { class CCMCipher : public HybridCipher { public: CCMCipher() : HybridObject(TAG) {} - ~CCMCipher() { - // Let parent destructor free the context - ctx = nullptr; - } + // Destructor defaulted: HybridCipher's unique_ptr ctx frees itself. + ~CCMCipher() override = default; void init(const std::shared_ptr cipher_key, const std::shared_ptr iv) override; std::shared_ptr update(const std::shared_ptr& data) override; diff --git a/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Cipher.cpp b/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Cipher.cpp index 5661856a..fc3b04ac 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Cipher.cpp +++ b/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Cipher.cpp @@ -7,11 +7,8 @@ namespace margelo::nitro::crypto { void ChaCha20Cipher::init(const std::shared_ptr cipher_key, const std::shared_ptr iv) { - // Clean up any existing context - if (ctx) { - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; - } + // Resetting the unique_ptr frees any previous context. + ctx.reset(); // Get ChaCha20 cipher implementation const EVP_CIPHER* cipher = EVP_chacha20(); @@ -20,18 +17,17 @@ void ChaCha20Cipher::init(const std::shared_ptr cipher_key, const s } // Create a new context - ctx = EVP_CIPHER_CTX_new(); + ctx.reset(EVP_CIPHER_CTX_new()); if (!ctx) { throw std::runtime_error("Failed to create cipher context"); } // Initialize the encryption/decryption operation - if (EVP_CipherInit_ex(ctx, cipher, nullptr, nullptr, nullptr, is_cipher) != 1) { + if (EVP_CipherInit_ex(ctx.get(), cipher, nullptr, nullptr, nullptr, is_cipher) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; + ctx.reset(); throw std::runtime_error("ChaCha20Cipher: Failed initial CipherInit setup: " + std::string(err_buf)); } @@ -52,12 +48,11 @@ void ChaCha20Cipher::init(const std::shared_ptr cipher_key, const s const unsigned char* key_ptr = reinterpret_cast(native_key->data()); const unsigned char* iv_ptr = reinterpret_cast(native_iv->data()); - if (EVP_CipherInit_ex(ctx, nullptr, nullptr, key_ptr, iv_ptr, is_cipher) != 1) { + if (EVP_CipherInit_ex(ctx.get(), nullptr, nullptr, key_ptr, iv_ptr, is_cipher) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; + ctx.reset(); throw std::runtime_error("ChaCha20Cipher: Failed to set key/IV: " + std::string(err_buf)); } } @@ -76,7 +71,7 @@ std::shared_ptr ChaCha20Cipher::update(const std::shared_ptrdata(), in_len) != 1) { + if (EVP_CipherUpdate(ctx.get(), out, &out_len, native_data->data(), in_len) != 1) { delete[] out; unsigned long err = ERR_get_error(); char err_buf[256]; diff --git a/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Cipher.hpp b/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Cipher.hpp index 6819b5b8..92b7e75a 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Cipher.hpp +++ b/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Cipher.hpp @@ -7,10 +7,8 @@ namespace margelo::nitro::crypto { class ChaCha20Cipher : public HybridCipher { public: ChaCha20Cipher() : HybridObject(TAG) {} - ~ChaCha20Cipher() { - // Let parent destructor free the context - ctx = nullptr; - } + // Destructor defaulted: HybridCipher's unique_ptr ctx frees itself. + ~ChaCha20Cipher() override = default; void init(const std::shared_ptr cipher_key, const std::shared_ptr iv) override; std::shared_ptr update(const std::shared_ptr& data) override; diff --git a/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Poly1305Cipher.cpp b/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Poly1305Cipher.cpp index 08a2e39a..ea24ba4e 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Poly1305Cipher.cpp +++ b/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Poly1305Cipher.cpp @@ -7,11 +7,8 @@ namespace margelo::nitro::crypto { void ChaCha20Poly1305Cipher::init(const std::shared_ptr cipher_key, const std::shared_ptr iv) { - // Clean up any existing context - if (ctx) { - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; - } + // Resetting the unique_ptr frees any previous context. + ctx.reset(); // Get ChaCha20-Poly1305 cipher implementation const EVP_CIPHER* cipher = EVP_chacha20_poly1305(); @@ -20,18 +17,17 @@ void ChaCha20Poly1305Cipher::init(const std::shared_ptr cipher_key, } // Create a new context - ctx = EVP_CIPHER_CTX_new(); + ctx.reset(EVP_CIPHER_CTX_new()); if (!ctx) { throw std::runtime_error("Failed to create cipher context"); } // Initialize the encryption/decryption operation - if (EVP_CipherInit_ex(ctx, cipher, nullptr, nullptr, nullptr, is_cipher) != 1) { + if (EVP_CipherInit_ex(ctx.get(), cipher, nullptr, nullptr, nullptr, is_cipher) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; + ctx.reset(); throw std::runtime_error("ChaCha20Poly1305Cipher: Failed initial CipherInit setup: " + std::string(err_buf)); } @@ -52,12 +48,11 @@ void ChaCha20Poly1305Cipher::init(const std::shared_ptr cipher_key, const unsigned char* key_ptr = reinterpret_cast(native_key->data()); const unsigned char* iv_ptr = reinterpret_cast(native_iv->data()); - if (EVP_CipherInit_ex(ctx, nullptr, nullptr, key_ptr, iv_ptr, is_cipher) != 1) { + if (EVP_CipherInit_ex(ctx.get(), nullptr, nullptr, key_ptr, iv_ptr, is_cipher) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; + ctx.reset(); throw std::runtime_error("ChaCha20Poly1305Cipher: Failed to set key/IV: " + std::string(err_buf)); } is_finalized = false; @@ -77,7 +72,7 @@ std::shared_ptr ChaCha20Poly1305Cipher::update(const std::shared_pt uint8_t* out = new uint8_t[out_len]; // Perform the cipher update operation - if (EVP_CipherUpdate(ctx, out, &out_len, native_data->data(), in_len) != 1) { + if (EVP_CipherUpdate(ctx.get(), out, &out_len, native_data->data(), in_len) != 1) { delete[] out; unsigned long err = ERR_get_error(); char err_buf[256]; @@ -97,7 +92,7 @@ std::shared_ptr ChaCha20Poly1305Cipher::final() { int out_len = 0; unsigned char* out = new unsigned char[0]; - if (EVP_CipherFinal_ex(ctx, out, &out_len) != 1) { + if (EVP_CipherFinal_ex(ctx.get(), out, &out_len) != 1) { delete[] out; unsigned long err = ERR_get_error(); char err_buf[256]; @@ -116,7 +111,7 @@ bool ChaCha20Poly1305Cipher::setAAD(const std::shared_ptr& data, st // Set AAD data int out_len = 0; - if (EVP_CipherUpdate(ctx, nullptr, &out_len, native_aad->data(), aad_len) != 1) { + if (EVP_CipherUpdate(ctx.get(), nullptr, &out_len, native_aad->data(), aad_len) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); @@ -136,7 +131,7 @@ std::shared_ptr ChaCha20Poly1305Cipher::getAuthTag() { // Get the authentication tag auto tag_buf = std::make_unique(kTagSize); - if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_GET_TAG, kTagSize, tag_buf.get()) != 1) { + if (EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_AEAD_GET_TAG, kTagSize, tag_buf.get()) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); @@ -158,7 +153,7 @@ bool ChaCha20Poly1305Cipher::setAuthTag(const std::shared_ptr& tag) throw std::runtime_error("ChaCha20-Poly1305 tag must be 16 bytes"); } - if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, kTagSize, native_tag->data()) != 1) { + if (EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_AEAD_SET_TAG, kTagSize, native_tag->data()) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); diff --git a/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Poly1305Cipher.hpp b/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Poly1305Cipher.hpp index 41633716..853dc184 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Poly1305Cipher.hpp +++ b/packages/react-native-quick-crypto/cpp/cipher/ChaCha20Poly1305Cipher.hpp @@ -7,10 +7,8 @@ namespace margelo::nitro::crypto { class ChaCha20Poly1305Cipher : public HybridCipher { public: ChaCha20Poly1305Cipher() : HybridObject(TAG) {} - ~ChaCha20Poly1305Cipher() { - // Let parent destructor free the context - ctx = nullptr; - } + // Destructor defaulted: HybridCipher's unique_ptr ctx frees itself. + ~ChaCha20Poly1305Cipher() override = default; void init(const std::shared_ptr cipher_key, const std::shared_ptr iv) override; std::shared_ptr update(const std::shared_ptr& data) override; diff --git a/packages/react-native-quick-crypto/cpp/cipher/GCMCipher.cpp b/packages/react-native-quick-crypto/cpp/cipher/GCMCipher.cpp index 5d15db56..dc632a47 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/GCMCipher.cpp +++ b/packages/react-native-quick-crypto/cpp/cipher/GCMCipher.cpp @@ -7,11 +7,8 @@ namespace margelo::nitro::crypto { void GCMCipher::init(const std::shared_ptr cipher_key, const std::shared_ptr iv) { - // Clean up any existing context - if (ctx) { - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; - } + // Resetting the unique_ptr frees any previous context. + ctx.reset(); // 1. Get cipher implementation by name const EVP_CIPHER* cipher = EVP_get_cipherbyname(cipher_type.c_str()); @@ -20,18 +17,17 @@ void GCMCipher::init(const std::shared_ptr cipher_key, const std::s } // 2. Create a new context - ctx = EVP_CIPHER_CTX_new(); + ctx.reset(EVP_CIPHER_CTX_new()); if (!ctx) { throw std::runtime_error("Failed to create cipher context"); } // 3. Initialize with cipher type only (no key/IV yet) - if (EVP_CipherInit_ex(ctx, cipher, nullptr, nullptr, nullptr, is_cipher) != 1) { + if (EVP_CipherInit_ex(ctx.get(), cipher, nullptr, nullptr, nullptr, is_cipher) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; + ctx.reset(); throw std::runtime_error("GCMCipher: Failed initial CipherInit setup: " + std::string(err_buf)); } @@ -40,12 +36,11 @@ void GCMCipher::init(const std::shared_ptr cipher_key, const std::s size_t iv_len = native_iv->size(); if (iv_len != 12) { // Only set if not the default length - if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_SET_IVLEN, static_cast(iv_len), nullptr) != 1) { + if (EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_GCM_SET_IVLEN, static_cast(iv_len), nullptr) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; + ctx.reset(); throw std::runtime_error("GCMCipher: Failed to set IV length: " + std::string(err_buf)); } } @@ -55,12 +50,11 @@ void GCMCipher::init(const std::shared_ptr cipher_key, const std::s const unsigned char* key_ptr = reinterpret_cast(native_key->data()); const unsigned char* iv_ptr = reinterpret_cast(native_iv->data()); - if (EVP_CipherInit_ex(ctx, nullptr, nullptr, key_ptr, iv_ptr, is_cipher) != 1) { + if (EVP_CipherInit_ex(ctx.get(), nullptr, nullptr, key_ptr, iv_ptr, is_cipher) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; + ctx.reset(); throw std::runtime_error("GCMCipher: Failed to set key/IV: " + std::string(err_buf)); } } diff --git a/packages/react-native-quick-crypto/cpp/cipher/HybridCipher.cpp b/packages/react-native-quick-crypto/cpp/cipher/HybridCipher.cpp index 85eafd8e..4d966c3e 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/HybridCipher.cpp +++ b/packages/react-native-quick-crypto/cpp/cipher/HybridCipher.cpp @@ -14,12 +14,9 @@ namespace margelo::nitro::crypto { -HybridCipher::~HybridCipher() { - if (ctx) { - EVP_CIPHER_CTX_free(ctx); - // No need to set ctx = nullptr here, object is being destroyed - } -} +// The unique_ptr in the base class destroys ctx automatically — nothing for +// us to do here. Subclasses MUST NOT touch ctx in their own destructors. +HybridCipher::~HybridCipher() = default; void HybridCipher::checkCtx() const { if (!ctx) { @@ -37,7 +34,7 @@ bool HybridCipher::maybePassAuthTagToOpenSSL() { if (auth_tag_state == kAuthTagKnown) { OSSL_PARAM params[] = {OSSL_PARAM_construct_octet_string(OSSL_CIPHER_PARAM_AEAD_TAG, auth_tag, auth_tag_len), OSSL_PARAM_construct_end()}; - if (!EVP_CIPHER_CTX_set_params(ctx, params)) { + if (!EVP_CIPHER_CTX_set_params(ctx.get(), params)) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); @@ -49,11 +46,8 @@ bool HybridCipher::maybePassAuthTagToOpenSSL() { } void HybridCipher::init(const std::shared_ptr cipher_key, const std::shared_ptr iv) { - // Clean up any existing context - if (ctx) { - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; - } + // Resetting the unique_ptr frees any previous context. + ctx.reset(); is_finalized = false; // 1. Get cipher implementation by name @@ -63,19 +57,18 @@ void HybridCipher::init(const std::shared_ptr cipher_key, const std } // 2. Create a new context - ctx = EVP_CIPHER_CTX_new(); + ctx.reset(EVP_CIPHER_CTX_new()); if (!ctx) { throw std::runtime_error("Failed to create cipher context"); } // Initialise the encryption/decryption operation with the cipher type. // Key and IV will be set later by the derived class if needed. - if (EVP_CipherInit_ex(ctx, cipher, nullptr, nullptr, nullptr, is_cipher) != 1) { + if (EVP_CipherInit_ex(ctx.get(), cipher, nullptr, nullptr, nullptr, is_cipher) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; + ctx.reset(); throw std::runtime_error("HybridCipher: Failed initial CipherInit setup: " + std::string(err_buf)); } @@ -86,12 +79,11 @@ void HybridCipher::init(const std::shared_ptr cipher_key, const std const unsigned char* key_ptr = reinterpret_cast(native_key->data()); const unsigned char* iv_ptr = reinterpret_cast(native_iv->data()); - if (EVP_CipherInit_ex(ctx, nullptr, nullptr, key_ptr, iv_ptr, is_cipher) != 1) { + if (EVP_CipherInit_ex(ctx.get(), nullptr, nullptr, key_ptr, iv_ptr, is_cipher) != 1) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); - EVP_CIPHER_CTX_free(ctx); - ctx = nullptr; + ctx.reset(); throw std::runtime_error("HybridCipher: Failed to set key/IV: " + std::string(err_buf)); } @@ -99,8 +91,8 @@ void HybridCipher::init(const std::shared_ptr cipher_key, const std std::string cipher_name(cipher_type); if (cipher_name.find("-wrap") != std::string::npos) { // This flag is required for AES-KW in OpenSSL 3.x - EVP_CIPHER_CTX_set_flags(ctx, EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); - EVP_CIPHER_CTX_set_padding(ctx, 0); + EVP_CIPHER_CTX_set_flags(ctx.get(), EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); + EVP_CIPHER_CTX_set_padding(ctx.get(), 0); } } @@ -113,11 +105,11 @@ std::shared_ptr HybridCipher::update(const std::shared_ptrdata(), in_len); + int ret = EVP_CipherUpdate(ctx.get(), out, &out_len, native_data->data(), in_len); if (!ret) { unsigned long err = ERR_get_error(); @@ -135,13 +127,13 @@ std::shared_ptr HybridCipher::final() { checkCtx(); checkNotFinalized(); // Block size is max output size for final, unless EVP_CIPH_NO_PADDING is set - int block_size = EVP_CIPHER_CTX_block_size(ctx); + int block_size = EVP_CIPHER_CTX_block_size(ctx.get()); if (block_size <= 0) block_size = 16; // Default if block size is weird (e.g., 0) auto out_buf = std::make_unique(block_size); int out_len = 0; - int ret = EVP_CipherFinal_ex(ctx, out_buf.get(), &out_len); + int ret = EVP_CipherFinal_ex(ctx.get(), out_buf.get(), &out_len); if (!ret) { unsigned long err = ERR_get_error(); char err_buf[256]; @@ -169,7 +161,7 @@ bool HybridCipher::setAAD(const std::shared_ptr& data, std::optiona // Set the AAD int out_len; - if (!EVP_CipherUpdate(ctx, nullptr, &out_len, native_data->data(), native_data->size())) { + if (!EVP_CipherUpdate(ctx.get(), nullptr, &out_len, native_data->data(), native_data->size())) { return false; } @@ -179,7 +171,7 @@ bool HybridCipher::setAAD(const std::shared_ptr& data, std::optiona bool HybridCipher::setAutoPadding(bool autoPad) { checkCtx(); - return EVP_CIPHER_CTX_set_padding(ctx, autoPad) == 1; + return EVP_CIPHER_CTX_set_padding(ctx.get(), autoPad) == 1; } bool HybridCipher::setAuthTag(const std::shared_ptr& tag) { @@ -193,7 +185,7 @@ bool HybridCipher::setAuthTag(const std::shared_ptr& tag) { size_t tag_len = native_tag->size(); uint8_t* tag_ptr = native_tag->data(); - int mode = EVP_CIPHER_CTX_mode(ctx); + int mode = EVP_CIPHER_CTX_mode(ctx.get()); if (mode == EVP_CIPH_GCM_MODE || mode == EVP_CIPH_OCB_MODE) { // Use EVP_CTRL_AEAD_SET_TAG for GCM/OCB decryption @@ -202,10 +194,10 @@ bool HybridCipher::setAuthTag(const std::shared_ptr& tag) { } // Add check for valid cipher in context before setting tag // Use the correct OpenSSL 3 function: EVP_CIPHER_CTX_cipher - if (!EVP_CIPHER_CTX_cipher(ctx)) { + if (!EVP_CIPHER_CTX_cipher(ctx.get())) { throw std::runtime_error("Context has no cipher set before setting GCM/OCB tag"); } - if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, tag_len, tag_ptr) <= 0) { + if (EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_AEAD_SET_TAG, tag_len, tag_ptr) <= 0) { unsigned long err = ERR_get_error(); char err_buf[256]; ERR_error_string_n(err, err_buf, sizeof(err_buf)); @@ -235,7 +227,7 @@ bool HybridCipher::setAuthTag(const std::shared_ptr& tag) { std::shared_ptr HybridCipher::getAuthTag() { checkCtx(); - int mode = EVP_CIPHER_CTX_mode(ctx); + int mode = EVP_CIPHER_CTX_mode(ctx.get()); if (!is_cipher) { throw std::runtime_error("getAuthTag can only be called during encryption."); @@ -246,7 +238,7 @@ std::shared_ptr HybridCipher::getAuthTag() { constexpr int max_tag_len = 16; // GCM/OCB tags are typically up to 16 bytes auto tag_buf = std::make_unique(max_tag_len); - int ret = EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_GET_TAG, max_tag_len, tag_buf.get()); + int ret = EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_AEAD_GET_TAG, max_tag_len, tag_buf.get()); if (ret <= 0) { unsigned long err = ERR_get_error(); @@ -283,7 +275,7 @@ int HybridCipher::getMode() { if (!ctx) { throw std::runtime_error("Cipher not initialized. Did you call setArgs()?"); } - return EVP_CIPHER_CTX_get_mode(ctx); + return EVP_CIPHER_CTX_get_mode(ctx.get()); } void HybridCipher::setArgs(const CipherArgs& args) { diff --git a/packages/react-native-quick-crypto/cpp/cipher/HybridCipher.hpp b/packages/react-native-quick-crypto/cpp/cipher/HybridCipher.hpp index 27822cd5..cec241db 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/HybridCipher.hpp +++ b/packages/react-native-quick-crypto/cpp/cipher/HybridCipher.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -13,6 +14,15 @@ namespace margelo::nitro::crypto { +// Owning smart pointer for EVP_CIPHER_CTX. Living in the base class means +// subclasses never have to remember to free it — the destruction order +// (subclass → base) automatically calls the deleter when the cipher object +// goes away. The previous design required each subclass to handle ctx in +// its destructor, and three subclasses (CCM, ChaCha20, ChaCha20-Poly1305) +// got it wrong by setting `ctx = nullptr` without calling the free first, +// leaking the OpenSSL cipher context. See audit Phase 1.3. +using EvpCipherCtxPtr = std::unique_ptr; + // Default tag length for OCB, SIV, CCM, ChaCha20-Poly1305 constexpr unsigned kDefaultAuthTagLength = 16; @@ -55,7 +65,7 @@ class HybridCipher : public HybridCipherSpec { bool is_cipher = true; bool is_finalized = false; std::string cipher_type; - EVP_CIPHER_CTX* ctx = nullptr; + EvpCipherCtxPtr ctx{nullptr, EVP_CIPHER_CTX_free}; bool pending_auth_failed = false; bool has_aad = false; uint8_t auth_tag[EVP_GCM_TLS_TAG_LEN]; diff --git a/packages/react-native-quick-crypto/cpp/cipher/OCBCipher.cpp b/packages/react-native-quick-crypto/cpp/cipher/OCBCipher.cpp index fc15be7e..96ad1a1f 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/OCBCipher.cpp +++ b/packages/react-native-quick-crypto/cpp/cipher/OCBCipher.cpp @@ -17,7 +17,7 @@ void OCBCipher::init(const std::shared_ptr& key, const std::shared_ if (auth_tag_len < 8 || auth_tag_len > 16) { throw std::runtime_error("OCB tag length must be between 8 and 16 bytes"); } - if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, auth_tag_len, nullptr) != 1) { + if (EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_AEAD_SET_TAG, auth_tag_len, nullptr) != 1) { throw std::runtime_error("Failed to set OCB tag length"); } } @@ -28,7 +28,7 @@ std::shared_ptr OCBCipher::getAuthTag() { throw std::runtime_error("getAuthTag can only be called during encryption."); } auto tag_buf = std::make_unique(auth_tag_len); - if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_GET_TAG, auth_tag_len, tag_buf.get()) != 1) { + if (EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_AEAD_GET_TAG, auth_tag_len, tag_buf.get()) != 1) { throw std::runtime_error("Failed to get OCB auth tag"); } uint8_t* raw_ptr = tag_buf.get(); @@ -45,7 +45,7 @@ bool OCBCipher::setAuthTag(const std::shared_ptr& tag) { if (tag_len < 8 || tag_len > 16) { throw std::runtime_error("Invalid OCB tag length"); } - if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, tag_len, native_tag->data()) != 1) { + if (EVP_CIPHER_CTX_ctrl(ctx.get(), EVP_CTRL_AEAD_SET_TAG, tag_len, native_tag->data()) != 1) { throw std::runtime_error("Failed to set OCB auth tag"); } auth_tag_len = tag_len; diff --git a/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Cipher.hpp b/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Cipher.hpp index d39fdf06..54fec370 100644 --- a/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Cipher.hpp +++ b/packages/react-native-quick-crypto/cpp/cipher/XSalsa20Cipher.hpp @@ -20,15 +20,14 @@ namespace margelo::nitro::crypto { class XSalsa20Cipher : public HybridCipher { public: XSalsa20Cipher() : HybridObject(TAG) {} - ~XSalsa20Cipher() { + ~XSalsa20Cipher() override { // Wipe key material and any cached keystream bytes before the heap is // returned. Without this the secret-bearing bytes persist on the heap // until overwritten — see audit HIGH finding (XSalsa20Cipher.hpp:19-22). + // The base-class unique_ptr ctx frees itself; we don't touch it here. secureZero(key); secureZero(nonce); secureZero(leftover_keystream); - // Let parent destructor free the context - ctx = nullptr; } void init(const std::shared_ptr cipher_key, const std::shared_ptr iv) override; From 7766d425cb6f5273bf7abae461fb9f444d810525 Mon Sep 17 00:00:00 2001 From: Brad Anderson Date: Sun, 26 Apr 2026 22:23:29 -0400 Subject: [PATCH 4/4] feat(security): replace Record getUIntOption with typed helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous getUIntOption signature defeated the type checker: function getUIntOption(options: Record, key: string) `any` everywhere meant the value extracted by `options[key]` was typed as `any` and passed to the cipher constructor with zero validation beyond the cryptic `value >>> 0 !== value` check. The function also returned `-1` as a sentinel for "missing", forcing every caller into the awkward `getUIntOption(...) !== -1 ? getUIntOption(...) : default` pattern (which double-evaluates). Replace with a typed helper: - Input: `Readonly> | undefined` — narrows from `any` to `unknown`, requiring runtime checks before use. - Return: `number | undefined` — the missing case is type-encoded. - Validation: explicit `Number.isFinite/Integer + range [0, 2^32-1]` with a typed `RangeError` and a descriptive message. The single call site collapses from a 4-line conditional to: const authTagLen = getUIntOption(options, 'authTagLength') ?? 16; Adds 4 regression tests covering negative, NaN, fractional, and missing-defaults-to-16 paths. Phase 1.4 of plans/todo/security-audit.md. --- example/src/tests/cipher/cipher_tests.ts | 45 +++++++++++++++++++ .../react-native-quick-crypto/src/cipher.ts | 10 +++-- .../src/utils/cipher.ts | 38 ++++++++++++---- 3 files changed, 81 insertions(+), 12 deletions(-) diff --git a/example/src/tests/cipher/cipher_tests.ts b/example/src/tests/cipher/cipher_tests.ts index 0f276982..44eb6b7e 100644 --- a/example/src/tests/cipher/cipher_tests.ts +++ b/example/src/tests/cipher/cipher_tests.ts @@ -492,3 +492,48 @@ test( expect(() => decipher.final()).to.throw(); }, ); + +// --- getUIntOption type-safety regression (Phase 1.4) --- +// +// Ensure the AEAD `authTagLength` option is validated at the JS boundary. +// The previous implementation used `Record` and the cryptic +// `value >>> 0 !== value` check; the typed replacement throws RangeError +// with a clear "must be a non-negative 32-bit integer" message. + +test(SUITE, 'createCipheriv: rejects negative authTagLength', () => { + expect(() => { + createCipheriv('aes-256-gcm', key32, iv12, { + authTagLength: -1, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + }).to.throw(/non-negative/i); +}); + +test(SUITE, 'createCipheriv: rejects NaN authTagLength', () => { + expect(() => { + createCipheriv('aes-256-gcm', key32, iv12, { + authTagLength: NaN, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + }).to.throw(/non-negative/i); +}); + +test(SUITE, 'createCipheriv: rejects fractional authTagLength', () => { + expect(() => { + createCipheriv('aes-256-gcm', key32, iv12, { + authTagLength: 12.5, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any); + }).to.throw(/non-negative/i); +}); + +test( + SUITE, + 'createCipheriv: missing authTagLength still defaults to 16', + () => { + // Sanity check that the new helper's `?? 16` default still kicks in. + expect(() => { + createCipheriv('aes-256-gcm', key32, iv12, {}); + }).to.not.throw(); + }, +); diff --git a/packages/react-native-quick-crypto/src/cipher.ts b/packages/react-native-quick-crypto/src/cipher.ts index 3492247e..d8917dc5 100644 --- a/packages/react-native-quick-crypto/src/cipher.ts +++ b/packages/react-native-quick-crypto/src/cipher.ts @@ -107,10 +107,12 @@ class CipherCommon extends Stream.Transform { } super(streamOptions); // Pass filtered options - const authTagLen: number = - getUIntOption(options ?? {}, 'authTagLength') !== -1 - ? getUIntOption(options ?? {}, 'authTagLength') - : 16; // defaults to 16 bytes + // defaults to 16 bytes for AEAD modes; non-AEAD callers ignore it. + const authTagLen = + getUIntOption( + options as Readonly> | undefined, + 'authTagLength', + ) ?? 16; const factory = NitroModules.createHybridObject('CipherFactory'); diff --git a/packages/react-native-quick-crypto/src/utils/cipher.ts b/packages/react-native-quick-crypto/src/utils/cipher.ts index df505beb..bbb71e25 100644 --- a/packages/react-native-quick-crypto/src/utils/cipher.ts +++ b/packages/react-native-quick-crypto/src/utils/cipher.ts @@ -48,13 +48,35 @@ export function validateEncoding(data: string, encoding: string) { } } -// eslint-disable-next-line @typescript-eslint/no-explicit-any -export function getUIntOption(options: Record, key: string) { - let value; - if (options && (value = options[key]) != null) { - // >>> Turns any type into a positive integer (also sets the sign bit to 0) - if (value >>> 0 !== value) throw new Error(`options.${key}: ${value}`); - return value; +/** + * Reads an unsigned-integer option from an options-like object. + * + * Returns `undefined` if the option is missing, `null`, or `undefined`. + * Throws `RangeError` if the value is present but not a non-negative + * 32-bit integer (NaN, Infinity, fractional, negative, or > 2^32 - 1). + * + * Replaces the previous `Record` + sentinel-`-1` signature, + * which defeated the type checker (audit Phase 1.4). Callers that used + * `getUIntOption(opts ?? {}, key) !== -1 ? getUIntOption(...) : default` + * collapse to `getUIntOption(opts, key) ?? default`. + */ +export function getUIntOption( + options: Readonly> | undefined, + key: string, +): number | undefined { + if (options == null) return undefined; + const value = options[key]; + if (value == null) return undefined; + if ( + typeof value !== 'number' || + !Number.isFinite(value) || + !Number.isInteger(value) || + value < 0 || + value > 0xffff_ffff + ) { + throw new RangeError( + `options.${key} must be a non-negative 32-bit integer, got ${String(value)}`, + ); } - return -1; + return value; }