Skip to content

Commit 50b5f8b

Browse files
rfkjasnell
authored andcommitted
crypto: clear err stack after ECDH::BufferToPoint
Functions that call `ECDH::BufferToPoint` were not clearing the error stack on failure, so an invalid key could leave leftover error state and cause subsequent (unrelated) signing operations to fail. PR-URL: #13275 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent c7ebf6e commit 50b5f8b

File tree

2 files changed

+27
-0
lines changed

2 files changed

+27
-0
lines changed

src/node_crypto.cc

+4
Original file line numberDiff line numberDiff line change
@@ -5136,6 +5136,8 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo<Value>& args) {
51365136
ECDH* ecdh;
51375137
ASSIGN_OR_RETURN_UNWRAP(&ecdh, args.Holder());
51385138

5139+
MarkPopErrorOnReturn mark_pop_error_on_return;
5140+
51395141
if (!ecdh->IsKeyPairValid())
51405142
return env->ThrowError("Invalid key pair");
51415143

@@ -5282,6 +5284,8 @@ void ECDH::SetPublicKey(const FunctionCallbackInfo<Value>& args) {
52825284

52835285
THROW_AND_RETURN_IF_NOT_BUFFER(args[0], "Public key");
52845286

5287+
MarkPopErrorOnReturn mark_pop_error_on_return;
5288+
52855289
EC_POINT* pub = ecdh->BufferToPoint(Buffer::Data(args[0].As<Object>()),
52865290
Buffer::Length(args[0].As<Object>()));
52875291
if (pub == nullptr)

test/parallel/test-crypto-dh.js

+23
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@ assert.strictEqual(bad_dh.verifyError, DH_NOT_SUITABLE_GENERATOR);
172172

173173

174174
const availableCurves = new Set(crypto.getCurves());
175+
const availableHashes = new Set(crypto.getHashes());
175176

176177
// Oakley curves do not clean up ERR stack, it was causing unexpected failure
177178
// when accessing other OpenSSL APIs afterwards.
@@ -307,6 +308,28 @@ if (availableCurves.has('prime256v1') && availableCurves.has('secp256k1')) {
307308
});
308309
}
309310

311+
// Use of invalid keys was not cleaning up ERR stack, and was causing
312+
// unexpected failure in subsequent signing operations.
313+
if (availableCurves.has('prime256v1') && availableHashes.has('sha256')) {
314+
const curve = crypto.createECDH('prime256v1');
315+
const invalidKey = Buffer.alloc(65);
316+
invalidKey.fill('\0');
317+
curve.generateKeys();
318+
assert.throws(() => {
319+
curve.computeSecret(invalidKey);
320+
}, /^Error: Failed to translate Buffer to a EC_POINT$/);
321+
// Check that signing operations are not impacted by the above error.
322+
const ecPrivateKey =
323+
'-----BEGIN EC PRIVATE KEY-----\n' +
324+
'MHcCAQEEIF+jnWY1D5kbVYDNvxxo/Y+ku2uJPDwS0r/VuPZQrjjVoAoGCCqGSM49\n' +
325+
'AwEHoUQDQgAEurOxfSxmqIRYzJVagdZfMMSjRNNhB8i3mXyIMq704m2m52FdfKZ2\n' +
326+
'pQhByd5eyj3lgZ7m7jbchtdgyOF8Io/1ng==\n' +
327+
'-----END EC PRIVATE KEY-----';
328+
assert.doesNotThrow(() => {
329+
crypto.createSign('SHA256').sign(ecPrivateKey);
330+
});
331+
}
332+
310333
// invalid test: curve argument is undefined
311334
assert.throws(() => {
312335
crypto.createECDH();

0 commit comments

Comments
 (0)