From 042e2640e68832554733377245a3d6d4f76a7002 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 16 Feb 2019 20:26:26 +0100 Subject: [PATCH] crypto: make ConvertKey clear openssl error stack Failed ConvertKey() operations should not leave errors on OpenSSL's error stack because that's observable by subsequent cryptographic operations. More to the point, it makes said operations fail with an unrelated error. PR-URL: https://github.com/nodejs/node/pull/26153 Fixes: https://github.com/nodejs/node/issues/26133 Reviewed-By: Colin Ihrig Reviewed-By: Daniel Bevenius Reviewed-By: James M Snell --- src/node_crypto.cc | 1 + test/parallel/test-crypto-ecdh-convert-key.js | 26 ++++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index aaba05b984f54f..fcc35ce63fdd9a 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -6122,6 +6122,7 @@ void ExportChallenge(const FunctionCallbackInfo& args) { // Convert the input public key to compressed, uncompressed, or hybrid formats. void ConvertKey(const FunctionCallbackInfo& args) { + MarkPopErrorOnReturn mark_pop_error_on_return; Environment* env = Environment::GetCurrent(args); CHECK_EQ(args.Length(), 3); diff --git a/test/parallel/test-crypto-ecdh-convert-key.js b/test/parallel/test-crypto-ecdh-convert-key.js index 407178a1953250..52d0fc2b7f0c74 100644 --- a/test/parallel/test-crypto-ecdh-convert-key.js +++ b/test/parallel/test-crypto-ecdh-convert-key.js @@ -5,7 +5,7 @@ if (!common.hasCrypto) const assert = require('assert'); -const { ECDH, getCurves } = require('crypto'); +const { ECDH, createSign, getCurves } = require('crypto'); // A valid private key for the secp256k1 curve. const cafebabeKey = 'cafebabe'.repeat(8); @@ -99,3 +99,27 @@ if (getCurves().includes('secp256k1')) { assert.strictEqual(ecdh1.getPublicKey('hex', 'compressed'), compressed); assert.strictEqual(ecdh1.getPublicKey('hex', 'hybrid'), hybrid); } + +// See https://github.com/nodejs/node/issues/26133, failed ConvertKey +// operations should not leave errors on OpenSSL's error stack because +// that's observable by subsequent operations. +{ + const privateKey = + '-----BEGIN EC PRIVATE KEY-----\n' + + 'MHcCAQEEIF+jnWY1D5kbVYDNvxxo/Y+ku2uJPDwS0r/VuPZQrjjVoAoGCCqGSM49\n' + + 'AwEHoUQDQgAEurOxfSxmqIRYzJVagdZfMMSjRNNhB8i3mXyIMq704m2m52FdfKZ2\n' + + 'pQhByd5eyj3lgZ7m7jbchtdgyOF8Io/1ng==\n' + + '-----END EC PRIVATE KEY-----'; + + const sign = createSign('sha256').update('plaintext'); + + // TODO(bnoordhuis) This should really bubble up the specific OpenSSL error + // rather than Node's generic error message. + const badKey = 'f'.repeat(128); + assert.throws( + () => ECDH.convertKey(badKey, 'secp256k1', 'hex', 'hex', 'compressed'), + /Failed to convert Buffer to EC_POINT/); + + // Next statement should not throw an exception. + sign.sign(privateKey); +}