Skip to content

Commit b10653a

Browse files
panvaaduh95
panva
authored and
aduh95
committed
crypto: add guards and adjust tests for BoringSSL
Signed-off-by: Filip Skokan <panva.ip@gmail.com> PR-URL: #62883 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
1 parent 19b7adf commit b10653a

53 files changed

Lines changed: 473 additions & 168 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎deps/ncrypto/ncrypto.cc‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,12 @@ DataPointer DataPointer::SecureAlloc(size_t len) {
147147
#ifndef OPENSSL_IS_BORINGSSL
148148
auto ptr = OPENSSL_secure_zalloc(len);
149149
if (ptr == nullptr) return {};
150-
return DataPointer(ptr, len, true);
150+
// OPENSSL_secure_zalloc transparently falls back to a regular allocation
151+
// when the secure heap is not initialized or is exhausted. Reflect the
152+
// actual provenance of the pointer so that reset() routes to the correct
153+
// free function (OPENSSL_secure_clear_free vs. OPENSSL_clear_free) and
154+
// callers of isSecure() get a truthful answer.
155+
return DataPointer(ptr, len, CRYPTO_secure_allocated(ptr) == 1);
151156
#else
152157
// BoringSSL does not implement the OPENSSL_secure_zalloc API.
153158
auto ptr = OPENSSL_malloc(len);
@@ -3103,9 +3108,13 @@ const Cipher Cipher::AES_256_GCM = Cipher::FromNid(NID_aes_256_gcm);
31033108
const Cipher Cipher::AES_128_KW = Cipher::FromNid(NID_id_aes128_wrap);
31043109
const Cipher Cipher::AES_192_KW = Cipher::FromNid(NID_id_aes192_wrap);
31053110
const Cipher Cipher::AES_256_KW = Cipher::FromNid(NID_id_aes256_wrap);
3111+
3112+
#ifndef OPENSSL_IS_BORINGSSL
31063113
const Cipher Cipher::AES_128_OCB = Cipher::FromNid(NID_aes_128_ocb);
31073114
const Cipher Cipher::AES_192_OCB = Cipher::FromNid(NID_aes_192_ocb);
31083115
const Cipher Cipher::AES_256_OCB = Cipher::FromNid(NID_aes_256_ocb);
3116+
#endif
3117+
31093118
const Cipher Cipher::CHACHA20_POLY1305 = Cipher::FromNid(NID_chacha20_poly1305);
31103119

31113120
bool Cipher::isGcmMode() const {

‎deps/ncrypto/ncrypto.h‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -309,9 +309,12 @@ class Cipher final {
309309
#else
310310
static constexpr size_t MAX_AUTH_TAG_LENGTH = 16;
311311
#endif
312-
static_assert(EVP_GCM_TLS_TAG_LEN <= MAX_AUTH_TAG_LENGTH &&
313-
EVP_CCM_TLS_TAG_LEN <= MAX_AUTH_TAG_LENGTH &&
314-
EVP_CHACHAPOLY_TLS_TAG_LEN <= MAX_AUTH_TAG_LENGTH);
312+
static_assert(EVP_GCM_TLS_TAG_LEN <= MAX_AUTH_TAG_LENGTH
313+
#ifndef OPENSSL_IS_BORINGSSL
314+
&& EVP_CCM_TLS_TAG_LEN <= MAX_AUTH_TAG_LENGTH &&
315+
EVP_CHACHAPOLY_TLS_TAG_LEN <= MAX_AUTH_TAG_LENGTH
316+
#endif
317+
); // NOLINT(whitespace/parens)
315318

316319
Cipher() = default;
317320
Cipher(const EVP_CIPHER* cipher) : cipher_(cipher) {}

‎src/crypto/crypto_cipher.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -447,8 +447,10 @@ bool CipherBase::InitAuthenticated(const char* cipher_type,
447447
// Other modes (CCM, OCB) require an explicit tag length.
448448
if (ctx_.isGcmMode()) {
449449
auth_tag_len = EVP_GCM_TLS_TAG_LEN;
450+
#ifdef EVP_CHACHAPOLY_TLS_TAG_LEN
450451
} else if (ctx_.isChaCha20Poly1305()) {
451452
auth_tag_len = EVP_CHACHAPOLY_TLS_TAG_LEN;
453+
#endif
452454
} else {
453455
THROW_ERR_CRYPTO_INVALID_AUTH_TAG(
454456
env(), "authTagLength required for %s", cipher_type);

‎src/crypto/crypto_context.cc‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1928,8 +1928,13 @@ void SecureContext::SetDHParam(const FunctionCallbackInfo<Value>& args) {
19281928
// true to this function instead of the original string. Any other string
19291929
// value will be interpreted as custom DH parameters below.
19301930
if (args[0]->IsTrue()) {
1931+
#ifdef SSL_CTX_set_dh_auto
19311932
CHECK(SSL_CTX_set_dh_auto(sc->ctx_.get(), true));
19321933
return;
1934+
#else
1935+
return THROW_ERR_CRYPTO_UNSUPPORTED_OPERATION(
1936+
env, "Automatic DH parameter selection is not supported");
1937+
#endif
19331938
}
19341939

19351940
DHPointer dh;

‎src/crypto/crypto_dh.cc‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -309,15 +309,17 @@ void ComputeSecret(const FunctionCallbackInfo<Value>& args) {
309309
BignumPointer key(key_buf.data(), key_buf.size());
310310

311311
switch (dh.checkPublicKey(key)) {
312-
case DHPointer::CheckPublicKeyResult::INVALID:
313-
// Fall-through
314312
case DHPointer::CheckPublicKeyResult::CHECK_FAILED:
315313
return THROW_ERR_CRYPTO_INVALID_KEYTYPE(env,
316314
"Unspecified validation error");
315+
#ifndef OPENSSL_IS_BORINGSSL
317316
case DHPointer::CheckPublicKeyResult::TOO_SMALL:
318317
return THROW_ERR_CRYPTO_INVALID_KEYLEN(env, "Supplied key is too small");
319318
case DHPointer::CheckPublicKeyResult::TOO_LARGE:
320319
return THROW_ERR_CRYPTO_INVALID_KEYLEN(env, "Supplied key is too large");
320+
#endif
321+
case DHPointer::CheckPublicKeyResult::INVALID:
322+
return THROW_ERR_CRYPTO_INVALID_KEYTYPE(env, "Supplied key is invalid");
321323
case DHPointer::CheckPublicKeyResult::NONE:
322324
break;
323325
}

‎src/crypto/crypto_rsa.cc‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,18 @@ Maybe<void> RsaKeyGenTraits::AdditionalConfig(
135135
params->params.modulus_bits = args[*offset + 1].As<Uint32>()->Value();
136136
params->params.exponent = args[*offset + 2].As<Uint32>()->Value();
137137

138+
#ifdef OPENSSL_IS_BORINGSSL
139+
// BoringSSL hangs indefinitely generating an RSA key with e=1, and for
140+
// other invalid exponents (e=0, even values) reports the misleading error
141+
// RSA_R_TOO_MANY_ITERATIONS only after running the full keygen loop. Reject
142+
// those up-front with a clear error. The constraint here (odd integer >= 3)
143+
// matches BoringSSL's own rsa_check_public_key validation.
144+
if (params->params.exponent < 3 || (params->params.exponent & 1) == 0) {
145+
THROW_ERR_OUT_OF_RANGE(env, "publicExponent is invalid");
146+
return Nothing<void>();
147+
}
148+
#endif
149+
138150
*offset += 3;
139151

140152
if (params->params.variant == kKeyVariantRSA_PSS) {

‎src/crypto/crypto_util.cc‎

Lines changed: 46 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -553,44 +553,51 @@ Maybe<void> Decorate(Environment* env,
553553
c = ToUpper(c);
554554
}
555555

556-
#define OSSL_ERROR_CODES_MAP(V) \
557-
V(SYS) \
558-
V(BN) \
559-
V(RSA) \
560-
V(DH) \
561-
V(EVP) \
562-
V(BUF) \
563-
V(OBJ) \
564-
V(PEM) \
565-
V(DSA) \
566-
V(X509) \
567-
V(ASN1) \
568-
V(CONF) \
569-
V(CRYPTO) \
570-
V(EC) \
571-
V(SSL) \
572-
V(BIO) \
573-
V(PKCS7) \
574-
V(X509V3) \
575-
V(PKCS12) \
576-
V(RAND) \
577-
V(DSO) \
578-
V(ENGINE) \
579-
V(OCSP) \
580-
V(UI) \
581-
V(COMP) \
582-
V(ECDSA) \
583-
V(ECDH) \
584-
V(OSSL_STORE) \
585-
V(FIPS) \
586-
V(CMS) \
587-
V(TS) \
588-
V(HMAC) \
589-
V(CT) \
590-
V(ASYNC) \
591-
V(KDF) \
592-
V(SM2) \
593-
V(USER) \
556+
#ifdef OPENSSL_IS_BORINGSSL
557+
#define OSSL_ERROR_CODES_MAP_OPENSSL_ONLY(V)
558+
#else
559+
#define OSSL_ERROR_CODES_MAP_OPENSSL_ONLY(V) \
560+
V(PKCS12) \
561+
V(DSO) \
562+
V(OSSL_STORE) \
563+
V(FIPS) \
564+
V(TS) \
565+
V(CT) \
566+
V(ASYNC) \
567+
V(KDF) \
568+
V(SM2)
569+
#endif
570+
571+
#define OSSL_ERROR_CODES_MAP(V) \
572+
V(SYS) \
573+
V(BN) \
574+
V(RSA) \
575+
V(DH) \
576+
V(EVP) \
577+
V(BUF) \
578+
V(OBJ) \
579+
V(PEM) \
580+
V(DSA) \
581+
V(X509) \
582+
V(ASN1) \
583+
V(CONF) \
584+
V(CRYPTO) \
585+
V(EC) \
586+
V(SSL) \
587+
V(BIO) \
588+
V(PKCS7) \
589+
V(X509V3) \
590+
V(RAND) \
591+
V(ENGINE) \
592+
V(OCSP) \
593+
V(UI) \
594+
V(COMP) \
595+
V(ECDSA) \
596+
V(ECDH) \
597+
V(CMS) \
598+
V(HMAC) \
599+
V(USER) \
600+
OSSL_ERROR_CODES_MAP_OPENSSL_ONLY(V)
594601

595602
#define V(name) case ERR_LIB_##name: lib = #name "_"; break;
596603
const char* lib = "";
@@ -600,6 +607,7 @@ Maybe<void> Decorate(Environment* env,
600607
}
601608
#undef V
602609
#undef OSSL_ERROR_CODES_MAP
610+
#undef OSSL_ERROR_CODES_MAP_OPENSSL_ONLY
603611
// Don't generate codes like "ERR_OSSL_SSL_".
604612
if (lib && strcmp(lib, "SSL_") == 0)
605613
prefix = "";
@@ -728,7 +736,6 @@ void SecureBuffer(const FunctionCallbackInfo<Value>& args) {
728736
uint32_t len = args[0].As<Uint32>()->Value();
729737

730738
auto data = DataPointer::SecureAlloc(len);
731-
CHECK(data.isSecure());
732739
if (!data) {
733740
return THROW_ERR_OPERATION_FAILED(env, "Allocation failed");
734741
}

‎test/parallel/test-crypto-async-sign-verify.js‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -102,17 +102,19 @@ if (!process.features.openssl_is_boringssl) {
102102
// ECDSA w/ ieee-p1363 signature encoding
103103
test('ec_secp256k1_public.pem', 'ec_secp256k1_private.pem', 'sha384', false,
104104
{ dsaEncoding: 'ieee-p1363' });
105-
}
106105

107-
// DSA w/ der signature encoding
108-
test('dsa_public.pem', 'dsa_private.pem', 'sha256',
109-
false);
110-
test('dsa_public.pem', 'dsa_private.pem', 'sha256',
111-
false, { dsaEncoding: 'der' });
106+
// DSA w/ der signature encoding
107+
test('dsa_public.pem', 'dsa_private.pem', 'sha256',
108+
false);
109+
test('dsa_public.pem', 'dsa_private.pem', 'sha256',
110+
false, { dsaEncoding: 'der' });
112111

113-
// DSA w/ ieee-p1363 signature encoding
114-
test('dsa_public.pem', 'dsa_private.pem', 'sha256', false,
115-
{ dsaEncoding: 'ieee-p1363' });