Commit 108c698
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: #26153
Fixes: #26133
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>1 parent a3f7471 commit 108c698
2 files changed
Lines changed: 26 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -6095,6 +6095,7 @@ void ExportChallenge(const FunctionCallbackInfo<Value>& args) { | |||
| 6095 | 6095 | ||
| 6096 | 6096 | // Convert the input public key to compressed, uncompressed, or hybrid formats. | |
| 6097 | 6097 | void ConvertKey(const FunctionCallbackInfo<Value>& args) { | |
| 6098 | + MarkPopErrorOnReturn mark_pop_error_on_return; | ||
| 6098 | 6099 | Environment* env = Environment::GetCurrent(args); | |
| 6099 | 6100 | ||
| 6100 | 6101 | CHECK_EQ(args.Length(), 3); | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -5,7 +5,7 @@ if (!common.hasCrypto) | |||
| 5 | 5 | ||
| 6 | 6 | const assert = require('assert'); | |
| 7 | 7 | ||
| 8 | - const { ECDH, getCurves } = require('crypto'); | ||
| 8 | + const { ECDH, createSign, getCurves } = require('crypto'); | ||
| 9 | 9 | ||
| 10 | 10 | // A valid private key for the secp256k1 curve. | |
| 11 | 11 | const cafebabeKey = 'cafebabe'.repeat(8); | |
@@ -99,3 +99,27 @@ if (getCurves().includes('secp256k1')) { | |||
| 99 | 99 | assert.strictEqual(ecdh1.getPublicKey('hex', 'compressed'), compressed); | |
| 100 | 100 | assert.strictEqual(ecdh1.getPublicKey('hex', 'hybrid'), hybrid); | |
| 101 | 101 | } | |
| 102 | + | ||
| 103 | + // See https://github.com/nodejs/node/issues/26133, failed ConvertKey | ||
| 104 | + // operations should not leave errors on OpenSSL's error stack because | ||
| 105 | + // that's observable by subsequent operations. | ||
| 106 | + { | ||
| 107 | + const privateKey = | ||
| 108 | + '-----BEGIN EC PRIVATE KEY-----\n' + | ||
| 109 | + 'MHcCAQEEIF+jnWY1D5kbVYDNvxxo/Y+ku2uJPDwS0r/VuPZQrjjVoAoGCCqGSM49\n' + | ||
| 110 | + 'AwEHoUQDQgAEurOxfSxmqIRYzJVagdZfMMSjRNNhB8i3mXyIMq704m2m52FdfKZ2\n' + | ||
| 111 | + 'pQhByd5eyj3lgZ7m7jbchtdgyOF8Io/1ng==\n' + | ||
| 112 | + '-----END EC PRIVATE KEY-----'; | ||
| 113 | + | ||
| 114 | + const sign = createSign('sha256').update('plaintext'); | ||
| 115 | + | ||
| 116 | + // TODO(bnoordhuis) This should really bubble up the specific OpenSSL error | ||
| 117 | + // rather than Node's generic error message. | ||
| 118 | + const badKey = 'f'.repeat(128); | ||
| 119 | + assert.throws( | ||
| 120 | + () => ECDH.convertKey(badKey, 'secp256k1', 'hex', 'hex', 'compressed'), | ||
| 121 | + /Failed to convert Buffer to EC_POINT/); | ||
| 122 | + | ||
| 123 | + // Next statement should not throw an exception. | ||
| 124 | + sign.sign(privateKey); | ||
| 125 | + } | ||
0 commit comments