FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: better crypto error messages · nodejs/node@1ed0c77 · GitHub

/ node Public

Commit 1ed0c77

Browse files
gla5001
authored and
MylesBorins
committed
crypto: better crypto error messages
Add openSSL error stack to the exception object thrown from crypto. The new exception property is only added to the object if the error stack has not cleared out prior to calling ThrowCryptoError. PR-URL: #15518 Refs: #5444 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
1 parent ec56cbe commit 1ed0c77

4 files changed

Lines changed: 163 additions & 16 deletions

File tree

‎doc/api/errors.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,10 @@ circumstance of why the error occurred. `Error` objects capture a "stack trace"
192192
detailing the point in the code at which the `Error` was instantiated, and may
193193
provide a text description of the error.
194194

195+
For crypto only, `Error` objects will include the OpenSSL error stack in a
196+
separate property called `opensslErrorStack` if it is available when the error
197+
is thrown.
198+
195199
All errors generated by Node.js, including all System and JavaScript errors,
196200
will either be instances of, or inherit from, the `Error` class.
197201

‎src/env.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,7 @@ class ModuleWrap;
225225
V(onstreamclose_string, "onstreamclose") \
226226
V(ontrailers_string, "ontrailers") \
227227
V(onwrite_string, "onwrite") \
228+
V(openssl_error_stack, "opensslErrorStack") \
228229
V(output_string, "output") \
229230
V(order_string, "order") \
230231
V(owner_string, "owner") \

‎src/node_crypto.cc‎

Lines changed: 56 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -255,13 +255,65 @@ void ThrowCryptoError(Environment* env,
255255
unsigned long err, // NOLINT(runtime/int)
256256
const char* default_message = nullptr) {
257257
HandleScope scope(env->isolate());
258+
Local<String> message;
259+
258260
if (err != 0 || default_message == nullptr) {
259261
char errmsg[128] = { 0 };
260262
ERR_error_string_n(err, errmsg, sizeof(errmsg));
261-
env->ThrowError(errmsg);
263+
message = String::NewFromUtf8(env->isolate(), errmsg,
264+
v8::NewStringType::kNormal)
265+
.ToLocalChecked();
262266
} else {
263-
env->ThrowError(default_message);
267+
message = String::NewFromUtf8(env->isolate(), default_message,
268+
v8::NewStringType::kNormal)
269+
.ToLocalChecked();
270+
}
271+
272+
Local<Value> exception_v = Exception::Error(message);
273+
CHECK(!exception_v.IsEmpty());
274+
Local<Object> exception = exception_v.As<Object>();
275+
ERR_STATE* es = ERR_get_state();
276+
277+
if (es->bottom != es->top) {
278+
Local<Array> error_stack = Array::New(env->isolate());
279+
int top = es->top;
280+
281+
// Build the error_stack array to be added to opensslErrorStack property.
282+
for (unsigned int i = 0; es->bottom != es->top;) {
283+
unsigned long err_buf = es->err_buffer[es->top]; // NOLINT(runtime/int)
284+
// Only add error string if there is valid err_buffer.
285+
if (err_buf) {
286+
char tmp_str[256];
287+
ERR_error_string_n(err_buf, tmp_str, sizeof(tmp_str));
288+
error_stack->Set(env->context(), i,
289+
String::NewFromUtf8(env->isolate(), tmp_str,
290+
v8::NewStringType::kNormal)
291+
.ToLocalChecked()).FromJust();
292+
// Only increment if we added to error_stack.
293+
i++;
294+
}
295+
296+
// Since the ERR_STATE is a ring buffer, we need to use modular
297+
// arithmetic to loop back around in the case where bottom is after top.
298+
// Using ERR_NUM_ERRORS macro defined in openssl.
299+
es->top = (((es->top - 1) % ERR_NUM_ERRORS) + ERR_NUM_ERRORS) %
300+
ERR_NUM_ERRORS;
301+
}
302+
303+
// Restore top.
304+
es->top = top;
305+
306+
// Add the opensslErrorStack property to the exception object.
307+
// The new property will look like the following:
308+
// opensslErrorStack: [
309+
// 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib',
310+
// 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 err'
311+
// ]
312+
exception->Set(env->context(), env->openssl_error_stack(), error_stack)
313+
.FromJust();
264314
}
315+
316+
env->isolate()->ThrowException(exception);
265317
}
266318

267319

@@ -4316,8 +4368,6 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem,
43164368
if (!initialised_)
43174369
return kSignNotInitialised;
43184370

4319-
ClearErrorOnReturn clear_error_on_return;
4320-
43214371
EVP_PKEY* pkey = nullptr;
43224372
BIO* bp = nullptr;
43234373
X509* x509 = nullptr;
@@ -4406,6 +4456,8 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem,
44064456
void Verify::VerifyFinal(const FunctionCallbackInfo<Value>& args) {
44074457
Environment* env = Environment::GetCurrent(args);
44084458

4459+
ClearErrorOnReturn clear_error_on_return;
4460+
44094461
Verify* verify;
44104462
ASSIGN_OR_RETURN_UNWRAP(&verify, args.Holder());
44114463

‎test/parallel/test-crypto.js‎

Lines changed: 102 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,14 @@ assert.throws(function() {
4646
const context = credentials.context;
4747
const notcontext = { setOptions: context.setOptions, setKey: context.setKey };
4848
tls.createSecureContext({ secureOptions: 1 }, notcontext);
49-
}, /^TypeError: Illegal invocation$/);
49+
}, (err) => {
50+
// Throws TypeError, so there is no opensslErrorStack property.
51+
if ((err instanceof Error) &&
52+
/^TypeError: Illegal invocation$/.test(err) &&
53+
err.opensslErrorStack === undefined) {
54+
return true;
55+
}
56+
});
5057

5158
// PFX tests
5259
assert.doesNotThrow(function() {
@@ -55,15 +62,36 @@ assert.doesNotThrow(function() {
5562

5663
assert.throws(function() {
5764
tls.createSecureContext({ pfx: certPfx });
58-
}, /^Error: mac verify failure$/);
65+
}, (err) => {
66+
// Throws general Error, so there is no opensslErrorStack property.
67+
if ((err instanceof Error) &&
68+
/^Error: mac verify failure$/.test(err) &&
69+
err.opensslErrorStack === undefined) {
70+
return true;
71+
}
72+
});
5973

6074
assert.throws(function() {
6175
tls.createSecureContext({ pfx: certPfx, passphrase: 'test' });
62-
}, /^Error: mac verify failure$/);
76+
}, (err) => {
77+
// Throws general Error, so there is no opensslErrorStack property.
78+
if ((err instanceof Error) &&
79+
/^Error: mac verify failure$/.test(err) &&
80+
err.opensslErrorStack === undefined) {
81+
return true;
82+
}
83+
});
6384

6485
assert.throws(function() {
6586
tls.createSecureContext({ pfx: 'sample', passphrase: 'test' });
66-
}, /^Error: not enough data$/);
87+
}, (err) => {
88+
// Throws general Error, so there is no opensslErrorStack property.
89+
if ((err instanceof Error) &&
90+
/^Error: not enough data$/.test(err) &&
91+
err.opensslErrorStack === undefined) {
92+
return true;
93+
}
94+
});
6795

6896

6997
// update() should only take buffers / strings
@@ -135,23 +163,62 @@ testImmutability(crypto.getCurves);
135163
// throw, not assert in C++ land.
136164
assert.throws(function() {
137165
crypto.createCipher('aes192', 'test').update('0', 'hex');
138-
}, common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/);
166+
}, (err) => {
167+
const errorMessage =
168+
common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/;
169+
// Throws general Error, so there is no opensslErrorStack property.
170+
if ((err instanceof Error) &&
171+
errorMessage.test(err) &&
172+
err.opensslErrorStack === undefined) {
173+
return true;
174+
}
175+
});
139176

140177
assert.throws(function() {
141178
crypto.createDecipher('aes192', 'test').update('0', 'hex');
142-
}, common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/);
179+
}, (err) => {
180+
const errorMessage =
181+
common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/;
182+
// Throws general Error, so there is no opensslErrorStack property.
183+
if ((err instanceof Error) &&
184+
errorMessage.test(err) &&
185+
err.opensslErrorStack === undefined) {
186+
return true;
187+
}
188+
});
143189

144190
assert.throws(function() {
145191
crypto.createHash('sha1').update('0', 'hex');
146-
}, /^TypeError: Bad input string$/);
192+
}, (err) => {
193+
// Throws TypeError, so there is no opensslErrorStack property.
194+
if ((err instanceof Error) &&
195+
/^TypeError: Bad input string$/.test(err) &&
196+
err.opensslErrorStack === undefined) {
197+
return true;
198+
}
199+
});
147200

148201
assert.throws(function() {
149202
crypto.createSign('SHA1').update('0', 'hex');
150-
}, /^TypeError: Bad input string$/);
203+
}, (err) => {
204+
// Throws TypeError, so there is no opensslErrorStack property.
205+
if ((err instanceof Error) &&
206+
/^TypeError: Bad input string$/.test(err) &&
207+
err.opensslErrorStack === undefined) {
208+
return true;
209+
}
210+
});
151211

152212
assert.throws(function() {
153213
crypto.createVerify('SHA1').update('0', 'hex');
154-
}, /^TypeError: Bad input string$/);
214+
}, (err) => {
215+
// Throws TypeError, so there is no opensslErrorStack property.
216+
if ((err instanceof Error) &&
217+
/^TypeError: Bad input string$/.test(err) &&
218+
err.opensslErrorStack === undefined) {
219+
return true;
220+
}
221+
});
155222

156223
assert.throws(function() {
157224
const priv = [
@@ -164,7 +231,13 @@ assert.throws(function() {
164231
''
165232
].join('\n');
166233
crypto.createSign('SHA256').update('test').sign(priv);
167-
}, /digest too big for rsa key$/);
234+
}, (err) => {
235+
if ((err instanceof Error) &&
236+
/digest too big for rsa key$/.test(err) &&
237+
err.opensslErrorStack === undefined) {
238+
return true;
239+
}
240+
});
168241

169242
assert.throws(function() {
170243
// The correct header inside `test_bad_rsa_privkey.pem` should have been
@@ -180,14 +253,31 @@ assert.throws(function() {
180253
'ascii');
181254
// this would inject errors onto OpenSSL's error stack
182255
crypto.createSign('sha1').sign(sha1_privateKey);
183-
}, /asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag/);
256+
}, (err) => {
257+
// Throws crypto error, so there is an opensslErrorStack property.
258+
// The openSSL stack should have content.
259+
if ((err instanceof Error) &&
260+
/asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag/.test(err) &&
261+
err.opensslErrorStack !== undefined &&
262+
Array.isArray(err.opensslErrorStack) &&
263+
err.opensslErrorStack.length > 0) {
264+
return true;
265+
}
266+
});
184267

185268
// Make sure memory isn't released before being returned
186269
console.log(crypto.randomBytes(16));
187270

188271
assert.throws(function() {
189272
tls.createSecureContext({ crl: 'not a CRL' });
190-
}, /^Error: Failed to parse CRL$/);
273+
}, (err) => {
274+
// Throws general error, so there is no opensslErrorStack property.
275+
if ((err instanceof Error) &&
276+
/^Error: Failed to parse CRL$/.test(err) &&
277+
err.opensslErrorStack === undefined) {
278+
return true;
279+
}
280+
});
191281

192282
/**
193283
* Check if the stream function uses utf8 as a default encoding.

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL