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

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 2 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
45 changes: 27 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,12 @@ 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);
CHECK(EVP_CipherInit_ex(ctx_.get(), cipher, nullptr,
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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 CHECKs out for now.

Copy link
Member

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.

nullptr, nullptr, encrypt));

int mode = EVP_CIPHER_CTX_mode(ctx_.get());
if (encrypt && (mode == EVP_CIPH_CTR_MODE || mode == EVP_CIPH_GCM_MODE ||
Expand All @@ -2616,12 +2619,12 @@ 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);
CHECK(EVP_CipherInit_ex(ctx_.get(),
nullptr,
nullptr,
reinterpret_cast<unsigned char*>(key),
reinterpret_cast<unsigned char*>(iv),
encrypt));
}


Expand Down Expand Up @@ -2656,6 +2659,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 +2690,8 @@ 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);
CHECK(EVP_CipherInit_ex(ctx_.get(), cipher, nullptr,
nullptr, nullptr, encrypt));

if (IsAuthenticatedMode()) {
CHECK(has_iv);
Expand All @@ -2699,12 +2704,12 @@ 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);
CHECK(EVP_CipherInit_ex(ctx_.get(),
nullptr,
nullptr,
reinterpret_cast<const unsigned char*>(key),
reinterpret_cast<const unsigned char*>(iv),
encrypt));
}


Expand Down Expand Up @@ -2749,6 +2754,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 +2899,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 +2959,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 +2971,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 +3052,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