Skip to content

Commit

Permalink
crypto: fix error code handling in ParsePrivateKey()
Browse files Browse the repository at this point in the history
This changes the code to select the latest error code instead of the
earliest one from the OpenSSL error stack. It helps in getting rid of
the inconsistency between the empty passphrase related error codes of
OpenSSL 1.1.1 and 3.

Refs: nodejs#42319 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
  • Loading branch information
RaisinTen committed Mar 19, 2022
1 parent 46a0d0d commit a60a849
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 39 deletions.
2 changes: 1 addition & 1 deletion src/crypto/crypto_context.cc
Expand Up @@ -143,7 +143,7 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
X509Pointer* cert,
X509Pointer* issuer) {
// Just to ensure that `ERR_peek_last_error` below will return only errors
// that we are interested in
// that we are interested in.
ERR_clear_error();

X509Pointer x(
Expand Down
6 changes: 5 additions & 1 deletion src/crypto/crypto_keys.cc
Expand Up @@ -214,6 +214,10 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey,
const PrivateKeyEncodingConfig& config,
const char* key,
size_t key_len) {
// Just to ensure that `ERR_peek_last_error` below will return only errors
// that we are interested in.
ERR_clear_error();

const ByteSource* passphrase = config.passphrase_.get();

if (config.format_ == kKeyFormatPEM) {
Expand Down Expand Up @@ -255,7 +259,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey,
}

// OpenSSL can fail to parse the key but still return a non-null pointer.
unsigned long err = ERR_peek_error(); // NOLINT(runtime/int)
unsigned long err = ERR_peek_last_error(); // NOLINT(runtime/int)
if (err != 0)
pkey->reset();

Expand Down
6 changes: 1 addition & 5 deletions test/parallel/test-crypto-key-objects.js
Expand Up @@ -518,11 +518,7 @@ const privateDsa = fixtures.readKey('dsa_private_encrypted_1025.pem',

{
// Reading an encrypted key without a passphrase should fail.
assert.throws(() => createPrivateKey(privateDsa), common.hasOpenSSL3 ? {
name: 'Error',
message: 'error:07880109:common libcrypto routines::interrupted or ' +
'cancelled',
} : {
assert.throws(() => createPrivateKey(privateDsa), {
name: 'TypeError',
code: 'ERR_MISSING_PASSPHRASE',
message: 'Passphrase required for encrypted key'
Expand Down
35 changes: 7 additions & 28 deletions test/parallel/test-crypto-keygen.js
Expand Up @@ -211,16 +211,11 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher);

// Since the private key is encrypted, signing shouldn't work anymore.
const publicKey = { key: publicKeyDER, ...publicKeyEncoding };
const expectedError = common.hasOpenSSL3 ? {
name: 'Error',
message: 'error:07880109:common libcrypto routines::interrupted or ' +
'cancelled'
} : {
assert.throws(() => testSignVerify(publicKey, privateKey), {
name: 'TypeError',
code: 'ERR_MISSING_PASSPHRASE',
message: 'Passphrase required for encrypted key'
};
assert.throws(() => testSignVerify(publicKey, privateKey), expectedError);
});

const key = { key: privateKey, passphrase: 'secret' };
testEncryptDecrypt(publicKey, key);
Expand Down Expand Up @@ -565,10 +560,7 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher);

// Since the private key is encrypted, signing shouldn't work anymore.
assert.throws(() => testSignVerify(publicKey, privateKey),
common.hasOpenSSL3 ? {
message: 'error:07880109:common libcrypto ' +
'routines::interrupted or cancelled'
} : {
{
name: 'TypeError',
code: 'ERR_MISSING_PASSPHRASE',
message: 'Passphrase required for encrypted key'
Expand Down Expand Up @@ -599,10 +591,7 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher);

// Since the private key is encrypted, signing shouldn't work anymore.
assert.throws(() => testSignVerify(publicKey, privateKey),
common.hasOpenSSL3 ? {
message: 'error:07880109:common libcrypto ' +
'routines::interrupted or cancelled'
} : {
{
name: 'TypeError',
code: 'ERR_MISSING_PASSPHRASE',
message: 'Passphrase required for encrypted key'
Expand Down Expand Up @@ -636,10 +625,7 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher);

// Since the private key is encrypted, signing shouldn't work anymore.
assert.throws(() => testSignVerify(publicKey, privateKey),
common.hasOpenSSL3 ? {
message: 'error:07880109:common libcrypto ' +
'routines::interrupted or cancelled'
} : {
{
name: 'TypeError',
code: 'ERR_MISSING_PASSPHRASE',
message: 'Passphrase required for encrypted key'
Expand Down Expand Up @@ -674,10 +660,7 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher);

// Since the private key is encrypted, signing shouldn't work anymore.
assert.throws(() => testSignVerify(publicKey, privateKey),
common.hasOpenSSL3 ? {
message: 'error:07880109:common libcrypto ' +
'routines::interrupted or cancelled'
} : {
{
name: 'TypeError',
code: 'ERR_MISSING_PASSPHRASE',
message: 'Passphrase required for encrypted key'
Expand Down Expand Up @@ -1571,11 +1554,7 @@ for (const type of ['pkcs1', 'pkcs8']) {
// the key, and not specifying a passphrase should fail when decoding it.
assert.throws(() => {
return testSignVerify(publicKey, privateKey);
}, common.hasOpenSSL3 ? {
name: 'Error',
code: 'ERR_OSSL_CRYPTO_INTERRUPTED_OR_CANCELLED',
message: 'error:07880109:common libcrypto routines::interrupted or cancelled'
} : {
}, {
name: 'TypeError',
code: 'ERR_MISSING_PASSPHRASE',
message: 'Passphrase required for encrypted key'
Expand Down
5 changes: 1 addition & 4 deletions test/parallel/test-crypto-private-decrypt-gh32240.js
Expand Up @@ -34,8 +34,5 @@ function decrypt(key) {
}

decrypt(pkey);
assert.throws(() => decrypt(pkeyEncrypted), common.hasOpenSSL3 ?
{ message: 'error:07880109:common libcrypto routines::interrupted or ' +
'cancelled' } :
{ code: 'ERR_MISSING_PASSPHRASE' });
assert.throws(() => decrypt(pkeyEncrypted), { code: 'ERR_MISSING_PASSPHRASE' });
decrypt(pkey); // Should not throw.

0 comments on commit a60a849

Please sign in to comment.