Skip to content

Commit 3a40e0d

Browse files
jasnellnpaun
authored andcommitted
src: cleanup crypto more
* Use ncrypto APIs where appropriate * Remove obsolete no-longer used functions * Improve error handling a bit * move secure heap handling to ncrypto To simplify handling of boringssl/openssl, move secure heap impl to ncrypto. Overall the reduces the complexity of the code in crypto_util by eliminating additional ifdef branches. * simplify CryptoErrorStore::ToException a bit * simplify error handling in crypto_common * move curve utility methods to ncrypto * verify that released DataPointers aren't on secure heap The ByteSource does not currently know how to free a DataPointer allocated on the secure heap, so just verify. DataPointers on the secure heap are not something that users can allocate on their own. Their use is rare. Eventually ByteSource is going to be refactored around ncrypto APIs so these additional checks should be temporary. * simplify some ifdefs that are covered by ncrypto * cleanup some obsolete includes in crypto_util PR-URL: nodejs/node#57323 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
1 parent 23b2adf commit 3a40e0d

File tree

2 files changed

+96
-17
lines changed

2 files changed

+96
-17
lines changed

include/ncrypto.h

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -587,9 +587,31 @@ class DataPointer final {
587587
static DataPointer Alloc(size_t len);
588588
static DataPointer Copy(const Buffer<const void>& buffer);
589589

590+
// Attempts to allocate the buffer space using the secure heap, if
591+
// supported/enabled. If the secure heap is disabled, then this
592+
// ends up being equivalent to Alloc(len). Note that allocation
593+
// will fail if there is not enough free space remaining in the
594+
// secure heap space.
595+
static DataPointer SecureAlloc(size_t len);
596+
597+
// If the secure heap is enabled, returns the amount of data that
598+
// has been allocated from the heap.
599+
static size_t GetSecureHeapUsed();
600+
601+
enum class InitSecureHeapResult {
602+
FAILED,
603+
UNABLE_TO_MEMORY_MAP,
604+
OK,
605+
};
606+
607+
// Attempt to initialize the secure heap. The secure heap is not
608+
// supported on all operating systems and whenever boringssl is
609+
// used.
610+
static InitSecureHeapResult TryInitSecureHeap(size_t amount, size_t min);
611+
590612
DataPointer() = default;
591-
explicit DataPointer(void* data, size_t len);
592-
explicit DataPointer(const Buffer<void>& buffer);
613+
explicit DataPointer(void* data, size_t len, bool secure = false);
614+
explicit DataPointer(const Buffer<void>& buffer, bool secure = false);
593615
DataPointer(DataPointer&& other) noexcept;
594616
DataPointer& operator=(DataPointer&& other) noexcept;
595617
NCRYPTO_DISALLOW_COPY(DataPointer)
@@ -625,9 +647,12 @@ class DataPointer final {
625647
};
626648
}
627649

650+
bool isSecure() const { return secure_; }
651+
628652
private:
629653
void* data_ = nullptr;
630654
size_t len_ = 0;
655+
bool secure_ = false;
631656
};
632657

633658
class BIOPointer final {

src/ncrypto.cpp

Lines changed: 69 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -154,20 +154,64 @@ DataPointer DataPointer::Alloc(size_t len) {
154154
#endif
155155
}
156156

157+
DataPointer DataPointer::SecureAlloc(size_t len) {
158+
#ifndef OPENSSL_IS_BORINGSSL
159+
auto ptr = OPENSSL_secure_zalloc(len);
160+
if (ptr == nullptr) return {};
161+
return DataPointer(ptr, len, true);
162+
#else
163+
// BoringSSL does not implement the OPENSSL_secure_zalloc API.
164+
auto ptr = OPENSSL_malloc(len);
165+
if (ptr == nullptr) return {};
166+
memset(ptr, 0, len);
167+
return DataPointer(ptr, len);
168+
#endif
169+
}
170+
171+
size_t DataPointer::GetSecureHeapUsed() {
172+
#ifndef OPENSSL_IS_BORINGSSL
173+
return CRYPTO_secure_malloc_initialized() ? CRYPTO_secure_used() : 0;
174+
#else
175+
// BoringSSL does not have the secure heap and therefore
176+
// will always return 0.
177+
return 0;
178+
#endif
179+
}
180+
181+
DataPointer::InitSecureHeapResult DataPointer::TryInitSecureHeap(size_t amount,
182+
size_t min) {
183+
#ifndef OPENSSL_IS_BORINGSSL
184+
switch (CRYPTO_secure_malloc_init(amount, min)) {
185+
case 0:
186+
return InitSecureHeapResult::FAILED;
187+
case 2:
188+
return InitSecureHeapResult::UNABLE_TO_MEMORY_MAP;
189+
case 1:
190+
return InitSecureHeapResult::OK;
191+
default:
192+
return InitSecureHeapResult::FAILED;
193+
}
194+
#else
195+
// BoringSSL does not actually support the secure heap
196+
return InitSecureHeapResult::FAILED;
197+
#endif
198+
}
199+
157200
DataPointer DataPointer::Copy(const Buffer<const void>& buffer) {
158201
return DataPointer(OPENSSL_memdup(buffer.data, buffer.len), buffer.len);
159202
}
160203

161-
DataPointer::DataPointer(void* data, size_t length)
162-
: data_(data), len_(length) {}
204+
DataPointer::DataPointer(void* data, size_t length, bool secure)
205+
: data_(data), len_(length), secure_(secure) {}
163206

164-
DataPointer::DataPointer(const Buffer<void>& buffer)
165-
: data_(buffer.data), len_(buffer.len) {}
207+
DataPointer::DataPointer(const Buffer<void>& buffer, bool secure)
208+
: data_(buffer.data), len_(buffer.len), secure_(secure) {}
166209

167210
DataPointer::DataPointer(DataPointer&& other) noexcept
168-
: data_(other.data_), len_(other.len_) {
211+
: data_(other.data_), len_(other.len_), secure_(other.secure_) {
169212
other.data_ = nullptr;
170213
other.len_ = 0;
214+
other.secure_ = false;
171215
}
172216

173217
DataPointer& DataPointer::operator=(DataPointer&& other) noexcept {
@@ -187,7 +231,11 @@ void DataPointer::zero() {
187231

188232
void DataPointer::reset(void* data, size_t length) {
189233
if (data_ != nullptr) {
190-
OPENSSL_clear_free(data_, len_);
234+
if (secure_) {
235+
OPENSSL_secure_clear_free(data_, len_);
236+
} else {
237+
OPENSSL_clear_free(data_, len_);
238+
}
191239
}
192240
data_ = data;
193241
len_ = length;
@@ -218,6 +266,7 @@ DataPointer DataPointer::resize(size_t len) {
218266

219267
// ============================================================================
220268
bool isFipsEnabled() {
269+
ClearErrorOnReturn clear_error_on_return;
221270
#if OPENSSL_VERSION_MAJOR >= 3
222271
return EVP_default_properties_is_fips_enabled(nullptr) == 1;
223272
#else
@@ -229,30 +278,31 @@ bool setFipsEnabled(bool enable, CryptoErrorList* errors) {
229278
if (isFipsEnabled() == enable) return true;
230279
ClearErrorOnReturn clearErrorOnReturn(errors);
231280
#if OPENSSL_VERSION_MAJOR >= 3
232-
return EVP_default_properties_enable_fips(nullptr, enable ? 1 : 0) == 1;
281+
return EVP_default_properties_enable_fips(nullptr, enable ? 1 : 0) == 1 &&
282+
EVP_default_properties_is_fips_enabled(nullptr);
233283
#else
234284
return FIPS_mode_set(enable ? 1 : 0) == 1;
235285
#endif
236286
}
237287

238288
bool testFipsEnabled() {
289+
ClearErrorOnReturn clear_error_on_return;
239290
#if OPENSSL_VERSION_MAJOR >= 3
240291
OSSL_PROVIDER* fips_provider = nullptr;
241292
if (OSSL_PROVIDER_available(nullptr, "fips")) {
242293
fips_provider = OSSL_PROVIDER_load(nullptr, "fips");
243294
}
244-
const auto enabled = fips_provider == nullptr ? 0
245-
: OSSL_PROVIDER_self_test(fips_provider) ? 1
246-
: 0;
295+
if (fips_provider == nullptr) return false;
296+
int result = OSSL_PROVIDER_self_test(fips_provider);
297+
OSSL_PROVIDER_unload(fips_provider);
298+
return result;
247299
#else
248300
#ifdef OPENSSL_FIPS
249-
const auto enabled = FIPS_selftest() ? 1 : 0;
250-
#else // OPENSSL_FIPS
251-
const auto enabled = 0;
301+
return FIPS_selftest();
302+
#else // OPENSSL_FIPS
303+
return false;
252304
#endif // OPENSSL_FIPS
253305
#endif
254-
255-
return enabled;
256306
}
257307

258308
// ============================================================================
@@ -3713,6 +3763,10 @@ EVPKeyCtxPointer EVPKeyCtxPointer::New(const EVPKeyPointer& key) {
37133763
}
37143764

37153765
EVPKeyCtxPointer EVPKeyCtxPointer::NewFromID(int id) {
3766+
#ifdef OPENSSL_IS_BORINGSSL
3767+
// DSA keys are not supported with BoringSSL
3768+
if (id == EVP_PKEY_DSA) return {};
3769+
#endif
37163770
return EVPKeyCtxPointer(EVP_PKEY_CTX_new_id(id, nullptr));
37173771
}
37183772

0 commit comments

Comments
 (0)