Skip to content
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.

Sign up for GitHub

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

Jump to bottom

crypto: handle OpenSSL error queue in CipherBase #21288

Closed
wants to merge 3 commits into from
Closed

crypto: handle OpenSSL error queue in CipherBase #21288

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
643bc49
crypto: handle OpenSSL error queue in CipherBase
tniessen Jun 12, 2018
0a89f42
Don't clear all errors in return
tniessen Jun 14, 2018
2fda8de
Add error checking for EVP_CipherInit_ex
tniessen Jul 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 39 additions & 18 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2566,6 +2566,7 @@ void CipherBase::Init(const char* cipher_type,
int key_buf_len,
unsigned int auth_tag_len) {
HandleScope scope(env()->isolate());
MarkPopErrorOnReturn mark_pop_error_on_return;

#ifdef NODE_FIPS_MODE
if (FIPS_mode()) {
Expand All @@ -2590,10 +2591,15 @@ void CipherBase::Init(const char* cipher_type,
1,
key,
iv);
CHECK_NE(key_len, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://www.openssl.org/docs/man1.0.2/crypto/EVP_BytesToKey.html, this method returns 0 when an error is encountered. Don't you think we should instead use an if statement here and throw something if key_len is 0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhh yeah, in theory, yes. Not sure which error conditions could cause that, so maybe I will defer that to another PR. This change won't make the situation any worse :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Totally fine by me, just wanted to make sure.


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,
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small style nit, but I was curious why you would do this over the regular EVP_CipherInit_ex(...) != 1? I find it a bit more readable, but would love to know why you picked this over it.

nullptr, nullptr, encrypt)) {
return ThrowCryptoError(env(), ERR_get_error(),
"Failed to initialize cipher");
}

int mode = EVP_CIPHER_CTX_mode(ctx_.get());
if (encrypt && (mode == EVP_CIPH_CTR_MODE || mode == EVP_CIPH_GCM_MODE ||
Expand All @@ -2616,12 +2622,15 @@ void CipherBase::Init(const char* cipher_type,

CHECK_EQ(1, EVP_CIPHER_CTX_set_key_length(ctx_.get(), key_len));

EVP_CipherInit_ex(ctx_.get(),
nullptr,
nullptr,
reinterpret_cast<unsigned char*>(key),
reinterpret_cast<unsigned char*>(iv),
encrypt);
if (1 != EVP_CipherInit_ex(ctx_.get(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

nullptr,
nullptr,
reinterpret_cast<unsigned char*>(key),
reinterpret_cast<unsigned char*>(iv),
encrypt)) {
return ThrowCryptoError(env(), ERR_get_error(),
"Failed to initialize cipher");
}
}


Expand Down Expand Up @@ -2656,6 +2665,7 @@ void CipherBase::InitIv(const char* cipher_type,
int iv_len,
unsigned int auth_tag_len) {
HandleScope scope(env()->isolate());
MarkPopErrorOnReturn mark_pop_error_on_return;

const EVP_CIPHER* const cipher = EVP_get_cipherbyname(cipher_type);
if (cipher == nullptr) {
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

nullptr, nullptr, encrypt)) {
return ThrowCryptoError(env(), ERR_get_error(),
"Failed to initialize cipher");
}

if (IsAuthenticatedMode()) {
CHECK(has_iv);
Expand All @@ -2699,12 +2713,15 @@ void CipherBase::InitIv(const char* cipher_type,
return env()->ThrowError("Invalid key length");
}

EVP_CipherInit_ex(ctx_.get(),
nullptr,
nullptr,
reinterpret_cast<const unsigned char*>(key),
reinterpret_cast<const unsigned char*>(iv),
encrypt);
if (1 != EVP_CipherInit_ex(ctx_.get(),
Copy link
Contributor

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? 😛

Copy link
Contributor

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 😅

nullptr,
nullptr,
reinterpret_cast<const unsigned char*>(key),
reinterpret_cast<const unsigned char*>(iv),
encrypt)) {
return ThrowCryptoError(env(), ERR_get_error(),
"Failed to initialize cipher");
}
}


Expand Down Expand Up @@ -2749,6 +2766,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;
Copy link
Member

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?

Copy link
Member Author

@tniessen tniessen Jun 14, 2018 •
edited
Loading

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).

Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Member Author

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.


if (!EVP_CIPHER_CTX_ctrl(ctx_.get(),
EVP_CTRL_AEAD_SET_IVLEN,
Expand Down Expand Up @@ -2893,6 +2911,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;
MarkPopErrorOnReturn mark_pop_error_on_return;

int outlen;
const int mode = EVP_CIPHER_CTX_mode(ctx_.get());
Expand Down Expand Up @@ -2952,6 +2971,7 @@ CipherBase::UpdateResult CipherBase::Update(const char* data,
int* out_len) {
if (!ctx_)
return kErrorState;
MarkPopErrorOnReturn mark_pop_error_on_return;

const int mode = EVP_CIPHER_CTX_mode(ctx_.get());

Expand All @@ -2963,10 +2983,10 @@ CipherBase::UpdateResult CipherBase::Update(const char* data,
// on first update:
if (kind_ == kDecipher && IsAuthenticatedMode() && auth_tag_len_ > 0 &&
auth_tag_len_ != kNoAuthTagLength && !auth_tag_set_) {
EVP_CIPHER_CTX_ctrl(ctx_.get(),
EVP_CTRL_GCM_SET_TAG,
auth_tag_len_,
reinterpret_cast<unsigned char*>(auth_tag_));
CHECK(EVP_CIPHER_CTX_ctrl(ctx_.get(),
Copy link
Contributor

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?

Copy link
Member Author

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.

EVP_CTRL_GCM_SET_TAG,
auth_tag_len_,
reinterpret_cast<unsigned char*>(auth_tag_)));
auth_tag_set_ = true;
}

Expand Down Expand Up @@ -3044,6 +3064,7 @@ void CipherBase::Update(const FunctionCallbackInfo<Value>& args) {
bool CipherBase::SetAutoPadding(bool auto_padding) {
if (!ctx_)
return false;
MarkPopErrorOnReturn mark_pop_error_on_return;
return EVP_CIPHER_CTX_set_padding(ctx_.get(), auto_padding);
}

Expand Down