Skip to content

Commit 4ab3c16

Browse files
jasnelladuh95
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: #57323 Reviewed-By: Yagiz Nizipli <[email protected]>
1 parent d5a26f6 commit 4ab3c16

19 files changed

+248
-195
lines changed

deps/ncrypto/ncrypto.cc

+103-14
Original file line numberDiff line numberDiff line change
@@ -111,20 +111,64 @@ DataPointer DataPointer::Alloc(size_t len) {
111111
#endif
112112
}
113113

114+
DataPointer DataPointer::SecureAlloc(size_t len) {
115+
#ifndef OPENSSL_IS_BORINGSSL
116+
auto ptr = OPENSSL_secure_zalloc(len);
117+
if (ptr == nullptr) return {};
118+
return DataPointer(ptr, len, true);
119+
#else
120+
// BoringSSL does not implement the OPENSSL_secure_zalloc API.
121+
auto ptr = OPENSSL_malloc(len);
122+
if (ptr == nullptr) return {};
123+
memset(ptr, 0, len);
124+
return DataPointer(ptr, len);
125+
#endif
126+
}
127+
128+
size_t DataPointer::GetSecureHeapUsed() {
129+
#ifndef OPENSSL_IS_BORINGSSL
130+
return CRYPTO_secure_malloc_initialized() ? CRYPTO_secure_used() : 0;
131+
#else
132+
// BoringSSL does not have the secure heap and therefore
133+
// will always return 0.
134+
return 0;
135+
#endif
136+
}
137+
138+
DataPointer::InitSecureHeapResult DataPointer::TryInitSecureHeap(size_t amount,
139+
size_t min) {
140+
#ifndef OPENSSL_IS_BORINGSSL
141+
switch (CRYPTO_secure_malloc_init(amount, min)) {
142+
case 0:
143+
return InitSecureHeapResult::FAILED;
144+
case 2:
145+
return InitSecureHeapResult::UNABLE_TO_MEMORY_MAP;
146+
case 1:
147+
return InitSecureHeapResult::OK;
148+
default:
149+
return InitSecureHeapResult::FAILED;
150+
}
151+
#else
152+
// BoringSSL does not actually support the secure heap
153+
return InitSecureHeapResult::FAILED;
154+
#endif
155+
}
156+
114157
DataPointer DataPointer::Copy(const Buffer<const void>& buffer) {
115158
return DataPointer(OPENSSL_memdup(buffer.data, buffer.len), buffer.len);
116159
}
117160

118-
DataPointer::DataPointer(void* data, size_t length)
119-
: data_(data), len_(length) {}
161+
DataPointer::DataPointer(void* data, size_t length, bool secure)
162+
: data_(data), len_(length), secure_(secure) {}
120163

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

124167
DataPointer::DataPointer(DataPointer&& other) noexcept
125-
: data_(other.data_), len_(other.len_) {
168+
: data_(other.data_), len_(other.len_), secure_(other.secure_) {
126169
other.data_ = nullptr;
127170
other.len_ = 0;
171+
other.secure_ = false;
128172
}
129173

130174
DataPointer& DataPointer::operator=(DataPointer&& other) noexcept {
@@ -144,7 +188,11 @@ void DataPointer::zero() {
144188

145189
void DataPointer::reset(void* data, size_t length) {
146190
if (data_ != nullptr) {
147-
OPENSSL_clear_free(data_, len_);
191+
if (secure_) {
192+
OPENSSL_secure_clear_free(data_, len_);
193+
} else {
194+
OPENSSL_clear_free(data_, len_);
195+
}
148196
}
149197
data_ = data;
150198
len_ = length;
@@ -175,6 +223,7 @@ DataPointer DataPointer::resize(size_t len) {
175223

176224
// ============================================================================
177225
bool isFipsEnabled() {
226+
ClearErrorOnReturn clear_error_on_return;
178227
#if OPENSSL_VERSION_MAJOR >= 3
179228
return EVP_default_properties_is_fips_enabled(nullptr) == 1;
180229
#else
@@ -186,30 +235,31 @@ bool setFipsEnabled(bool enable, CryptoErrorList* errors) {
186235
if (isFipsEnabled() == enable) return true;
187236
ClearErrorOnReturn clearErrorOnReturn(errors);
188237
#if OPENSSL_VERSION_MAJOR >= 3
189-
return EVP_default_properties_enable_fips(nullptr, enable ? 1 : 0) == 1;
238+
return EVP_default_properties_enable_fips(nullptr, enable ? 1 : 0) == 1 &&
239+
EVP_default_properties_is_fips_enabled(nullptr);
190240
#else
191241
return FIPS_mode_set(enable ? 1 : 0) == 1;
192242
#endif
193243
}
194244

195245
bool testFipsEnabled() {
246+
ClearErrorOnReturn clear_error_on_return;
196247
#if OPENSSL_VERSION_MAJOR >= 3
197248
OSSL_PROVIDER* fips_provider = nullptr;
198249
if (OSSL_PROVIDER_available(nullptr, "fips")) {
199250
fips_provider = OSSL_PROVIDER_load(nullptr, "fips");
200251
}
201-
const auto enabled = fips_provider == nullptr ? 0
202-
: OSSL_PROVIDER_self_test(fips_provider) ? 1
203-
: 0;
252+
if (fips_provider == nullptr) return false;
253+
int result = OSSL_PROVIDER_self_test(fips_provider);
254+
OSSL_PROVIDER_unload(fips_provider);
255+
return result;
204256
#else
205257
#ifdef OPENSSL_FIPS
206-
const auto enabled = FIPS_selftest() ? 1 : 0;
258+
return FIPS_selftest();
207259
#else // OPENSSL_FIPS
208-
const auto enabled = 0;
260+
return false;
209261
#endif // OPENSSL_FIPS
210262
#endif
211-
212-
return enabled;
213263
}
214264

215265
// ============================================================================
@@ -2689,6 +2739,21 @@ std::optional<std::string_view> SSLPointer::getCipherVersion() const {
26892739
return SSL_CIPHER_get_version(cipher);
26902740
}
26912741

2742+
std::optional<int> SSLPointer::getSecurityLevel() {
2743+
#ifndef OPENSSL_IS_BORINGSSL
2744+
auto ctx = SSLCtxPointer::New();
2745+
if (!ctx) return std::nullopt;
2746+
2747+
auto ssl = SSLPointer::New(ctx);
2748+
if (!ssl) return std::nullopt;
2749+
2750+
return SSL_get_security_level(ssl);
2751+
#else
2752+
// for BoringSSL assume the same as the default
2753+
return OPENSSL_TLS_SECURITY_LEVEL;
2754+
#endif // OPENSSL_IS_BORINGSSL
2755+
}
2756+
26922757
SSLCtxPointer::SSLCtxPointer(SSL_CTX* ctx) : ctx_(ctx) {}
26932758

26942759
SSLCtxPointer::SSLCtxPointer(SSLCtxPointer&& other) noexcept
@@ -3290,6 +3355,10 @@ EVPKeyCtxPointer EVPKeyCtxPointer::New(const EVPKeyPointer& key) {
32903355
}
32913356

32923357
EVPKeyCtxPointer EVPKeyCtxPointer::NewFromID(int id) {
3358+
#ifdef OPENSSL_IS_BORINGSSL
3359+
// DSA keys are not supported with BoringSSL
3360+
if (id == EVP_PKEY_DSA) return {};
3361+
#endif
32933362
return EVPKeyCtxPointer(EVP_PKEY_CTX_new_id(id, nullptr));
32943363
}
32953364

@@ -3852,6 +3921,26 @@ int Ec::getCurve() const {
38523921
return EC_GROUP_get_curve_name(getGroup());
38533922
}
38543923

3924+
int Ec::GetCurveIdFromName(std::string_view name) {
3925+
int nid = EC_curve_nist2nid(name.data());
3926+
if (nid == NID_undef) {
3927+
nid = OBJ_sn2nid(name.data());
3928+
}
3929+
return nid;
3930+
}
3931+
3932+
bool Ec::GetCurves(Ec::GetCurveCallback callback) {
3933+
const size_t count = EC_get_builtin_curves(nullptr, 0);
3934+
std::vector<EC_builtin_curve> curves(count);
3935+
if (EC_get_builtin_curves(curves.data(), count) != count) {
3936+
return false;
3937+
}
3938+
for (auto curve : curves) {
3939+
if (!callback(OBJ_nid2sn(curve.nid))) return false;
3940+
}
3941+
return true;
3942+
}
3943+
38553944
// ============================================================================
38563945

38573946
EVPMDCtxPointer::EVPMDCtxPointer() : ctx_(nullptr) {}

deps/ncrypto/ncrypto.h

+34-2
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,11 @@ class Ec final {
469469
inline operator bool() const { return ec_ != nullptr; }
470470
inline operator OSSL3_CONST EC_KEY*() const { return ec_; }
471471

472+
static int GetCurveIdFromName(std::string_view name);
473+
474+
using GetCurveCallback = std::function<bool(std::string_view)>;
475+
static bool GetCurves(GetCurveCallback callback);
476+
472477
private:
473478
OSSL3_CONST EC_KEY* ec_ = nullptr;
474479
};
@@ -480,9 +485,31 @@ class DataPointer final {
480485
static DataPointer Alloc(size_t len);
481486
static DataPointer Copy(const Buffer<const void>& buffer);
482487

488+
// Attempts to allocate the buffer space using the secure heap, if
489+
// supported/enabled. If the secure heap is disabled, then this
490+
// ends up being equivalent to Alloc(len). Note that allocation
491+
// will fail if there is not enough free space remaining in the
492+
// secure heap space.
493+
static DataPointer SecureAlloc(size_t len);
494+
495+
// If the secure heap is enabled, returns the amount of data that
496+
// has been allocated from the heap.
497+
static size_t GetSecureHeapUsed();
498+
499+
enum class InitSecureHeapResult {
500+
FAILED,
501+
UNABLE_TO_MEMORY_MAP,
502+
OK,
503+
};
504+
505+
// Attempt to initialize the secure heap. The secure heap is not
506+
// supported on all operating systems and whenever boringssl is
507+
// used.
508+
static InitSecureHeapResult TryInitSecureHeap(size_t amount, size_t min);
509+
483510
DataPointer() = default;
484-
explicit DataPointer(void* data, size_t len);
485-
explicit DataPointer(const Buffer<void>& buffer);
511+
explicit DataPointer(void* data, size_t len, bool secure = false);
512+
explicit DataPointer(const Buffer<void>& buffer, bool secure = false);
486513
DataPointer(DataPointer&& other) noexcept;
487514
DataPointer& operator=(DataPointer&& other) noexcept;
488515
NCRYPTO_DISALLOW_COPY(DataPointer)
@@ -518,9 +545,12 @@ class DataPointer final {
518545
};
519546
}
520547

548+
bool isSecure() const { return secure_; }
549+
521550
private:
522551
void* data_ = nullptr;
523552
size_t len_ = 0;
553+
bool secure_ = false;
524554
};
525555

526556
class BIOPointer final {
@@ -1048,6 +1078,8 @@ class SSLPointer final {
10481078

10491079
std::optional<uint32_t> verifyPeerCertificate() const;
10501080

1081+
static std::optional<int> getSecurityLevel();
1082+
10511083
void getCiphers(std::function<void(const std::string_view)> cb) const;
10521084

10531085
static SSLPointer New(const SSLCtxPointer& ctx);

src/crypto/crypto_common.cc

+3-4
Original file line numberDiff line numberDiff line change
@@ -255,11 +255,10 @@ MaybeLocal<Object> GetEphemeralKey(Environment* env, const SSLPointer& ssl) {
255255
MaybeLocal<Object> ECPointToBuffer(Environment* env,
256256
const EC_GROUP* group,
257257
const EC_POINT* point,
258-
point_conversion_form_t form,
259-
const char** error) {
258+
point_conversion_form_t form) {
260259
size_t len = EC_POINT_point2oct(group, point, form, nullptr, 0, nullptr);
261260
if (len == 0) {
262-
if (error != nullptr) *error = "Failed to get public key length";
261+
THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to get public key length");
263262
return MaybeLocal<Object>();
264263
}
265264

@@ -273,7 +272,7 @@ MaybeLocal<Object> ECPointToBuffer(Environment* env,
273272
bs->ByteLength(),
274273
nullptr);
275274
if (len == 0) {
276-
if (error != nullptr) *error = "Failed to get public key";
275+
THROW_ERR_CRYPTO_OPERATION_FAILED(env, "Failed to get public key");
277276
return MaybeLocal<Object>();
278277
}
279278

src/crypto/crypto_common.h

+4-6
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,10 @@ v8::MaybeLocal<v8::Value> GetPeerCert(Environment* env,
3535
bool abbreviated = false,
3636
bool is_server = false);
3737

38-
v8::MaybeLocal<v8::Object> ECPointToBuffer(
39-
Environment* env,
40-
const EC_GROUP* group,
41-
const EC_POINT* point,
42-
point_conversion_form_t form,
43-
const char** error);
38+
v8::MaybeLocal<v8::Object> ECPointToBuffer(Environment* env,
39+
const EC_GROUP* group,
40+
const EC_POINT* point,
41+
point_conversion_form_t form);
4442

4543
} // namespace crypto
4644
} // namespace node

src/crypto/crypto_dh.cc

+9-2
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,17 @@ void DiffieHellman::MemoryInfo(MemoryTracker* tracker) const {
5555

5656
namespace {
5757
MaybeLocal<Value> DataPointerToBuffer(Environment* env, DataPointer&& data) {
58+
struct Flag {
59+
bool secure;
60+
};
5861
auto backing = ArrayBuffer::NewBackingStore(
5962
data.get(),
6063
data.size(),
61-
[](void* data, size_t len, void* ptr) { DataPointer free_me(data, len); },
62-
nullptr);
64+
[](void* data, size_t len, void* ptr) {
65+
std::unique_ptr<Flag> flag(static_cast<Flag*>(ptr));
66+
DataPointer free_me(data, len, flag->secure);
67+
},
68+
new Flag{data.isSecure()});
6369
data.release();
6470

6571
auto ab = ArrayBuffer::New(env->isolate(), std::move(backing));
@@ -482,6 +488,7 @@ ByteSource StatelessDiffieHellmanThreadsafe(const EVPKeyPointer& our_key,
482488
const EVPKeyPointer& their_key) {
483489
auto dp = DHPointer::stateless(our_key, their_key);
484490
if (!dp) return {};
491+
DCHECK(!dp.isSecure());
485492

486493
return ByteSource::Allocated(dp.release());
487494
}

src/crypto/crypto_dsa.cc

+1-6
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,9 @@ using v8::Value;
2828

2929
namespace crypto {
3030
EVPKeyCtxPointer DsaKeyGenTraits::Setup(DsaKeyPairGenConfig* params) {
31-
#ifdef OPENSSL_IS_BORINGSSL
32-
// Operation is unsupported in BoringSSL
33-
return {};
34-
#else
3531
auto param_ctx = EVPKeyCtxPointer::NewFromID(EVP_PKEY_DSA);
3632

37-
if (!param_ctx.initForParamgen() ||
33+
if (!param_ctx || !param_ctx.initForParamgen() ||
3834
!param_ctx.setDsaParameters(
3935
params->params.modulus_bits,
4036
params->params.divisor_bits != -1
@@ -49,7 +45,6 @@ EVPKeyCtxPointer DsaKeyGenTraits::Setup(DsaKeyPairGenConfig* params) {
4945
EVPKeyCtxPointer key_ctx = key_params.newCtx();
5046
if (!key_ctx.initForKeygen()) return {};
5147
return key_ctx;
52-
#endif
5348
}
5449

5550
// Input arguments for DsaKeyPairGenJob

0 commit comments

Comments
 (0)