Feature request: better crypto error messages

Version: master
Platform: N/A
Subsystem: crypto

I noticed today when working with public keys and verifying signatures that I would get generic error messages when there was a problem reading a public key, for example: PEM_read_bio_PUBKEY failed

Whereas if I check the OpenSSL error stack, I find more detailed/useful error strings, for example:

error:10067066:elliptic curve routines:ec_GFp_simple_oct2point:invalid encoding
error:10098010:elliptic curve routines:o2i_ECPublicKey:EC lib
error:100D708E:elliptic curve routines:ECKEY_PUB_DECODE:decode error
error:0B07707D:x509 certificate routines:X509_PUBKEY_get:public key decode error
error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib

(The first being the most useful probably)

It would be great to have at least the first error message instead of the more generic one.

/cc @nodejs/crypto

Do you have a test?

Sure, all that needs to be done is pass in any invalid key:

var crypto = require('crypto');
var verifier = crypto.createVerify('sha256');
verifier.update('foo');
var badKey = [
  '-----BEGIN PUBLIC KEY-----',
  'AFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEcCBI3Rvez5Ake8TVukoQ3GNL7FSc',
  'mwLYfY+VubJxFHytq8sLdMBd2Gjr5emoyQDJb3nt/YH+0aaPPeDr8acy9A==',
  '-----END PUBLIC KEY-----'
].join('\n');
verifier.verify(badKey, new Buffer('1234567890'));

If you then apply this:

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 986fe80..de7d8c4 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -4060,8 +4060,10 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem,
   // consumer of public keys.
   if (strncmp(key_pem, PUBLIC_KEY_PFX, PUBLIC_KEY_PFX_LEN) == 0) {
     pkey = PEM_read_bio_PUBKEY(bp, nullptr, CryptoPemCallback, nullptr);
-    if (pkey == nullptr)
+    if (pkey == nullptr) {
+      ERR_print_errors_fp(stderr);
       goto exit;
+    }
   } else if (strncmp(key_pem, PUBRSA_KEY_PFX, PUBRSA_KEY_PFX_LEN) == 0) {
     RSA* rsa =
         PEM_read_bio_RSAPublicKey(bp, nullptr, CryptoPemCallback, nullptr);

and re-run the example you will get more information like:

139899430213440:error:0D0680A8:asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag:../deps/openssl/openssl/crypto/asn1/tasn_dec.c:1201:
139899430213440:error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 error:../deps/openssl/openssl/crypto/asn1/tasn_dec.c:374:Type=X509_PUBKEY
139899430213440:error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib:../deps/openssl/openssl/crypto/pem/pem_oth.c:83:

I think what may be happening is that when it jumps to exit, additional OpenSSL functions are used which somehow clears the error stack and that is why when SignBase::CheckThrow() is called, ERR_get_error() doesn't return an error, causing one of the generic messages to be used instead?

@indutny

@indutny Thoughts?

/cc @nodejs/collaborators Does anyone have any thoughts/opinions on how many of the OpenSSL errors should be returned or just use the most recent one on the OpenSSL error stack?

I wouldn't object to exposing the openssl error stack as a property on the exception object.

You call ERR_set_mark() before starting the operation that can fail. On failure, you scan the elements from ERR_get_state() until you find the one with the ERR_FLAG_MARK flag set.

@mscdex

@mscdex this sounds good to me

I still think the actual exception message should be changed even if the OpenSSL error stack is added as a separate property, since not everyone will (or will always know to) access that property to find the "real" error.

Would anyone be opposed to changing the exception message to the most recent OpenSSL error and adding the rest of the stack as a separate property? Also any suggestions on that property name?

@mscdex

@mscdex should we add them as an array then? It could be opensslError property, but I'm not the best person to ask about naming!

This is still an unfulfilled feature request?

Yes. I've added 'good first contribution' and 'c++' tags. It's probably a fair amount of work but not very complex so a good starting point for someone who wants to dive into the C++ side of things. If you want to work on this and have questions, don't hesitate to ping me.

Would anyone be opposed to changing the exception message to the most recent OpenSSL error and adding the rest of the stack as a separate property?

Seems fine to me. Changing the error message automatically makes it semver-major though.

The latter, the binding function.

Can you point me in the direction of where these exception objects are?

ThrowCryptoError() is the function you want, most likely. You'll probably have to move some code around to be able to adorn the exception object.

Total noob question here, but im not grasping something fundamental here. I understand that i'll prob have to move some code around but i'm failing to see how to add a new property to the exception object. Like, how would one add the property 'foo' with value 'bar' to the exception object.

If i step through the code, I can see the VerifyFinal return the error enum, then CheckThrow is called. Which will then call ThrowCryptoError, which will call ThrowError with the errmsg string from openssl, which will then create a v8 Exception with the errmsg passed in and throws the exception in v8. I doubt we want to be editing the v8 exception/code to take in additional parameters. Maybe i'm overlooking something but the only thing i can see is a err string being passed over, which eventually gets turned into v8 Exception. So how would one add a new property to the exception to get passed back over to JS?

Any help or resources would be appreciated.

Like this, basically:

Local<String> message = /* ... */;
Local<Value> exception_v = Exception::TypeError(message);
CHECK(!exception_v.IsEmpty());
Local<Object> exception = exception_v.As<Object>();
Local<String> key = /* ... */;
Local<Value> value = /* ... */;
exception->Set(env->context(), key, value).FromJust();
env->isolate()->ThrowException(exception);

crypto: better crypto error messages

I see that commits referencing this issue have been added. Shouldn't this issue be closed?

Yes, this is complete. I'll close.

Navigation Menu

Repository navigation

Feature request: better crypto error messages #5444

4 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Feature request: better crypto error messages #5444

Description

Activity

mscdex commented on Feb 26, 2016

indutny commented on Mar 1, 2016

mscdex commented on Mar 1, 2016

mscdex commented on Apr 15, 2016

mscdex commented on Jun 16, 2016

bnoordhuis commented on Jun 16, 2016

indutny commented on Jun 16, 2016

mscdex commented on Jun 18, 2016

indutny commented on Jun 18, 2016

Trott commented on Jul 7, 2017

bnoordhuis commented on Jul 8, 2017

4 remaining items

bnoordhuis commented on Aug 4, 2017

gla5001 commented on Aug 8, 2017

bnoordhuis commented on Aug 8, 2017

ryzokuken commented on Mar 4, 2018

bnoordhuis commented on Mar 4, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions