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: Improve error checking and reporting #3753

Closed
wants to merge 1 commit into from
Closed

crypto: Improve error checking and reporting #3753

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
42 changes: 26 additions & 16 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3394,10 +3394,14 @@ void Hmac::HmacInit(const char* hash_type, const char* key, int key_len) {
return env()->ThrowError("Unknown message digest");
}
HMAC_CTX_init(&ctx_);
int result = 0;
if (key_len == 0) {
HMAC_Init(&ctx_, "", 0, md_);
result = HMAC_Init(&ctx_, "", 0, md_);
} else {
HMAC_Init(&ctx_, key, key_len, md_);
result = HMAC_Init(&ctx_, key, key_len, md_);
}
if (!result) {
return ThrowCryptoError(env(), ERR_get_error());
}
initialised_ = true;
}
Expand Down Expand Up @@ -3518,7 +3522,8 @@ void Hash::New(const FunctionCallbackInfo<Value>& args) {

Hash* hash = new Hash(env, args.This());
if (!hash->HashInit(*hash_type)) {
return env->ThrowError("Digest method not supported");
return ThrowCryptoError(env, ERR_get_error(),
"Digest method not supported");
}
}

Expand All @@ -3530,6 +3535,9 @@ bool Hash::HashInit(const char* hash_type) {
return false;
EVP_MD_CTX_init(&mdctx_);
EVP_DigestInit_ex(&mdctx_, md_, nullptr);
if (0 != ERR_peek_error()) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

@stefanmb Is there a reason you use ERR_peek_error() here instead of looking at the return value of EVP_DigestInit_ex()?

See #4221 for context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnoordhuis Nope, chalk it up to my unfamiliarity with OpenSSL API. Checking EVP_DigestInit_ex's return code should accomplish the same thing, without breaking #4221.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stefanmb Checking error stack instead of function return status is wrong as it will generate false positive in case there is previously (intentionally) ignored error. It broke node on Alpine Linux.

+1 for fixing this properly.

(yeah, openssl api is funky...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ncopa Thank you for bringing this up. Let's continue the discussion under #4221.

initialised_ = true;
return true;
}
Expand Down Expand Up @@ -4211,7 +4219,8 @@ void DiffieHellman::Initialize(Environment* env, Local<Object> target) {

bool DiffieHellman::Init(int primeLength, int g) {
dh = DH_new();
DH_generate_parameters_ex(dh, primeLength, g, 0);
if (!DH_generate_parameters_ex(dh, primeLength, g, 0))
return false;
bool result = VerifyContext();
if (!result)
return false;
Expand Down Expand Up @@ -4304,7 +4313,7 @@ void DiffieHellman::New(const FunctionCallbackInfo<Value>& args) {
}

if (!initialized) {
return env->ThrowError("Initialization failed");
return ThrowCryptoError(env, ERR_get_error(), "Initialization failed");
}
}

Expand All @@ -4315,11 +4324,11 @@ void DiffieHellman::GenerateKeys(const FunctionCallbackInfo<Value>& args) {
DiffieHellman* diffieHellman = Unwrap<DiffieHellman>(args.Holder());

if (!diffieHellman->initialised_) {
return env->ThrowError("Not initialized");
return ThrowCryptoError(env, ERR_get_error(), "Not initialized");
}

if (!DH_generate_key(diffieHellman->dh)) {
return env->ThrowError("Key generation failed");
return ThrowCryptoError(env, ERR_get_error(), "Key generation failed");
}

int dataSize = BN_num_bytes(diffieHellman->dh->pub_key);
Expand All @@ -4338,7 +4347,7 @@ void DiffieHellman::GetPrime(const FunctionCallbackInfo<Value>& args) {
DiffieHellman* diffieHellman = Unwrap<DiffieHellman>(args.Holder());

if (!diffieHellman->initialised_) {
return env->ThrowError("Not initialized");
return ThrowCryptoError(env, ERR_get_error(), "Not initialized");
}

int dataSize = BN_num_bytes(diffieHellman->dh->p);
Expand All @@ -4356,7 +4365,7 @@ void DiffieHellman::GetGenerator(const FunctionCallbackInfo<Value>& args) {
DiffieHellman* diffieHellman = Unwrap<DiffieHellman>(args.Holder());

if (!diffieHellman->initialised_) {
return env->ThrowError("Not initialized");
return ThrowCryptoError(env, ERR_get_error(), "Not initialized");
}

int dataSize = BN_num_bytes(diffieHellman->dh->g);
Expand All @@ -4374,7 +4383,7 @@ void DiffieHellman::GetPublicKey(const FunctionCallbackInfo<Value>& args) {
DiffieHellman* diffieHellman = Unwrap<DiffieHellman>(args.Holder());

if (!diffieHellman->initialised_) {
return env->ThrowError("Not initialized");
return ThrowCryptoError(env, ERR_get_error(), "Not initialized");
}

if (diffieHellman->dh->pub_key == nullptr) {
Expand All @@ -4397,7 +4406,7 @@ void DiffieHellman::GetPrivateKey(const FunctionCallbackInfo<Value>& args) {
DiffieHellman* diffieHellman = Unwrap<DiffieHellman>(args.Holder());

if (!diffieHellman->initialised_) {
return env->ThrowError("Not initialized");
return ThrowCryptoError(env, ERR_get_error(), "Not initialized");
}

if (diffieHellman->dh->priv_key == nullptr) {
Expand All @@ -4420,7 +4429,7 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo<Value>& args) {
DiffieHellman* diffieHellman = Unwrap<DiffieHellman>(args.Holder());

if (!diffieHellman->initialised_) {
return env->ThrowError("Not initialized");
return ThrowCryptoError(env, ERR_get_error(), "Not initialized");
}

ClearErrorOnReturn clear_error_on_return;
Expand Down Expand Up @@ -4453,7 +4462,7 @@ void DiffieHellman::ComputeSecret(const FunctionCallbackInfo<Value>& args) {
delete[] data;

if (!checked) {
return env->ThrowError("Invalid key");
return ThrowCryptoError(env, ERR_get_error(), "Invalid Key");
} else if (checkResult) {
if (checkResult & DH_CHECK_PUBKEY_TOO_SMALL) {
return env->ThrowError("Supplied key is too small");
Expand Down Expand Up @@ -4490,7 +4499,7 @@ void DiffieHellman::SetPublicKey(const FunctionCallbackInfo<Value>& args) {
Environment* env = diffieHellman->env();

if (!diffieHellman->initialised_) {
return env->ThrowError("Not initialized");
return ThrowCryptoError(env, ERR_get_error(), "Not initialized");
}

if (args.Length() == 0) {
Expand All @@ -4509,7 +4518,7 @@ void DiffieHellman::SetPrivateKey(const FunctionCallbackInfo<Value>& args) {
Environment* env = diffieHellman->env();

if (!diffieHellman->initialised_) {
return env->ThrowError("Not initialized");
return ThrowCryptoError(env, ERR_get_error(), "Not initialized");
}

if (args.Length() == 0) {
Expand All @@ -4531,7 +4540,8 @@ void DiffieHellman::VerifyErrorGetter(Local<String> property,
DiffieHellman* diffieHellman = Unwrap<DiffieHellman>(args.Holder());

if (!diffieHellman->initialised_)
return diffieHellman->env()->ThrowError("Not initialized");
return ThrowCryptoError(diffieHellman->env(), ERR_get_error(),
"Not initialized");

args.GetReturnValue().Set(diffieHellman->verifyError_);
}
Expand Down