-
Notifications
You must be signed in to change notification settings - Fork 29.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
crypto: handle OpenSSL error queue in CipherBase #21288
crypto: handle OpenSSL error queue in CipherBase #21288
Conversation
This handles all errors produced by OpenSSL within the CipherBase class. API functions clear the error queue on return, utility functions such as InitAuthenticated() ensure that they do not add any new errors to the queue. Previously ignored return values are now being CHECK'd. Fixes: nodejs#21281 Refs: nodejs#21287
@tniessen sadly an error occured when I tried to trigger a build :( |
cc @nodejs/crypto |
@@ -2749,6 +2753,7 @@ static bool IsValidGCMTagLength(unsigned int tag_len) { | |||
bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len, | |||
unsigned int auth_tag_len) { | |||
CHECK(IsAuthenticatedMode()); | |||
MarkPopErrorOnReturn mark_pop_error_on_return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own understanding: MarkPopErrorOnReturn
basically means that OpenSSL errors inside this scope are swallowed/ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, all errors that occur in this scope are dropped once the destructor is called (unless they have been handled specifically).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now that we're handling the errors manually, Node will be throwing an error instead of just crashing, I suppose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And because we're manually handling the errors now, it's better because we'll throw an error that you could catch and what not instead of just crashing, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't throw more errors than before (except for ::Init
), it will just make sure that if it throws, it also clears the internal error queue of OpenSSL.
Travis CI failure looks related:
|
@addaleax Yes, see above:
As far as I can tell, #15037 should have set the flag before calling |
src/node_crypto.cc
Outdated
|
||
ctx_.reset(EVP_CIPHER_CTX_new()); | ||
const bool encrypt = (kind_ == kCipher); | ||
EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, nullptr, nullptr, encrypt); | ||
CHECK(EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EVP_CipherInit_ex()
can fail for legitimate reasons (e.g. an engine that fails to load) so a CHECK
is not appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the old behavior (= no error checking) isn't appropriate either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, errors should be reported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make this semver-major I guess, so I'll probably just leave the CHECK
s out for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mwah, I don't think better error checking constitutes semver-major. The kind of bugs it'd flush out are programmer bugs and environmental issues.
src/node_crypto.cc
Outdated
@@ -2893,6 +2898,7 @@ void CipherBase::SetAuthTag(const FunctionCallbackInfo<Value>& args) { | |||
bool CipherBase::SetAAD(const char* data, unsigned int len, int plaintext_len) { | |||
if (!ctx_ || !IsAuthenticatedMode()) | |||
return false; | |||
ClearErrorOnReturn clear_error_on_return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should ideally be MarkPopErrorOnReturn
. ClearErrorOnReturn
is a blunt hammer and might swallow genuine errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point taken, I can change it. In practice, the error stack should be empty when the JS layer calls into the C++ layer though, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, except for the following:
- It can mess up C++ -> C++ calls when the caller also manipulates the error stack.
- I know of at least one add-on that leaves openssl errors on the stack for some time.
Instead of clearing all errors, mark and pop only errors that were added within the current scope.
@bnoordhuis Please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated, but node_crypto.cc
seems to have become rather long, don't you think? Do you believe we should try break it into smaller parts (and even namespaces) at this point?
|
||
ctx_.reset(EVP_CIPHER_CTX_new()); | ||
const bool encrypt = (kind_ == kCipher); | ||
EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, nullptr, nullptr, encrypt); | ||
if (1 != EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit, but shouldn't this be done the other way around? Eg: EVP_CipherInit_ex(...) != 1
? Would love to hear why you chose this over the former.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know we had a preference. I usually do the opposite, but I think having != 1
at the end of a multi-line expression looks weird.
Fun fact, this style was commonly used to prevent accidental assignment of variables / pointers in boolean expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because people would accidentally type =
instead of ==
all the times? Sounds more likely in the era of butterfly keyboards, heh. If it makes sense for you to keep this, feel free to do so. I agree wholeheartedly that != 1
looks weird when in a multiline statement, just wanted to make sure we're not going against the style guide here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, even though I personally still prefer var == constant
, the only reason here is that it looks weird. If no one has a strong opinion against this, I'd prefer to keep it this way.
reinterpret_cast<unsigned char*>(key), | ||
reinterpret_cast<unsigned char*>(iv), | ||
encrypt); | ||
if (1 != EVP_CipherInit_ex(ctx_.get(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@@ -2686,7 +2696,11 @@ void CipherBase::InitIv(const char* cipher_type, | |||
EVP_CIPHER_CTX_set_flags(ctx_.get(), EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); | |||
|
|||
const bool encrypt = (kind_ == kCipher); | |||
EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, nullptr, nullptr, encrypt); | |||
if (1 != EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
reinterpret_cast<const unsigned char*>(key), | ||
reinterpret_cast<const unsigned char*>(iv), | ||
encrypt); | ||
if (1 != EVP_CipherInit_ex(ctx_.get(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting boring, isn't it? 😛
@@ -2749,6 +2753,7 @@ static bool IsValidGCMTagLength(unsigned int tag_len) { | |||
bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len, | |||
unsigned int auth_tag_len) { | |||
CHECK(IsAuthenticatedMode()); | |||
MarkPopErrorOnReturn mark_pop_error_on_return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now that we're handling the errors manually, Node will be throwing an error instead of just crashing, I suppose?
reinterpret_cast<unsigned char*>(key), | ||
reinterpret_cast<unsigned char*>(iv), | ||
encrypt); | ||
if (1 != EVP_CipherInit_ex(ctx_.get(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@@ -2686,7 +2696,11 @@ void CipherBase::InitIv(const char* cipher_type, | |||
EVP_CIPHER_CTX_set_flags(ctx_.get(), EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); | |||
|
|||
const bool encrypt = (kind_ == kCipher); | |||
EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, nullptr, nullptr, encrypt); | |||
if (1 != EVP_CipherInit_ex(ctx_.get(), cipher, nullptr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
reinterpret_cast<const unsigned char*>(key), | ||
reinterpret_cast<const unsigned char*>(iv), | ||
encrypt); | ||
if (1 != EVP_CipherInit_ex(ctx_.get(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
God, isn't this repetitive 😅
@@ -2749,6 +2753,7 @@ static bool IsValidGCMTagLength(unsigned int tag_len) { | |||
bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len, | |||
unsigned int auth_tag_len) { | |||
CHECK(IsAuthenticatedMode()); | |||
MarkPopErrorOnReturn mark_pop_error_on_return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And because we're manually handling the errors now, it's better because we'll throw an error that you could catch and what not instead of just crashing, right?
EVP_CTRL_GCM_SET_TAG, | ||
auth_tag_len_, | ||
reinterpret_cast<unsigned char*>(auth_tag_)); | ||
CHECK(EVP_CIPHER_CTX_ctrl(ctx_.get(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit intrigued.
https://www.openssl.org/docs/man1.0.2/crypto/EVP_EncryptInit.html doesn't show EVP_CIPHER_CTX_ctrl
returning anything. If it's actually returning void
(I hope not), the would this work?
If it doesn't return void
, shouldn't we use an if
statement here as well and throw something nice instead of crashing the process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reasons I can think of right now are a bug (e.g. because we permitted the call in an invalid state) or a memory allocation failure, and those are generally not handled within our APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CI: https://ci.nodejs.org/job/node-test-pull-request/15753/ Pinging @nodejs/crypto one last time. |
Landed in 85b0f16. |
This handles all errors produced by OpenSSL within the CipherBase class. API functions ensure that they do not add any new errors to the error queue. Also adds a couple of CHECKs and throws under certain conditions. PR-URL: #21288 Fixes: #21281 Refs: #21287 Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
This handles all errors produced by OpenSSL within the CipherBase class. API functions ensure that they do not add any new errors to the error queue. Also adds a couple of CHECKs and throws under certain conditions. PR-URL: #21288 Fixes: #21281 Refs: #21287 Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Squashed commit of the following: commit a61cace46f4f745283177a6ca4551e6237aa4d8a Author: Hugo Ferrando Seage <hugoseage@gmail.com> Date: Wed May 13 15:33:19 2020 +0200 Downgrade eslint to version 6.8.0 Version 7 dropped support for node 8 commit f08aa1e125738fe00183306f925d4e2ecf9e5704 Author: Hugo Ferrando Seage <hugoseage@gmail.com> Date: Wed May 13 14:52:18 2020 +0200 FIX unit tests - Made environment.js work with the newer mocha version - Remove all Buffer constructors - Change test order to fix broken tests in node < 10.7 - nodejs/node#21288 - Changed minimum node version to 8, due to the updated dependencies commit ce1f40d4e5ae79f4bcf6a3cca0f914ef51effd3b Author: Hugo Ferrando Seage <hugoseage@gmail.com> Date: Wed May 13 14:52:04 2020 +0200 Relint jwt-utils.js with newer eslint rules commit 59940f5552dbfb8353e71250b1a0d3038913a17e Author: Hugo Ferrando Seage <hugoseage@gmail.com> Date: Wed May 13 14:50:51 2020 +0200 Replace new Buffer with Buffer.from and Buffer.alloc The Buffer constructor was deprecated: https://nodejs.org/en/docs/guides/buffer-constructor-deprecation/ commit b46245788a6235b0490f9902e9218945cb6d585c Author: Hugo Ferrando Seage <hugoseage@gmail.com> Date: Wed May 13 14:48:56 2020 +0200 Remove travis and coveralls badges Also removed the .travis file and npm script. Neither service is executing their pipelines for this repo anymore commit 23f537ad6fdc4b8924add6feb16628512529a47a Author: Hugo Ferrando Seage <hugoseage@gmail.com> Date: Wed May 13 14:48:42 2020 +0200 Integrate jscsrc rules into eslintrc commit c0e23b53a27d82d14430d05b63426dcae868cfd9 Author: Hugo Ferrando Seage <hugoseage@gmail.com> Date: Wed May 13 14:44:52 2020 +0200 CHORE update dependencies - coveralls removed as the pipeline is not executed anymore - jscs removed as it was deprecated - added eslint-google dependency to replace jscs with eslint - jwa NOT updated as it would break compatibility - therror NOT updated
This handles all errors produced by OpenSSL within the
CipherBase
class. API functions clear the error queue on return, utility functions such asInitAuthenticated()
ensure that they do not add any new errors to the queue. Previously ignored return values are now beingCHECK
'd.::Final
does not clear the error queue as there is a custom error handler in place.Note that #21287 is required for this to work, otherwise the
CHECK
in line 2597 will fail.Fixes: #21281
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes