Visitar URL original
crypto: use available BoringSSL APIs · nodejs/node@137ff67 · GitHub
Skip to content

Commit 137ff67

Browse files
panvaaduh95
panva
authored and
aduh95
committed
crypto: use available BoringSSL APIs
Use BoringSSL's current RSA and DH validation results instead of maintaining backend-specific prechecks and collapsing key errors. Report negotiated TLS groups and the documented zero security level through BoringSSL's compatibility APIs. Signed-off-by: Filip Skokan <panva.ip@gmail.com> PR-URL: #65423 Backport-PR-URL: #65483 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent 0e87576 commit 137ff67

10 files changed

Lines changed: 66 additions & 89 deletions

File tree

‎deps/ncrypto/ncrypto.cc‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2252,14 +2252,11 @@ DHPointer::CheckPublicKeyResult DHPointer::checkPublicKey(
22522252
if (DH_check_pub_key(dh_.get(), pub_key.get(), &codes) != 1) {
22532253
return DHPointer::CheckPublicKeyResult::CHECK_FAILED;
22542254
}
2255-
#ifndef OPENSSL_IS_BORINGSSL
2256-
// Boringssl does not define DH_CHECK_PUBKEY_TOO_SMALL or TOO_LARGE
22572255
if (codes & DH_CHECK_PUBKEY_TOO_SMALL) {
22582256
return DHPointer::CheckPublicKeyResult::TOO_SMALL;
22592257
} else if (codes & DH_CHECK_PUBKEY_TOO_LARGE) {
22602258
return DHPointer::CheckPublicKeyResult::TOO_LARGE;
22612259
}
2262-
#endif
22632260
if (codes != 0) {
22642261
return DHPointer::CheckPublicKeyResult::INVALID;
22652262
}
@@ -4309,6 +4306,13 @@ std::optional<std::string_view> SSLPointer::getNegotiatedGroup() const {
43094306
const char* group = SSL_get0_group_name(get());
43104307
if (group == nullptr) return std::nullopt;
43114308
return group;
4309+
#elif defined(OPENSSL_IS_BORINGSSL)
4310+
if (!ssl_) return std::nullopt;
4311+
const int nid = SSL_get_negotiated_group(get());
4312+
if (nid == NID_undef) return std::nullopt;
4313+
const char* group = OBJ_nid2sn(nid);
4314+
if (group == nullptr) return std::nullopt;
4315+
return group;
43124316
#else
43134317
return std::nullopt;
43144318
#endif
@@ -4333,19 +4337,17 @@ std::optional<std::string_view> SSLPointer::getCipherVersion() const {
43334337
}
43344338

43354339
std::optional<int> SSLPointer::getSecurityLevel() {
4336-
#ifndef OPENSSL_IS_BORINGSSL
43374340
auto ctx = SSLCtxPointer::New();
43384341
if (!ctx) return std::nullopt;
43394342

4343+
#ifdef OPENSSL_IS_BORINGSSL
4344+
return SSL_CTX_get_security_level(ctx.get());
4345+
#else
43404346
auto ssl = SSLPointer::New(ctx);
43414347
if (!ssl) return std::nullopt;
43424348

43434349
return SSL_get_security_level(ssl);
4344-
#else
4345-
// OPENSSL_TLS_SECURITY_LEVEL is not defined in BoringSSL
4346-
// so assume it is the default OPENSSL_TLS_SECURITY_LEVEL value.
4347-
return 1;
4348-
#endif // OPENSSL_IS_BORINGSSL
4350+
#endif
43494351
}
43504352

43514353
SSLCtxPointer::SSLCtxPointer(SSL_CTX* ctx) : ctx_(ctx) {}

‎deps/ncrypto/ncrypto.h‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1231,9 +1231,9 @@ class DHPointer final {
12311231
UNABLE_TO_CHECK_GENERATOR = 0x04,
12321232
NOT_SUITABLE_GENERATOR = 0x08,
12331233
Q_NOT_PRIME = 0x10,
1234-
#ifndef OPENSSL_IS_BORINGSSL
1235-
// Boringssl does not define the DH_CHECK_INVALID_[Q or J]_VALUE
12361234
INVALID_Q = 0x20,
1235+
#ifndef OPENSSL_IS_BORINGSSL
1236+
// BoringSSL does not define DH_CHECK_INVALID_J_VALUE.
12371237
INVALID_J = 0x40,
12381238
MODULUS_TOO_SMALL = 0x80,
12391239
MODULUS_TOO_LARGE = 0x100,
@@ -1244,14 +1244,9 @@ class DHPointer final {
12441244

12451245
enum class CheckPublicKeyResult {
12461246
NONE,
1247-
#ifndef OPENSSL_IS_BORINGSSL
1248-
// Boringssl does not define DH_R_CHECK_PUBKEY_TOO_SMALL or TOO_LARGE
1249-
TOO_SMALL = DH_R_CHECK_PUBKEY_TOO_SMALL,
1250-
TOO_LARGE = DH_R_CHECK_PUBKEY_TOO_LARGE,
1251-
INVALID = DH_R_CHECK_PUBKEY_INVALID,
1252-
#else
1253-
INVALID = DH_R_INVALID_PUBKEY,
1254-
#endif
1247+
TOO_SMALL,
1248+
TOO_LARGE,
1249+
INVALID,
12551250
CHECK_FAILED = 512,
12561251
};
12571252
// Check to see if the given public key is suitable for this DH instance.

‎src/crypto/crypto_dh.cc‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -319,12 +319,10 @@ void ComputeSecret(const FunctionCallbackInfo<Value>& args) {
319319
case DHPointer::CheckPublicKeyResult::CHECK_FAILED:
320320
return THROW_ERR_CRYPTO_INVALID_KEYTYPE(env,
321321
"Unspecified validation error");
322-
#ifndef OPENSSL_IS_BORINGSSL
323322
case DHPointer::CheckPublicKeyResult::TOO_SMALL:
324323
return THROW_ERR_CRYPTO_INVALID_KEYLEN(env, "Supplied key is too small");
325324
case DHPointer::CheckPublicKeyResult::TOO_LARGE:
326325
return THROW_ERR_CRYPTO_INVALID_KEYLEN(env, "Supplied key is too large");
327-
#endif
328326
case DHPointer::CheckPublicKeyResult::INVALID:
329327
return THROW_ERR_CRYPTO_INVALID_KEYTYPE(env, "Supplied key is invalid");
330328
case DHPointer::CheckPublicKeyResult::NONE:

‎src/crypto/crypto_rsa.cc‎

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

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

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

‎test/common/boringssl.js‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -137,12 +137,9 @@ function testRenegotiationUnsupported() {
137137
}
138138

139139
/**
140-
* OpenSSL exposes the negotiated ephemeral key type, name, and size for TLS
141-
* clients. With BoringSSL the same ECDHE TLS 1.2 handshake succeeds, but
142-
* getEphemeralKeyInfo() returns null on the server side and an object whose
143-
* fields are undefined on the client side.
140+
* BoringSSL exposes the negotiated TLS group but not the ephemeral key size.
144141
*/
145-
function testEphemeralKeyInfoUnsupported() {
142+
function testEphemeralKeyInfo() {
146143
const server = tls.createServer({
147144
key: fixtures.readKey('agent2-key.pem'),
148145
cert: fixtures.readKey('agent2-cert.pem'),
@@ -161,8 +158,8 @@ function testEphemeralKeyInfoUnsupported() {
161158
maxVersion: 'TLSv1.2',
162159
}, common.mustCall(() => {
163160
assert.deepStrictEqual(client.getEphemeralKeyInfo(), {
164-
type: undefined,
165-
name: undefined,
161+
type: 'TLSGroup',
162+
name: 'prime256v1',
166163
size: undefined,
167164
});
168165
server.close();
@@ -337,7 +334,7 @@ module.exports = {
337334
assertMultiKeyUnsupported,
338335
assertNoCipherMatch,
339336
assertOpenSSLSecurityLevelsUnsupported,
340-
testEphemeralKeyInfoUnsupported,
337+
testEphemeralKeyInfo,
341338
testLegacyProtocolUnsupported,
342339
testMultiPfxSelectionDifference,
343340
testPskTls13Unsupported,

‎test/parallel/test-crypto-dh-curves.js‎

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -35,33 +35,33 @@ if (!process.features.openssl_is_boringssl) {
3535
assert.strictEqual(
3636
crypto.createDiffieHellman(notSafePrime, Buffer.from([2])).verifyError,
3737
DH_CHECK_P_NOT_SAFE_PRIME);
38-
39-
const group = crypto.getDiffieHellman('modp14');
40-
const alice = crypto.createDiffieHellman(
41-
group.getPrime(), group.getGenerator());
42-
alice.generateKeys();
43-
const groupPrime = BigInt(`0x${group.getPrime('hex')}`);
44-
assert.throws(
45-
() => alice.computeSecret(Buffer.from([1])),
46-
{
47-
code: 'ERR_CRYPTO_INVALID_KEYLEN',
48-
message: 'Supplied key is too small'
49-
});
50-
assert.throws(
51-
() => alice.computeSecret(group.getPrime()),
52-
{
53-
code: 'ERR_CRYPTO_INVALID_KEYLEN',
54-
message: 'Supplied key is too large'
55-
});
56-
assert.throws(
57-
() => alice.computeSecret(
58-
Buffer.from((groupPrime - 1n).toString(16), 'hex')),
59-
{
60-
code: 'ERR_CRYPTO_INVALID_KEYLEN',
61-
message: 'Supplied key is too large'
62-
});
6338
}
6439

40+
const group = crypto.getDiffieHellman('modp14');
41+
const alice = crypto.createDiffieHellman(
42+
group.getPrime(), group.getGenerator());
43+
alice.generateKeys();
44+
const groupPrime = BigInt(`0x${group.getPrime('hex')}`);
45+
assert.throws(
46+
() => alice.computeSecret(Buffer.from([1])),
47+
{
48+
code: 'ERR_CRYPTO_INVALID_KEYLEN',
49+
message: 'Supplied key is too small'
50+
});
51+
assert.throws(
52+
() => alice.computeSecret(group.getPrime()),
53+
{
54+
code: 'ERR_CRYPTO_INVALID_KEYLEN',
55+
message: 'Supplied key is too large'
56+
});
57+
assert.throws(
58+
() => alice.computeSecret(
59+
Buffer.from((groupPrime - 1n).toString(16), 'hex')),
60+
{
61+
code: 'ERR_CRYPTO_INVALID_KEYLEN',
62+
message: 'Supplied key is too large'
63+
});
64+
6565
// Confirm DH_check() results are exposed for optional examination.
6666
const bad_dh = process.features.openssl_is_boringssl ?
6767
crypto.createDiffieHellman('abcd', 'hex', 0) :

‎test/parallel/test-crypto-dh.js‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,9 +93,7 @@ const {
9393
{
9494
assert.throws(() => {
9595
dh3.computeSecret('');
96-
}, { message: process.features.openssl_is_boringssl ?
97-
'Supplied key is invalid' :
98-
'Supplied key is too small' });
96+
}, { message: 'Supplied key is too small' });
9997
}
10098
}
10199

‎test/parallel/test-crypto-keygen.js‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -376,25 +376,20 @@ const isBoringSSL = process.features.openssl_is_boringssl;
376376
}
377377

378378
// Test invalid exponents. (caught by OpenSSL)
379+
let invalidExponentError = /bad e value/;
380+
if (isBoringSSL) {
381+
invalidExponentError = /BAD_E_VALUE/;
382+
} else if (hasOpenSSL3) {
383+
invalidExponentError = /exponent/;
384+
}
379385
for (const publicExponent of [1, 1 + 0x10001]) {
380-
if (isBoringSSL) {
381-
assert.throws(() => generateKeyPair('rsa', {
382-
modulusLength: 4096,
383-
publicExponent
384-
}, common.mustNotCall()), {
385-
name: 'RangeError',
386-
code: 'ERR_OUT_OF_RANGE',
387-
message: 'publicExponent is invalid',
388-
});
389-
} else {
390-
generateKeyPair('rsa', {
391-
modulusLength: 4096,
392-
publicExponent
393-
}, common.mustCall((err) => {
394-
assert.strictEqual(err.name, 'Error');
395-
assert.match(err.message, hasOpenSSL3 ? /exponent/ : /bad e value/);
396-
}));
397-
}
386+
generateKeyPair('rsa', {
387+
modulusLength: 4096,
388+
publicExponent
389+
}, common.mustCall((err) => {
390+
assert.strictEqual(err.name, 'Error');
391+
assert.match(err.message, invalidExponentError);
392+
}));
398393
}
399394
}
400395

‎test/parallel/test-crypto-sec-level.js‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,4 +15,8 @@ const assert = require('assert');
1515
// This test simply validates that we can get some value for the secLevel
1616
// when needed by tests.
1717
const secLevel = require('internal/crypto/util').getOpenSSLSecLevel();
18-
assert.ok(secLevel >= 0 && secLevel <= 5);
18+
if (process.features.openssl_is_boringssl) {
19+
assert.strictEqual(secLevel, 0);
20+
} else {
21+
assert.ok(secLevel >= 0 && secLevel <= 5);
22+
}

‎test/parallel/test-tls-client-getephemeralkeyinfo.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ if (!common.hasCrypto)
44
common.skip('missing crypto');
55

66
if (process.features.openssl_is_boringssl) {
7-
require('../common/boringssl').testEphemeralKeyInfoUnsupported();
7+
require('../common/boringssl').testEphemeralKeyInfo();
88
return;
99
}
1010

0 commit comments

Comments
 (0)