Skip to content

Decorate crypto openssl errors with .code, .reason, ... #26868

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

Closed
sam-github wants to merge 2 commits into from
Closed

Decorate crypto openssl errors with .code, .reason, ... #26868

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5bef6e
crypto: add openssl specific error properties
sam-github Mar 27, 2019
3e3e8d8
tls: return an OpenSSL error from renegotiate
sam-github Mar 27, 2019
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
Next Next commit
crypto: add openssl specific error properties 
Don't force the user to parse the long-style OpenSSL error message,
decorate the error with the library, reason, code, function.
  • Loading branch information
@sam-github
sam-github committed Mar 27, 2019
commit c5bef6e77c8a960245b3667af39825ee8f929a1c
28 changes: 24 additions & 4 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,6 @@ circumstance of why the error occurred. `Error` objects capture a "stack trace"
detailing the point in the code at which the `Error` was instantiated, and may
provide a text description of the error.

For crypto only, `Error` objects will include the OpenSSL error stack in a
separate property called `opensslErrorStack` if it is available when the error
is thrown.

All errors generated by Node.js, including all System and JavaScript errors,
will either be instances of, or inherit from, the `Error` class.

Expand Down Expand Up @@ -589,6 +585,30 @@ program. For a comprehensive list, see the [`errno`(3) man page][].
encountered by [`http`][] or [`net`][] — often a sign that a `socket.end()`
was not properly called.

## OpenSSL Errors

Errors originating in `crypto` or `tls` are of class `Error`, and in addition to
the standard `.code` and `.message` properties, may have some additional
OpenSSL-specific properties.

### error.opensslErrorStack
Comment thread
BridgeAR marked this conversation as resolved.
Outdated

An array of errors that can give context to where in the OpenSSL library an
error originates from.

### error.function

The OpenSSL function the error originates in.

### error.library

The OpenSSL library the error originates in.

### error.reason

A human-readable string describing the reason for the error.


<a id="nodejs-error-codes"></a>
## Node.js Error Codes

Expand Down
115 changes: 114 additions & 1 deletion src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,113 @@ static int NoPasswordCallback(char* buf, int size, int rwflag, void* u) {
}


// namespace node::crypto::error
namespace error {
Comment thread
sam-github marked this conversation as resolved.
Outdated
void Decorate(Environment* env, Local<Object> obj,
unsigned long err) { // NOLINT(runtime/int)
if (err == 0) return; // No decoration possible.

const char* ls = ERR_lib_error_string(err);
const char* fs = ERR_func_error_string(err);
const char* rs = ERR_reason_error_string(err);

Isolate* isolate = env->isolate();
Local<Context> context = isolate->GetCurrentContext();

if (ls != nullptr) {
if (obj->Set(context, env->library_string(),
OneByteString(isolate, ls)).IsNothing()) {
return;
}
}
if (fs != nullptr) {
if (obj->Set(context, env->function_string(),
OneByteString(isolate, fs)).IsNothing()) {
return;
}
}
if (rs != nullptr) {
if (obj->Set(context, env->reason_string(),
OneByteString(isolate, rs)).IsNothing()) {
return;
}

// SSL has no API to recover the error name from the number, so we
// transform reason strings like "this error" to "ERR_SSL_THIS_ERROR",
// which ends up being close to the original error macro name.
std::string reason(rs);

for (auto& c : reason) {
if (c == ' ')
c = '_';
else
c = ToUpper(c);
}

#define OSSL_ERROR_CODES_MAP(V) \
V(SYS) \
V(BN) \
V(RSA) \
V(DH) \
V(EVP) \
V(BUF) \
V(OBJ) \
V(PEM) \
V(DSA) \
V(X509) \
V(ASN1) \
V(CONF) \
V(CRYPTO) \
V(EC) \
V(SSL) \
V(BIO) \
V(PKCS7) \
V(X509V3) \
V(PKCS12) \
V(RAND) \
V(DSO) \
V(ENGINE) \
V(OCSP) \
V(UI) \
V(COMP) \
V(ECDSA) \
V(ECDH) \
V(OSSL_STORE) \
V(FIPS) \
V(CMS) \
V(TS) \
V(HMAC) \
V(CT) \
V(ASYNC) \
V(KDF) \
V(SM2) \
V(USER) \

#define V(name) case ERR_LIB_##name: lib = #name "_"; break;
const char* lib = "";
const char* prefix = "OSSL_";
switch (ERR_GET_LIB(err)) { OSSL_ERROR_CODES_MAP(V) }
#undef V
#undef OSSL_ERROR_CODES_MAP
// Don't generate codes like "ERR_OSSL_SSL_".
if (lib && strcmp(lib, "SSL_") == 0)
prefix = "";

// All OpenSSL reason strings fit in a single 80-column macro definition,
// all prefix lengths are <= 10, and ERR_OSSL_ is 9, so 128 is more than
// sufficient.
char code[128];
snprintf(code, sizeof(code), "ERR_%s%s%s", prefix, lib, reason.c_str());

if (obj->Set(env->isolate()->GetCurrentContext(),
env->code_string(),
OneByteString(env->isolate(), code)).IsNothing())
return;
}
}
} // namespace error


struct CryptoErrorVector : public std::vector<std::string> {
inline void Capture() {
clear();
Expand Down Expand Up @@ -258,6 +365,8 @@ struct CryptoErrorVector : public std::vector<std::string> {

void ThrowCryptoError(Environment* env,
unsigned long err, // NOLINT(runtime/int)
// Default, only used if there is no SSL `err` which can
// be used to create a long-style message string.
const char* message = nullptr) {
char message_buffer[128] = {0};
if (err != 0 || message == nullptr) {
Expand All @@ -270,7 +379,11 @@ void ThrowCryptoError(Environment* env,
.ToLocalChecked();
CryptoErrorVector errors;
errors.Capture();
auto exception = errors.ToException(env, exception_string);
Local<Value> exception = errors.ToException(env, exception_string);
Local<Object> obj;
if (!exception->ToObject(env->context()).ToLocal(&obj))
return;
error::Decorate(env, obj, err);
Copy link
Copy Markdown
Member

@addaleax addaleax Mar 27, 2019

Choose a reason for hiding this comment

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

Suggested change
error::Decorate(env, obj, err);
if (error::Decorate(env, obj, err).IsEmpty())
return;

env->isolate()->ThrowException(exception);
}

Expand Down
2 changes: 1 addition & 1 deletion src/tls_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ Local<Value> TLSWrap::GetSSLError(int status, int* err, std::string* msg) {
if (c == ' ')
c = '_';
else
c = ::toupper(c);
c = ToUpper(c);
}
obj->Set(context, env()->code_string(),
OneByteString(isolate, ("ERR_SSL_" + code).c_str()))
Expand Down
11 changes: 11 additions & 0 deletions src/util-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,17 @@ std::string ToLower(const std::string& in) {
return out;
}

char ToUpper(char c) {
return c >= 'a' && c <= 'z' ? (c - 'a') + 'A' : c;
}

std::string ToUpper(const std::string& in) {
std::string out(in.size(), 0);
for (size_t i = 0; i < in.size(); ++i)
out[i] = ToUpper(in[i]);
return out;
}

bool StringEqualNoCase(const char* a, const char* b) {
do {
if (*a == '\0')
Expand Down
4 changes: 4 additions & 0 deletions src/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,10 @@ inline void SwapBytes64(char* data, size_t nbytes);
inline char ToLower(char c);
inline std::string ToLower(const std::string& in);

// toupper() is locale-sensitive. Use ToUpper() instead.
inline char ToUpper(char c);
inline std::string ToUpper(const std::string& in);

// strcasecmp() is locale-sensitive. Use StringEqualNoCase() instead.
inline bool StringEqualNoCase(const char* a, const char* b);

Expand Down
13 changes: 9 additions & 4 deletions test/parallel/test-crypto-dh.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,13 @@ const secret4 = dh4.computeSecret(key2, 'hex', 'base64');

assert.strictEqual(secret1, secret4);

const wrongBlockLength =
/^Error: error:0606506D:digital envelope routines:EVP_DecryptFinal_ex:wrong final block length$/;
const wrongBlockLength = {
message: 'error:0606506D:digital envelope' +
' routines:EVP_DecryptFinal_ex:wrong final block length',
code: 'ERR_OSSL_EVP_WRONG_FINAL_BLOCK_LENGTH',
library: 'digital envelope routines',
reason: 'wrong final block length'
};

// Run this one twice to make sure that the dh3 clears its error properly
{
Expand All @@ -111,7 +116,7 @@ const wrongBlockLength =

assert.throws(() => {
dh3.computeSecret('');
}, /^Error: Supplied key is too small$/);
}, { message: 'Supplied key is too small' });

// Create a shared using a DH group.
const alice = crypto.createDiffieHellmanGroup('modp5');
Expand Down Expand Up @@ -260,7 +265,7 @@ if (availableCurves.has('prime256v1') && availableCurves.has('secp256k1')) {

assert.throws(() => {
ecdh4.setPublicKey(ecdh3.getPublicKey());
}, /^Error: Failed to convert Buffer to EC_POINT$/);
}, { message: 'Failed to convert Buffer to EC_POINT' });

// Verify that we can use ECDH without having to use newly generated keys.
const ecdh5 = crypto.createECDH('secp256k1');
Expand Down
8 changes: 7 additions & 1 deletion test/parallel/test-crypto-key-objects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,13 @@ const privatePem = fixtures.readSync('test_rsa_privkey.pem', 'ascii');
// This should not cause a crash: https://github.com/nodejs/node/issues/25247
assert.throws(() => {
createPrivateKey({ key: '' });
}, /null/);
}, {
message: 'error:2007E073:BIO routines:BIO_new_mem_buf:null parameter',
code: 'ERR_OSSL_BIO_NULL_PARAMETER',
reason: 'null parameter',
library: 'BIO routines',
function: 'BIO_new_mem_buf',
});
}

[
Expand Down
14 changes: 12 additions & 2 deletions test/parallel/test-crypto-padding.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,12 @@ assert.strictEqual(enc(EVEN_LENGTH_PLAIN, true), EVEN_LENGTH_ENCRYPTED);
assert.throws(function() {
// Input must have block length %.
enc(ODD_LENGTH_PLAIN, false);
}, /data not multiple of block length/);
}, {
message: 'error:0607F08A:digital envelope routines:EVP_EncryptFinal_ex:' +
'data not multiple of block length',
code: 'ERR_OSSL_EVP_DATA_NOT_MULTIPLE_OF_BLOCK_LENGTH',
reason: 'data not multiple of block length',
});

assert.strictEqual(
enc(EVEN_LENGTH_PLAIN, false), EVEN_LENGTH_ENCRYPTED_NOPAD
Expand All @@ -99,7 +104,12 @@ assert.strictEqual(dec(EVEN_LENGTH_ENCRYPTED, false).length, 48);
assert.throws(function() {
// Must have at least 1 byte of padding (PKCS):
assert.strictEqual(dec(EVEN_LENGTH_ENCRYPTED_NOPAD, true), EVEN_LENGTH_PLAIN);
}, /bad decrypt/);
}, {
message: 'error:06065064:digital envelope routines:EVP_DecryptFinal_ex:' +
'bad decrypt',
reason: 'bad decrypt',
code: 'ERR_OSSL_EVP_BAD_DECRYPT',
});

// No-pad encrypted string should return the same:
assert.strictEqual(
Expand Down
10 changes: 8 additions & 2 deletions test/parallel/test-crypto-rsa-dsa.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,14 @@ const dsaKeyPem = fixtures.readSync('test_dsa_privkey.pem', 'ascii');
const dsaKeyPemEncrypted = fixtures.readSync('test_dsa_privkey_encrypted.pem',
'ascii');

const decryptError =
/^Error: error:06065064:digital envelope routines:EVP_DecryptFinal_ex:bad decrypt$/;
const decryptError = {
message: 'error:06065064:digital envelope routines:EVP_DecryptFinal_ex:' +
'bad decrypt',
code: 'ERR_OSSL_EVP_BAD_DECRYPT',
reason: 'bad decrypt',
function: 'EVP_DecryptFinal_ex',
library: 'digital envelope routines',
};

// Test RSA encryption/decryption
{
Expand Down
7 changes: 5 additions & 2 deletions test/parallel/test-crypto-stream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,11 @@ const cipher = crypto.createCipheriv('aes-128-cbc', key, iv);
const decipher = crypto.createDecipheriv('aes-128-cbc', badkey, iv);

cipher.pipe(decipher)
.on('error', common.mustCall(function end(err) {
assert(/bad decrypt/.test(err));
.on('error', common.expectsError({
message: /bad decrypt/,
function: 'EVP_DecryptFinal_ex',
library: 'digital envelope routines',
reason: 'bad decrypt',
}));

cipher.end('Papaya!'); // Should not cause an unhandled exception.
18 changes: 11 additions & 7 deletions test/parallel/test-crypto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,15 +238,19 @@ assert.throws(function() {
// This would inject errors onto OpenSSL's error stack
crypto.createSign('sha1').sign(sha1_privateKey);
}, (err) => {
// Do the standard checks, but then do some custom checks afterwards.
assert.throws(() => { throw err; }, {
message: 'error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag',
library: 'asn1 encoding routines',
function: 'asn1_check_tlen',
reason: 'wrong tag',
code: 'ERR_OSSL_ASN1_WRONG_TAG',
});
// Throws crypto error, so there is an opensslErrorStack property.
// The openSSL stack should have content.
if ((err instanceof Error) &&
/asn1 encoding routines:[^:]*:wrong tag/.test(err) &&
err.opensslErrorStack !== undefined &&
Array.isArray(err.opensslErrorStack) &&
err.opensslErrorStack.length > 0) {
return true;
}
assert(Array.isArray(err.opensslErrorStack));
assert(err.opensslErrorStack.length > 0);
return true;
});

// Make sure memory isn't released before being returned
Expand Down