From f2c9e4957473f4a0033698b366aa1993fa3095f5 Mon Sep 17 00:00:00 2001 From: Greg Alexander Date: Wed, 9 Aug 2017 22:26:03 -0500 Subject: [PATCH 1/8] crypto: better crypto error messages Feature request to add openSSL error stack to the exception object thrown from crypto. New exception property only added to object if the error stack has not cleared out prior to calling ThrowCryptoError. Refs: https://github.com/nodejs/node/issues/5444 --- src/node_crypto.cc | 50 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index fe79b14042296f..73137f272fc338 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -255,13 +255,53 @@ void ThrowCryptoError(Environment* env, unsigned long err, // NOLINT(runtime/int) const char* default_message = nullptr) { HandleScope scope(env->isolate()); + Local message; + if (err != 0 || default_message == nullptr) { char errmsg[128] = { 0 }; ERR_error_string_n(err, errmsg, sizeof(errmsg)); - env->ThrowError(errmsg); + message = String::NewFromUtf8(env->isolate(), errmsg, + v8::NewStringType::kInternalized) + .ToLocalChecked(); } else { - env->ThrowError(default_message); + message = String::NewFromUtf8(env->isolate(), default_message, + v8::NewStringType::kInternalized) + .ToLocalChecked(); + } + + Local exception_v = Exception::Error(message); + CHECK(!exception_v.IsEmpty()); + // add the openSSLErrorStack property + Local exception = exception_v.As(); + Local key = String::NewFromUtf8(env->isolate(), "openSSLErrorStack", + v8::NewStringType::kInternalized) + .ToLocalChecked(); + Local errorStack = Array::New(env->isolate()); + + // get the error state + // will only add to errorStack if state has not been cleared out + ERR_STATE *es; + es = ERR_get_state(); + + // add content to the openssl error stack + unsigned int i = 0; + while (es->bottom != es->top + && (es->err_flags[es->top] & ERR_FLAG_MARK) == 0) { + unsigned long err_buf = es->err_buffer[es->top]; // NOLINT(runtime/int) + // only add if there is valid err_buffer + if (err_buf) { + char tmpStr[128] = { 0 }; + ERR_error_string_n(err_buf, tmpStr, sizeof(tmpStr)); + errorStack->Set(i, String::NewFromUtf8(env->isolate(), tmpStr, + v8::NewStringType::kInternalized) + .ToLocalChecked()); + } + i++; + es->top -= 1; } + + exception->Set(env->context(), key, errorStack).FromJust(); + env->isolate()->ThrowException(exception); } @@ -4278,7 +4318,7 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem, if (!initialised_) return kSignNotInitialised; - ClearErrorOnReturn clear_error_on_return; + // ClearErrorOnReturn clear_error_on_return; EVP_PKEY* pkey = nullptr; BIO* bp = nullptr; @@ -4289,6 +4329,8 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem, int r = 0; EVP_PKEY_CTX* pkctx = nullptr; + ERR_set_mark(); + bp = BIO_new_mem_buf(const_cast(key_pem), key_pem_len); if (bp == nullptr) goto exit; @@ -4368,6 +4410,8 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem, void Verify::VerifyFinal(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); + ClearErrorOnReturn clear_error_on_return; + Verify* verify; ASSIGN_OR_RETURN_UNWRAP(&verify, args.Holder()); From dc2165a0f7f671b1b830c1794fd336c9a7e9bf0a Mon Sep 17 00:00:00 2001 From: Greg Alexander Date: Fri, 11 Aug 2017 16:19:30 -0500 Subject: [PATCH 2/8] Review updates and tests --- doc/api/errors.md | 4 +- src/node_crypto.cc | 31 +++++----- test/parallel/test-crypto.js | 116 +++++++++++++++++++++++++++++++---- 3 files changed, 122 insertions(+), 29 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index ba81f2f75eee5b..7ccfa5407801a8 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -190,7 +190,9 @@ intercepted. A generic JavaScript `Error` object that does not denote any specific 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. +provide a text description of the error. For crypto only, `Error` objects will +include the OpenSSL error stack, if it's available when the error is thrown, in +a separate property. All errors generated by Node.js, including all System and JavaScript errors, will either be instances of, or inherit from, the `Error` class. diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 73137f272fc338..5e91c4b34e3401 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -261,45 +261,44 @@ void ThrowCryptoError(Environment* env, char errmsg[128] = { 0 }; ERR_error_string_n(err, errmsg, sizeof(errmsg)); message = String::NewFromUtf8(env->isolate(), errmsg, - v8::NewStringType::kInternalized) + v8::NewStringType::kNormal) .ToLocalChecked(); } else { message = String::NewFromUtf8(env->isolate(), default_message, - v8::NewStringType::kInternalized) + v8::NewStringType::kNormal) .ToLocalChecked(); } Local exception_v = Exception::Error(message); CHECK(!exception_v.IsEmpty()); - // add the openSSLErrorStack property + // Add the openSSLErrorStack property to the exception object. Local exception = exception_v.As(); Local key = String::NewFromUtf8(env->isolate(), "openSSLErrorStack", v8::NewStringType::kInternalized) .ToLocalChecked(); Local errorStack = Array::New(env->isolate()); - // get the error state - // will only add to errorStack if state has not been cleared out - ERR_STATE *es; - es = ERR_get_state(); - - // add content to the openssl error stack - unsigned int i = 0; - while (es->bottom != es->top - && (es->err_flags[es->top] & ERR_FLAG_MARK) == 0) { + ERR_STATE *es = ERR_get_state(); + // Build the errorStack array to be added to openSSLErrorStack property. + for (unsigned int i = 0; es->bottom != es->top + && (es->err_flags[es->top] & ERR_FLAG_MARK) == 0; i++) { unsigned long err_buf = es->err_buffer[es->top]; // NOLINT(runtime/int) - // only add if there is valid err_buffer + // Only add error string if there is valid err_buffer. if (err_buf) { - char tmpStr[128] = { 0 }; + char tmpStr[256] = { }; ERR_error_string_n(err_buf, tmpStr, sizeof(tmpStr)); errorStack->Set(i, String::NewFromUtf8(env->isolate(), tmpStr, - v8::NewStringType::kInternalized) + v8::NewStringType::kNormal) .ToLocalChecked()); } - i++; es->top -= 1; } + // Adding the new property that will look like the following: + // openSSLErrorStack: [ + // 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib', + // 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 error' + // ] exception->Set(env->context(), key, errorStack).FromJust(); env->isolate()->ThrowException(exception); } diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index 2a704ff07c8c3f..c9ed19c465ccf0 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -46,7 +46,14 @@ assert.throws(function() { const context = credentials.context; const notcontext = { setOptions: context.setOptions, setKey: context.setKey }; tls.createSecureContext({ secureOptions: 1 }, notcontext); -}, /^TypeError: Illegal invocation$/); +}, (err) => { + // Throws TypeError, so there is no openSSLErrorStack property. + if ((err instanceof Error) && + /^TypeError: Illegal invocation$/.test(err) && + err.openSSLErrorStack === undefined) { + return true; + } +}); // PFX tests assert.doesNotThrow(function() { @@ -55,15 +62,36 @@ assert.doesNotThrow(function() { assert.throws(function() { tls.createSecureContext({ pfx: certPfx }); -}, /^Error: mac verify failure$/); +}, (err) => { + // Throws general Error, so there is no openSSLErrorStack property. + if ((err instanceof Error) && + /^Error: mac verify failure$/.test(err) && + err.openSSLErrorStack === undefined) { + return true; + } +}); assert.throws(function() { tls.createSecureContext({ pfx: certPfx, passphrase: 'test' }); -}, /^Error: mac verify failure$/); +}, (err) => { + // Throws general Error, so there is no openSSLErrorStack property. + if ((err instanceof Error) && + /^Error: mac verify failure$/.test(err) && + err.openSSLErrorStack === undefined) { + return true; + } +}); assert.throws(function() { tls.createSecureContext({ pfx: 'sample', passphrase: 'test' }); -}, /^Error: not enough data$/); +}, (err) => { + // Throws general Error, so there is no openSSLErrorStack property. + if ((err instanceof Error) && + /^Error: not enough data$/.test(err) && + err.openSSLErrorStack === undefined) { + return true; + } +}); // update() should only take buffers / strings @@ -138,23 +166,62 @@ testImmutability(crypto.getCurves); // throw, not assert in C++ land. assert.throws(function() { crypto.createCipher('aes192', 'test').update('0', 'hex'); -}, common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/); +}, (err) => { + const errorMessage = + common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/; + // Throws general Error, so there is no openSSLErrorStack property. + if ((err instanceof Error) && + errorMessage.test(err) && + err.openSSLErrorStack === undefined) { + return true; + } +}); assert.throws(function() { crypto.createDecipher('aes192', 'test').update('0', 'hex'); -}, common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/); +}, (err) => { + const errorMessage = + common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/; + // Throws general Error, so there is no openSSLErrorStack property. + if ((err instanceof Error) && + errorMessage.test(err) && + err.openSSLErrorStack === undefined) { + return true; + } +}); assert.throws(function() { crypto.createHash('sha1').update('0', 'hex'); -}, /^TypeError: Bad input string$/); +}, (err) => { + // Throws TypeError, so there is no openSSLErrorStack property. + if ((err instanceof Error) && + /^TypeError: Bad input string$/.test(err) && + err.openSSLErrorStack === undefined) { + return true; + } +}); assert.throws(function() { crypto.createSign('SHA1').update('0', 'hex'); -}, /^TypeError: Bad input string$/); +}, (err) => { + // Throws TypeError, so there is no openSSLErrorStack property. + if ((err instanceof Error) && + /^TypeError: Bad input string$/.test(err) && + err.openSSLErrorStack === undefined) { + return true; + } +}); assert.throws(function() { crypto.createVerify('SHA1').update('0', 'hex'); -}, /^TypeError: Bad input string$/); +}, (err) => { + // Throws TypeError, so there is no openSSLErrorStack property. + if ((err instanceof Error) && + /^TypeError: Bad input string$/.test(err) && + err.openSSLErrorStack === undefined) { + return true; + } +}); assert.throws(function() { const priv = [ @@ -167,7 +234,16 @@ assert.throws(function() { '' ].join('\n'); crypto.createSign('SHA256').update('test').sign(priv); -}, /digest too big for rsa key$/); +}, (err) => { + // Throws crypto error, so there is an openSSLErrorStack property. + // The openSSL stack is empty. + if ((err instanceof Error) && + /digest too big for rsa key$/.test(err) && + err.openSSLErrorStack !== undefined && + err.openSSLErrorStack.length === 0) { + return true; + } +}); assert.throws(function() { // The correct header inside `test_bad_rsa_privkey.pem` should have been @@ -183,14 +259,30 @@ assert.throws(function() { 'ascii'); // this would inject errors onto OpenSSL's error stack crypto.createSign('sha1').sign(sha1_privateKey); -}, /asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag/); +}, (err) => { + // Throws crypto error, so there is an openSSLErrorStack property. + // The openSSL stack should have content. + if ((err instanceof Error) && + /asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag/.test(err) && + err.openSSLErrorStack !== undefined && + err.openSSLErrorStack.length > 0) { + return true; + } +}); // Make sure memory isn't released before being returned console.log(crypto.randomBytes(16)); assert.throws(function() { tls.createSecureContext({ crl: 'not a CRL' }); -}, /^Error: Failed to parse CRL$/); +}, (err) => { + // Throws general error, so there is no openSSLErrorStack property. + if ((err instanceof Error) && + /^Error: Failed to parse CRL$/.test(err) && + err.openSSLErrorStack === undefined) { + return true; + } +}); /** * Check if the stream function uses utf8 as a default encoding. From 5c24f07ab7f9081a6069d39e2e3c124d2adf4443 Mon Sep 17 00:00:00 2001 From: Greg Alexander Date: Mon, 14 Aug 2017 20:51:24 -0500 Subject: [PATCH 3/8] Additional review updates. --- doc/api/errors.md | 7 ++++--- src/env.h | 1 + src/node_crypto.cc | 15 ++++++--------- test/parallel/test-crypto.js | 2 ++ 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 7ccfa5407801a8..00cfe73cefc971 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -190,9 +190,10 @@ intercepted. A generic JavaScript `Error` object that does not denote any specific 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, if it's available when the error is thrown, in -a separate property. +provide a text description of the error. + +For crypto only, `Error` objects will include the OpenSSL error stack in a +separate property 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. diff --git a/src/env.h b/src/env.h index ef4874057a8fd8..7674dcb428b446 100644 --- a/src/env.h +++ b/src/env.h @@ -224,6 +224,7 @@ class ModuleWrap; V(onstreamclose_string, "onstreamclose") \ V(ontrailers_string, "ontrailers") \ V(onwrite_string, "onwrite") \ + V(openssl_error_stack, "openSSLErrorStack") \ V(output_string, "output") \ V(order_string, "order") \ V(owner_string, "owner") \ diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 5e91c4b34e3401..c2fd90685699fd 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -262,20 +262,16 @@ void ThrowCryptoError(Environment* env, ERR_error_string_n(err, errmsg, sizeof(errmsg)); message = String::NewFromUtf8(env->isolate(), errmsg, v8::NewStringType::kNormal) - .ToLocalChecked(); + .ToLocalChecked(); } else { message = String::NewFromUtf8(env->isolate(), default_message, v8::NewStringType::kNormal) - .ToLocalChecked(); + .ToLocalChecked(); } Local exception_v = Exception::Error(message); CHECK(!exception_v.IsEmpty()); - // Add the openSSLErrorStack property to the exception object. Local exception = exception_v.As(); - Local key = String::NewFromUtf8(env->isolate(), "openSSLErrorStack", - v8::NewStringType::kInternalized) - .ToLocalChecked(); Local errorStack = Array::New(env->isolate()); ERR_STATE *es = ERR_get_state(); @@ -289,17 +285,18 @@ void ThrowCryptoError(Environment* env, ERR_error_string_n(err_buf, tmpStr, sizeof(tmpStr)); errorStack->Set(i, String::NewFromUtf8(env->isolate(), tmpStr, v8::NewStringType::kNormal) - .ToLocalChecked()); + .ToLocalChecked()); } es->top -= 1; } - // Adding the new property that will look like the following: + // Add the openSSLErrorStack property to the exception object. + // The new property will look like the following: // openSSLErrorStack: [ // 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib', // 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 error' // ] - exception->Set(env->context(), key, errorStack).FromJust(); + exception->Set(env->openssl_error_stack(), errorStack); env->isolate()->ThrowException(exception); } diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index c9ed19c465ccf0..c8ccc296cffada 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -240,6 +240,7 @@ assert.throws(function() { if ((err instanceof Error) && /digest too big for rsa key$/.test(err) && err.openSSLErrorStack !== undefined && + Array.isArray(err.openSSLErrorStack) && err.openSSLErrorStack.length === 0) { return true; } @@ -265,6 +266,7 @@ assert.throws(function() { if ((err instanceof Error) && /asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag/.test(err) && err.openSSLErrorStack !== undefined && + Array.isArray(err.openSSLErrorStack) && err.openSSLErrorStack.length > 0) { return true; } From d1296cfbcd38994418404cbdfa26f02375fb1964 Mon Sep 17 00:00:00 2001 From: Greg Alexander Date: Tue, 15 Aug 2017 16:38:47 -0500 Subject: [PATCH 4/8] Review update. Use the overload of Set() --- src/node_crypto.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c2fd90685699fd..394e60f04ac8db 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -283,9 +283,10 @@ void ThrowCryptoError(Environment* env, if (err_buf) { char tmpStr[256] = { }; ERR_error_string_n(err_buf, tmpStr, sizeof(tmpStr)); - errorStack->Set(i, String::NewFromUtf8(env->isolate(), tmpStr, + errorStack->Set(env->context(), i, + String::NewFromUtf8(env->isolate(), tmpStr, v8::NewStringType::kNormal) - .ToLocalChecked()); + .ToLocalChecked()).FromJust(); } es->top -= 1; } From 2b177cbeaa2e42b5d425763a26fd9d520fd07173 Mon Sep 17 00:00:00 2001 From: Greg Alexander Date: Wed, 16 Aug 2017 08:46:20 -0500 Subject: [PATCH 5/8] Nit fix: changing local variable to snake_case --- src/node_crypto.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 394e60f04ac8db..4be19285d49a42 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -272,10 +272,10 @@ void ThrowCryptoError(Environment* env, Local exception_v = Exception::Error(message); CHECK(!exception_v.IsEmpty()); Local exception = exception_v.As(); - Local errorStack = Array::New(env->isolate()); + Local error_stack = Array::New(env->isolate()); ERR_STATE *es = ERR_get_state(); - // Build the errorStack array to be added to openSSLErrorStack property. + // Build the error_stack array to be added to openSSLErrorStack property. for (unsigned int i = 0; es->bottom != es->top && (es->err_flags[es->top] & ERR_FLAG_MARK) == 0; i++) { unsigned long err_buf = es->err_buffer[es->top]; // NOLINT(runtime/int) @@ -283,7 +283,7 @@ void ThrowCryptoError(Environment* env, if (err_buf) { char tmpStr[256] = { }; ERR_error_string_n(err_buf, tmpStr, sizeof(tmpStr)); - errorStack->Set(env->context(), i, + error_stack->Set(env->context(), i, String::NewFromUtf8(env->isolate(), tmpStr, v8::NewStringType::kNormal) .ToLocalChecked()).FromJust(); @@ -297,7 +297,7 @@ void ThrowCryptoError(Environment* env, // 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib', // 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 error' // ] - exception->Set(env->openssl_error_stack(), errorStack); + exception->Set(env->openssl_error_stack(), error_stack); env->isolate()->ThrowException(exception); } From bbaea8b461bef9e8ae59dcb96b3c0b85a886f866 Mon Sep 17 00:00:00 2001 From: Greg Alexander Date: Wed, 20 Sep 2017 21:04:53 -0500 Subject: [PATCH 6/8] PR review updates --- doc/api/errors.md | 2 +- src/node_crypto.cc | 65 +++++++++++++++++++++--------------- test/parallel/test-crypto.js | 6 +--- 3 files changed, 40 insertions(+), 33 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 00cfe73cefc971..bb7bd5ac2ebfc6 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -193,7 +193,7 @@ 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 if it is available when the error is thrown. +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. diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 4be19285d49a42..8bb03e10d9ff61 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -272,32 +272,47 @@ void ThrowCryptoError(Environment* env, Local exception_v = Exception::Error(message); CHECK(!exception_v.IsEmpty()); Local exception = exception_v.As(); - Local error_stack = Array::New(env->isolate()); - - ERR_STATE *es = ERR_get_state(); - // Build the error_stack array to be added to openSSLErrorStack property. - for (unsigned int i = 0; es->bottom != es->top - && (es->err_flags[es->top] & ERR_FLAG_MARK) == 0; i++) { - unsigned long err_buf = es->err_buffer[es->top]; // NOLINT(runtime/int) - // Only add error string if there is valid err_buffer. - if (err_buf) { - char tmpStr[256] = { }; - ERR_error_string_n(err_buf, tmpStr, sizeof(tmpStr)); - error_stack->Set(env->context(), i, - String::NewFromUtf8(env->isolate(), tmpStr, - v8::NewStringType::kNormal) - .ToLocalChecked()).FromJust(); + ERR_STATE* es = ERR_get_state(); + + if (es->bottom != es->top) { + Local error_stack = Array::New(env->isolate()); + int top = es->top; + + // Build the error_stack array to be added to openSSLErrorStack property. + for (unsigned int i = 0; es->bottom != es->top;) { + unsigned long err_buf = es->err_buffer[es->top]; // NOLINT(runtime/int) + // Only add error string if there is valid err_buffer. + if (err_buf) { + char tmp_str[256]; + ERR_error_string_n(err_buf, tmp_str, sizeof(tmp_str)); + error_stack->Set(env->context(), i, + String::NewFromUtf8(env->isolate(), tmp_str, + v8::NewStringType::kNormal) + .ToLocalChecked()).FromJust(); + // Only increment if we added to error_stack. + i++; + } + + // Since the ERR_STATE is a ring buffer, we need to use modular + // arithmetic to loop back around in the case where bottom is after top. + // Using ERR_NUM_ERRORS macro defined in openssl. + es->top = (((es->top - 1) % ERR_NUM_ERRORS) + ERR_NUM_ERRORS) % + ERR_NUM_ERRORS; } - es->top -= 1; + + // Restore top. + es->top = top; + + // Add the openSSLErrorStack property to the exception object. + // The new property will look like the following: + // openSSLErrorStack: [ + // 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib', + // 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 err' + // ] + exception->Set(env->context(), env->openssl_error_stack(), error_stack) + .FromJust(); } - // Add the openSSLErrorStack property to the exception object. - // The new property will look like the following: - // openSSLErrorStack: [ - // 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib', - // 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 error' - // ] - exception->Set(env->openssl_error_stack(), error_stack); env->isolate()->ThrowException(exception); } @@ -4315,8 +4330,6 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem, if (!initialised_) return kSignNotInitialised; - // ClearErrorOnReturn clear_error_on_return; - EVP_PKEY* pkey = nullptr; BIO* bp = nullptr; X509* x509 = nullptr; @@ -4326,8 +4339,6 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem, int r = 0; EVP_PKEY_CTX* pkctx = nullptr; - ERR_set_mark(); - bp = BIO_new_mem_buf(const_cast(key_pem), key_pem_len); if (bp == nullptr) goto exit; diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index c8ccc296cffada..c5b62d2f61bb11 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -235,13 +235,9 @@ assert.throws(function() { ].join('\n'); crypto.createSign('SHA256').update('test').sign(priv); }, (err) => { - // Throws crypto error, so there is an openSSLErrorStack property. - // The openSSL stack is empty. if ((err instanceof Error) && /digest too big for rsa key$/.test(err) && - err.openSSLErrorStack !== undefined && - Array.isArray(err.openSSLErrorStack) && - err.openSSLErrorStack.length === 0) { + err.openSSLErrorStack === undefined) { return true; } }); From a5a2f64b94467836a53c91a1d1058af1213f7319 Mon Sep 17 00:00:00 2001 From: Greg Alexander Date: Fri, 22 Sep 2017 09:10:52 -0500 Subject: [PATCH 7/8] changing openSSLErrorStack property name to opensslErrorStack --- doc/api/errors.md | 2 +- src/env.h | 2 +- src/node_crypto.cc | 6 ++--- test/parallel/test-crypto.js | 50 ++++++++++++++++++------------------ 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index bb7bd5ac2ebfc6..0a4b08bebecc36 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -193,7 +193,7 @@ 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. +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. diff --git a/src/env.h b/src/env.h index 7674dcb428b446..786f87c4374f05 100644 --- a/src/env.h +++ b/src/env.h @@ -224,7 +224,7 @@ class ModuleWrap; V(onstreamclose_string, "onstreamclose") \ V(ontrailers_string, "ontrailers") \ V(onwrite_string, "onwrite") \ - V(openssl_error_stack, "openSSLErrorStack") \ + V(openssl_error_stack, "opensslErrorStack") \ V(output_string, "output") \ V(order_string, "order") \ V(owner_string, "owner") \ diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 8bb03e10d9ff61..8989740b5012c9 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -278,7 +278,7 @@ void ThrowCryptoError(Environment* env, Local error_stack = Array::New(env->isolate()); int top = es->top; - // Build the error_stack array to be added to openSSLErrorStack property. + // Build the error_stack array to be added to opensslErrorStack property. for (unsigned int i = 0; es->bottom != es->top;) { unsigned long err_buf = es->err_buffer[es->top]; // NOLINT(runtime/int) // Only add error string if there is valid err_buffer. @@ -303,9 +303,9 @@ void ThrowCryptoError(Environment* env, // Restore top. es->top = top; - // Add the openSSLErrorStack property to the exception object. + // Add the opensslErrorStack property to the exception object. // The new property will look like the following: - // openSSLErrorStack: [ + // opensslErrorStack: [ // 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib', // 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 err' // ] diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index c5b62d2f61bb11..d501991dd2f8fb 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -47,10 +47,10 @@ assert.throws(function() { const notcontext = { setOptions: context.setOptions, setKey: context.setKey }; tls.createSecureContext({ secureOptions: 1 }, notcontext); }, (err) => { - // Throws TypeError, so there is no openSSLErrorStack property. + // Throws TypeError, so there is no opensslErrorStack property. if ((err instanceof Error) && /^TypeError: Illegal invocation$/.test(err) && - err.openSSLErrorStack === undefined) { + err.opensslErrorStack === undefined) { return true; } }); @@ -63,10 +63,10 @@ assert.doesNotThrow(function() { assert.throws(function() { tls.createSecureContext({ pfx: certPfx }); }, (err) => { - // Throws general Error, so there is no openSSLErrorStack property. + // Throws general Error, so there is no opensslErrorStack property. if ((err instanceof Error) && /^Error: mac verify failure$/.test(err) && - err.openSSLErrorStack === undefined) { + err.opensslErrorStack === undefined) { return true; } }); @@ -74,10 +74,10 @@ assert.throws(function() { assert.throws(function() { tls.createSecureContext({ pfx: certPfx, passphrase: 'test' }); }, (err) => { - // Throws general Error, so there is no openSSLErrorStack property. + // Throws general Error, so there is no opensslErrorStack property. if ((err instanceof Error) && /^Error: mac verify failure$/.test(err) && - err.openSSLErrorStack === undefined) { + err.opensslErrorStack === undefined) { return true; } }); @@ -85,10 +85,10 @@ assert.throws(function() { assert.throws(function() { tls.createSecureContext({ pfx: 'sample', passphrase: 'test' }); }, (err) => { - // Throws general Error, so there is no openSSLErrorStack property. + // Throws general Error, so there is no opensslErrorStack property. if ((err instanceof Error) && /^Error: not enough data$/.test(err) && - err.openSSLErrorStack === undefined) { + err.opensslErrorStack === undefined) { return true; } }); @@ -169,10 +169,10 @@ assert.throws(function() { }, (err) => { const errorMessage = common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/; - // Throws general Error, so there is no openSSLErrorStack property. + // Throws general Error, so there is no opensslErrorStack property. if ((err instanceof Error) && errorMessage.test(err) && - err.openSSLErrorStack === undefined) { + err.opensslErrorStack === undefined) { return true; } }); @@ -182,10 +182,10 @@ assert.throws(function() { }, (err) => { const errorMessage = common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/; - // Throws general Error, so there is no openSSLErrorStack property. + // Throws general Error, so there is no opensslErrorStack property. if ((err instanceof Error) && errorMessage.test(err) && - err.openSSLErrorStack === undefined) { + err.opensslErrorStack === undefined) { return true; } }); @@ -193,10 +193,10 @@ assert.throws(function() { assert.throws(function() { crypto.createHash('sha1').update('0', 'hex'); }, (err) => { - // Throws TypeError, so there is no openSSLErrorStack property. + // Throws TypeError, so there is no opensslErrorStack property. if ((err instanceof Error) && /^TypeError: Bad input string$/.test(err) && - err.openSSLErrorStack === undefined) { + err.opensslErrorStack === undefined) { return true; } }); @@ -204,10 +204,10 @@ assert.throws(function() { assert.throws(function() { crypto.createSign('SHA1').update('0', 'hex'); }, (err) => { - // Throws TypeError, so there is no openSSLErrorStack property. + // Throws TypeError, so there is no opensslErrorStack property. if ((err instanceof Error) && /^TypeError: Bad input string$/.test(err) && - err.openSSLErrorStack === undefined) { + err.opensslErrorStack === undefined) { return true; } }); @@ -215,10 +215,10 @@ assert.throws(function() { assert.throws(function() { crypto.createVerify('SHA1').update('0', 'hex'); }, (err) => { - // Throws TypeError, so there is no openSSLErrorStack property. + // Throws TypeError, so there is no opensslErrorStack property. if ((err instanceof Error) && /^TypeError: Bad input string$/.test(err) && - err.openSSLErrorStack === undefined) { + err.opensslErrorStack === undefined) { return true; } }); @@ -237,7 +237,7 @@ assert.throws(function() { }, (err) => { if ((err instanceof Error) && /digest too big for rsa key$/.test(err) && - err.openSSLErrorStack === undefined) { + err.opensslErrorStack === undefined) { return true; } }); @@ -257,13 +257,13 @@ assert.throws(function() { // this would inject errors onto OpenSSL's error stack crypto.createSign('sha1').sign(sha1_privateKey); }, (err) => { - // Throws crypto error, so there is an openSSLErrorStack property. + // Throws crypto error, so there is an opensslErrorStack property. // The openSSL stack should have content. if ((err instanceof Error) && /asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag/.test(err) && - err.openSSLErrorStack !== undefined && - Array.isArray(err.openSSLErrorStack) && - err.openSSLErrorStack.length > 0) { + err.opensslErrorStack !== undefined && + Array.isArray(err.opensslErrorStack) && + err.opensslErrorStack.length > 0) { return true; } }); @@ -274,10 +274,10 @@ console.log(crypto.randomBytes(16)); assert.throws(function() { tls.createSecureContext({ crl: 'not a CRL' }); }, (err) => { - // Throws general error, so there is no openSSLErrorStack property. + // Throws general error, so there is no opensslErrorStack property. if ((err instanceof Error) && /^Error: Failed to parse CRL$/.test(err) && - err.openSSLErrorStack === undefined) { + err.opensslErrorStack === undefined) { return true; } }); From 2ad7440f4b134221d0723168cee7528c350dcfbd Mon Sep 17 00:00:00 2001 From: Greg Alexander Date: Tue, 26 Sep 2017 09:37:52 -0500 Subject: [PATCH 8/8] fixing long doc line --- doc/api/errors.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 0a4b08bebecc36..dc530c758931d4 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -193,7 +193,8 @@ 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. +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.