diff --git a/doc/api/errors.md b/doc/api/errors.md index 0161853774367d..44ec5548aeab35 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -186,10 +186,6 @@ circumstance of why the error occurred. `Error` objects capture a "stack trace" detailing the point in the code at which the `Error` was instantiated, and may provide a text description of the error. -For crypto only, `Error` objects will include the OpenSSL error stack in a -separate property called `opensslErrorStack` if it is available when the error -is thrown. - All errors generated by Node.js, including all System and JavaScript errors, will either be instances of, or inherit from, the `Error` class. @@ -589,6 +585,30 @@ program. For a comprehensive list, see the [`errno`(3) man page][]. encountered by [`http`][] or [`net`][] — often a sign that a `socket.end()` was not properly called. +## OpenSSL Errors + +Errors originating in `crypto` or `tls` are of class `Error`, and in addition to +the standard `.code` and `.message` properties, may have some additional +OpenSSL-specific properties. + +### error.opensslErrorStack + +An array of errors that can give context to where in the OpenSSL library an +error originates from. + +### error.function + +The OpenSSL function the error originates in. + +### error.library + +The OpenSSL library the error originates in. + +### error.reason + +A human-readable string describing the reason for the error. + + ## Node.js Error Codes @@ -1751,11 +1771,6 @@ Valid TLS protocol versions are `'TLSv1'`, `'TLSv1.1'`, or `'TLSv1.2'`. Attempting to set a TLS protocol `minVersion` or `maxVersion` conflicts with an attempt to set the `secureProtocol` explicitly. Use one mechanism or the other. - -### ERR_TLS_RENEGOTIATE - -An attempt to renegotiate the TLS session failed. - ### ERR_TLS_RENEGOTIATION_DISABLED diff --git a/doc/api/tls.md b/doc/api/tls.md index c86d2ac08768cb..ee390e1bdbc846 100644 --- a/doc/api/tls.md +++ b/doc/api/tls.md @@ -1046,9 +1046,10 @@ added: v0.11.8 `true`. * `requestCert` * `callback` {Function} If `renegotiate()` returned `true`, callback is - attached once to the `'secure'` event. If it returned `false`, it will be - called in the next tick with `ERR_TLS_RENEGOTIATE`, unless the `tlsSocket` - has been destroyed, in which case it will not be called at all. + attached once to the `'secure'` event. If `renegotiate()` returned `false`, + `callback` will be called in the next tick with an error, unless the + `tlsSocket` has been destroyed, in which case `callback` will not be called + at all. * Returns: {boolean} `true` if renegotiation was initiated, `false` otherwise. diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 5e0f648797d661..44a5994a8df9d5 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -48,7 +48,6 @@ const { ERR_SOCKET_CLOSED, ERR_TLS_DH_PARAM_SIZE, ERR_TLS_HANDSHAKE_TIMEOUT, - ERR_TLS_RENEGOTIATE, ERR_TLS_RENEGOTIATION_DISABLED, ERR_TLS_REQUIRED_SERVER_NAME, ERR_TLS_SESSION_ATTACK, @@ -661,9 +660,11 @@ TLSSocket.prototype.renegotiate = function(options, callback) { // Ensure that we'll cycle through internal openssl's state this.write(''); - if (!this._handle.renegotiate()) { + try { + this._handle.renegotiate(); + } catch (err) { if (callback) { - process.nextTick(callback, new ERR_TLS_RENEGOTIATE()); + process.nextTick(callback, err); } return false; } diff --git a/lib/internal/errors.js b/lib/internal/errors.js index e3347374cc702b..fee313a0f466e9 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1046,7 +1046,6 @@ E('ERR_TLS_INVALID_PROTOCOL_VERSION', '%j is not a valid %s TLS protocol version', TypeError); E('ERR_TLS_PROTOCOL_VERSION_CONFLICT', 'TLS protocol version %j conflicts with secureProtocol %j', TypeError); -E('ERR_TLS_RENEGOTIATE', 'Attempt to renegotiate TLS session failed', Error); E('ERR_TLS_RENEGOTIATION_DISABLED', 'TLS session renegotiation disabled for this socket', Error); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 91d06c8248b523..e963a50c500644 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -212,6 +212,113 @@ static int NoPasswordCallback(char* buf, int size, int rwflag, void* u) { } +// namespace node::crypto::error +namespace error { +void Decorate(Environment* env, Local obj, + unsigned long err) { // NOLINT(runtime/int) + if (err == 0) return; // No decoration possible. + + const char* ls = ERR_lib_error_string(err); + const char* fs = ERR_func_error_string(err); + const char* rs = ERR_reason_error_string(err); + + Isolate* isolate = env->isolate(); + Local context = isolate->GetCurrentContext(); + + if (ls != nullptr) { + if (obj->Set(context, env->library_string(), + OneByteString(isolate, ls)).IsNothing()) { + return; + } + } + if (fs != nullptr) { + if (obj->Set(context, env->function_string(), + OneByteString(isolate, fs)).IsNothing()) { + return; + } + } + if (rs != nullptr) { + if (obj->Set(context, env->reason_string(), + OneByteString(isolate, rs)).IsNothing()) { + return; + } + + // SSL has no API to recover the error name from the number, so we + // transform reason strings like "this error" to "ERR_SSL_THIS_ERROR", + // which ends up being close to the original error macro name. + std::string reason(rs); + + for (auto& c : reason) { + if (c == ' ') + c = '_'; + else + c = ToUpper(c); + } + +#define OSSL_ERROR_CODES_MAP(V) \ + V(SYS) \ + V(BN) \ + V(RSA) \ + V(DH) \ + V(EVP) \ + V(BUF) \ + V(OBJ) \ + V(PEM) \ + V(DSA) \ + V(X509) \ + V(ASN1) \ + V(CONF) \ + V(CRYPTO) \ + V(EC) \ + V(SSL) \ + V(BIO) \ + V(PKCS7) \ + V(X509V3) \ + V(PKCS12) \ + V(RAND) \ + V(DSO) \ + V(ENGINE) \ + V(OCSP) \ + V(UI) \ + V(COMP) \ + V(ECDSA) \ + V(ECDH) \ + V(OSSL_STORE) \ + V(FIPS) \ + V(CMS) \ + V(TS) \ + V(HMAC) \ + V(CT) \ + V(ASYNC) \ + V(KDF) \ + V(SM2) \ + V(USER) \ + +#define V(name) case ERR_LIB_##name: lib = #name "_"; break; + const char* lib = ""; + const char* prefix = "OSSL_"; + switch (ERR_GET_LIB(err)) { OSSL_ERROR_CODES_MAP(V) } +#undef V +#undef OSSL_ERROR_CODES_MAP + // Don't generate codes like "ERR_OSSL_SSL_". + if (lib && strcmp(lib, "SSL_") == 0) + prefix = ""; + + // All OpenSSL reason strings fit in a single 80-column macro definition, + // all prefix lengths are <= 10, and ERR_OSSL_ is 9, so 128 is more than + // sufficient. + char code[128]; + snprintf(code, sizeof(code), "ERR_%s%s%s", prefix, lib, reason.c_str()); + + if (obj->Set(env->isolate()->GetCurrentContext(), + env->code_string(), + OneByteString(env->isolate(), code)).IsNothing()) + return; + } +} +} // namespace error + + struct CryptoErrorVector : public std::vector { inline void Capture() { clear(); @@ -258,6 +365,8 @@ struct CryptoErrorVector : public std::vector { void ThrowCryptoError(Environment* env, unsigned long err, // NOLINT(runtime/int) + // Default, only used if there is no SSL `err` which can + // be used to create a long-style message string. const char* message = nullptr) { char message_buffer[128] = {0}; if (err != 0 || message == nullptr) { @@ -270,7 +379,11 @@ void ThrowCryptoError(Environment* env, .ToLocalChecked(); CryptoErrorVector errors; errors.Capture(); - auto exception = errors.ToException(env, exception_string); + Local exception = errors.ToException(env, exception_string); + Local obj; + if (!exception->ToObject(env->context()).ToLocal(&obj)) + return; + error::Decorate(env, obj, err); env->isolate()->ThrowException(exception); } @@ -2226,9 +2339,9 @@ void SSLWrap::Renegotiate(const FunctionCallbackInfo& args) { ClearErrorOnReturn clear_error_on_return; - // TODO(@sam-github) Return/throw an error, don't discard the SSL error info. - bool yes = SSL_renegotiate(w->ssl_.get()) == 1; - args.GetReturnValue().Set(yes); + if (SSL_renegotiate(w->ssl_.get()) != 1) { + return ThrowCryptoError(w->ssl_env(), ERR_get_error()); + } } diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 59400d8f3a4a72..14802b1dfb74b8 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -453,7 +453,7 @@ Local TLSWrap::GetSSLError(int status, int* err, std::string* msg) { if (c == ' ') c = '_'; else - c = ::toupper(c); + c = ToUpper(c); } obj->Set(context, env()->code_string(), OneByteString(isolate, ("ERR_SSL_" + code).c_str())) diff --git a/src/util-inl.h b/src/util-inl.h index 5b59f25bc5ff16..99d470205e1ce7 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -274,6 +274,17 @@ std::string ToLower(const std::string& in) { return out; } +char ToUpper(char c) { + return c >= 'a' && c <= 'z' ? (c - 'a') + 'A' : c; +} + +std::string ToUpper(const std::string& in) { + std::string out(in.size(), 0); + for (size_t i = 0; i < in.size(); ++i) + out[i] = ToUpper(in[i]); + return out; +} + bool StringEqualNoCase(const char* a, const char* b) { do { if (*a == '\0') diff --git a/src/util.h b/src/util.h index 56b2bc83382698..a9de8f8636c11b 100644 --- a/src/util.h +++ b/src/util.h @@ -284,6 +284,10 @@ inline void SwapBytes64(char* data, size_t nbytes); inline char ToLower(char c); inline std::string ToLower(const std::string& in); +// toupper() is locale-sensitive. Use ToUpper() instead. +inline char ToUpper(char c); +inline std::string ToUpper(const std::string& in); + // strcasecmp() is locale-sensitive. Use StringEqualNoCase() instead. inline bool StringEqualNoCase(const char* a, const char* b); diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js index ead77d6a30c811..69d18f458e60bd 100644 --- a/test/parallel/test-crypto-dh.js +++ b/test/parallel/test-crypto-dh.js @@ -91,8 +91,13 @@ const secret4 = dh4.computeSecret(key2, 'hex', 'base64'); assert.strictEqual(secret1, secret4); -const wrongBlockLength = - /^Error: error:0606506D:digital envelope routines:EVP_DecryptFinal_ex:wrong final block length$/; +const wrongBlockLength = { + message: 'error:0606506D:digital envelope' + + ' routines:EVP_DecryptFinal_ex:wrong final block length', + code: 'ERR_OSSL_EVP_WRONG_FINAL_BLOCK_LENGTH', + library: 'digital envelope routines', + reason: 'wrong final block length' +}; // Run this one twice to make sure that the dh3 clears its error properly { @@ -111,7 +116,7 @@ const wrongBlockLength = assert.throws(() => { dh3.computeSecret(''); -}, /^Error: Supplied key is too small$/); +}, { message: 'Supplied key is too small' }); // Create a shared using a DH group. const alice = crypto.createDiffieHellmanGroup('modp5'); @@ -260,7 +265,7 @@ if (availableCurves.has('prime256v1') && availableCurves.has('secp256k1')) { assert.throws(() => { ecdh4.setPublicKey(ecdh3.getPublicKey()); - }, /^Error: Failed to convert Buffer to EC_POINT$/); + }, { message: 'Failed to convert Buffer to EC_POINT' }); // Verify that we can use ECDH without having to use newly generated keys. const ecdh5 = crypto.createECDH('secp256k1'); diff --git a/test/parallel/test-crypto-key-objects.js b/test/parallel/test-crypto-key-objects.js index fb1c3de1d7e224..c9cb90d408500f 100644 --- a/test/parallel/test-crypto-key-objects.js +++ b/test/parallel/test-crypto-key-objects.js @@ -165,7 +165,13 @@ const privatePem = fixtures.readSync('test_rsa_privkey.pem', 'ascii'); // This should not cause a crash: https://github.com/nodejs/node/issues/25247 assert.throws(() => { createPrivateKey({ key: '' }); - }, /null/); + }, { + message: 'error:2007E073:BIO routines:BIO_new_mem_buf:null parameter', + code: 'ERR_OSSL_BIO_NULL_PARAMETER', + reason: 'null parameter', + library: 'BIO routines', + function: 'BIO_new_mem_buf', + }); } [ diff --git a/test/parallel/test-crypto-padding.js b/test/parallel/test-crypto-padding.js index d4a5b95cec9242..909c014bd0f87a 100644 --- a/test/parallel/test-crypto-padding.js +++ b/test/parallel/test-crypto-padding.js @@ -82,7 +82,12 @@ assert.strictEqual(enc(EVEN_LENGTH_PLAIN, true), EVEN_LENGTH_ENCRYPTED); assert.throws(function() { // Input must have block length %. enc(ODD_LENGTH_PLAIN, false); -}, /data not multiple of block length/); +}, { + message: 'error:0607F08A:digital envelope routines:EVP_EncryptFinal_ex:' + + 'data not multiple of block length', + code: 'ERR_OSSL_EVP_DATA_NOT_MULTIPLE_OF_BLOCK_LENGTH', + reason: 'data not multiple of block length', +}); assert.strictEqual( enc(EVEN_LENGTH_PLAIN, false), EVEN_LENGTH_ENCRYPTED_NOPAD @@ -99,7 +104,12 @@ assert.strictEqual(dec(EVEN_LENGTH_ENCRYPTED, false).length, 48); assert.throws(function() { // Must have at least 1 byte of padding (PKCS): assert.strictEqual(dec(EVEN_LENGTH_ENCRYPTED_NOPAD, true), EVEN_LENGTH_PLAIN); -}, /bad decrypt/); +}, { + message: 'error:06065064:digital envelope routines:EVP_DecryptFinal_ex:' + + 'bad decrypt', + reason: 'bad decrypt', + code: 'ERR_OSSL_EVP_BAD_DECRYPT', +}); // No-pad encrypted string should return the same: assert.strictEqual( diff --git a/test/parallel/test-crypto-rsa-dsa.js b/test/parallel/test-crypto-rsa-dsa.js index 348fd15b74d495..69b0a7af09de5b 100644 --- a/test/parallel/test-crypto-rsa-dsa.js +++ b/test/parallel/test-crypto-rsa-dsa.js @@ -22,8 +22,14 @@ const dsaKeyPem = fixtures.readSync('test_dsa_privkey.pem', 'ascii'); const dsaKeyPemEncrypted = fixtures.readSync('test_dsa_privkey_encrypted.pem', 'ascii'); -const decryptError = - /^Error: error:06065064:digital envelope routines:EVP_DecryptFinal_ex:bad decrypt$/; +const decryptError = { + message: 'error:06065064:digital envelope routines:EVP_DecryptFinal_ex:' + + 'bad decrypt', + code: 'ERR_OSSL_EVP_BAD_DECRYPT', + reason: 'bad decrypt', + function: 'EVP_DecryptFinal_ex', + library: 'digital envelope routines', +}; // Test RSA encryption/decryption { diff --git a/test/parallel/test-crypto-stream.js b/test/parallel/test-crypto-stream.js index 9430084bbee179..2d005c89db3f09 100644 --- a/test/parallel/test-crypto-stream.js +++ b/test/parallel/test-crypto-stream.js @@ -71,8 +71,11 @@ const cipher = crypto.createCipheriv('aes-128-cbc', key, iv); const decipher = crypto.createDecipheriv('aes-128-cbc', badkey, iv); cipher.pipe(decipher) - .on('error', common.mustCall(function end(err) { - assert(/bad decrypt/.test(err)); + .on('error', common.expectsError({ + message: /bad decrypt/, + function: 'EVP_DecryptFinal_ex', + library: 'digital envelope routines', + reason: 'bad decrypt', })); cipher.end('Papaya!'); // Should not cause an unhandled exception. diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index 005a733c0779d9..1952f8908acb03 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -238,15 +238,19 @@ assert.throws(function() { // This would inject errors onto OpenSSL's error stack crypto.createSign('sha1').sign(sha1_privateKey); }, (err) => { + // Do the standard checks, but then do some custom checks afterwards. + assert.throws(() => { throw err; }, { + message: 'error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag', + library: 'asn1 encoding routines', + function: 'asn1_check_tlen', + reason: 'wrong tag', + code: 'ERR_OSSL_ASN1_WRONG_TAG', + }); // Throws crypto error, so there is an opensslErrorStack property. // The openSSL stack should have content. - if ((err instanceof Error) && - /asn1 encoding routines:[^:]*:wrong tag/.test(err) && - err.opensslErrorStack !== undefined && - Array.isArray(err.opensslErrorStack) && - err.opensslErrorStack.length > 0) { - return true; - } + assert(Array.isArray(err.opensslErrorStack)); + assert(err.opensslErrorStack.length > 0); + return true; }); // Make sure memory isn't released before being returned diff --git a/test/parallel/test-tls-client-renegotiation-13.js b/test/parallel/test-tls-client-renegotiation-13.js index 8af63c4f791ddc..075eb70e917c06 100644 --- a/test/parallel/test-tls-client-renegotiation-13.js +++ b/test/parallel/test-tls-client-renegotiation-13.js @@ -28,8 +28,12 @@ connect({ assert.strictEqual(client.getProtocol(), 'TLSv1.3'); const ok = client.renegotiate({}, common.mustCall((err) => { - assert(err.code, 'ERR_TLS_RENEGOTIATE'); - assert(err.message, 'Attempt to renegotiate TLS session failed'); + assert.throws(() => { throw err; }, { + message: 'error:1420410A:SSL routines:SSL_renegotiate:wrong ssl version', + code: 'ERR_SSL_WRONG_SSL_VERSION', + library: 'SSL routines', + reason: 'wrong ssl version', + }); cleanup(); }));