GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: fix crypto.privateEncrypt fails first time by liuxingbaoyu · Pull Request #42793 · nodejs/node

Conversation

@liuxingbaoyu

@liuxingbaoyu liuxingbaoyu commented Apr 20, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Fix crypto.privateEncrypt fails for the first time after crypto.generateKeyPairSync with certain parameters

Because the error stack is not cleaned up when crypto.generateKeyPairSync exits.

Fixes: #40814

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Apr 20, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Apr 20, 2022
@liuxingbaoyu
src: fix crypto.privateEncrypt fails first time
b306018
crypto.privateEncrypt fails for the first time after
crypto.generateKeyPairSync with certain parameters
Because the error stack is not cleaned up
when crypto.generateKeyPairSync exits.

Fixes: nodejs#40814
@liuxingbaoyu
liuxingbaoyu force-pushed the fix-crypto branch from 686d32a to b306018 Compare April 20, 2022 06:45
bnoordhuis
bnoordhuis approved these changes Apr 20, 2022
View reviewed changes
tniessen
tniessen reviewed Apr 20, 2022
View reviewed changes

@tniessen tniessen left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be interesting to know why some ciphers populate the OpenSSL error queue even though WritePrivateKey appears to succeed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread test/parallel/test-crypto-aes-128-ecb.js
@@ -0,0 +1,35 @@
'use strict';

@tniessen tniessen Apr 20, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either rename this file to better match what it tests, or add the test case to an existing test file that tests key objects.

Also, please add a comment explaining that this is a regression test for #40814.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@liuxingbaoyu

liuxingbaoyu commented Apr 20, 2022

Copy link
Copy Markdown
Contributor Author

indeed! I'm also interested in why the exception is populated.

But I don't know about openssl and I didn't find relevant information on the documentation.

I'll show the stack for anyone with experience to investigate in depth.

Callstack:

node.exe!EVP_CIPHER_get_asn1_iv(evp_cipher_ctx_st * ctx, asn1_type_st * type) 行 56	C
node.exe!evp_cipher_asn1_to_param_ex(evp_cipher_ctx_st * c, asn1_type_st * type, evp_cipher_aead_asn1_params * asn1_params) 行 212	C
node.exe!EVP_CIPHER_asn1_to_param(evp_cipher_ctx_st * c, asn1_type_st * type) 行 42	C
node.exe!PKCS5_v2_PBE_keyivgen_ex(evp_cipher_ctx_st * ctx, const char * pass, int passlen, asn1_type_st * param, const evp_cipher_st * c, const evp_md_st * md, int en_de, ossl_lib_ctx_st * libctx, const char * propq) 行 160	C
node.exe!EVP_PBE_CipherInit_ex(asn1_object_st * pbe_obj, const char * pass, int passlen, asn1_type_st * param, evp_cipher_ctx_st * ctx, int en_de, ossl_lib_ctx_st * libctx, const char * propq) 行 151	C
node.exe!PKCS12_pbe_crypt_ex(const X509_algor_st * algor, const char * pass, int passlen, const unsigned char * in, int inlen, unsigned char * * data, int * datalen, int en_de, ossl_lib_ctx_st * libctx, const char * propq) 行 36	C
node.exe!PKCS12_item_i2d_encrypt_ex(X509_algor_st * algor, const ASN1_ITEM_st * it, const char * pass, int passlen, void * obj, int zbuf, ossl_lib_ctx_st * ctx, const char * propq) 行 189	C
node.exe!PKCS8_set0_pbe_ex(const char * pass, int passlen, pkcs8_priv_key_info_st * p8inf, X509_algor_st * pbe, ossl_lib_ctx_st * ctx, const char * propq) 行 76	C
node.exe!PKCS8_encrypt_ex(int pbe_nid, const evp_cipher_st * cipher, const char * pass, int passlen, unsigned char * salt, int saltlen, int iter, pkcs8_priv_key_info_st * p8inf, ossl_lib_ctx_st * libctx, const char * propq) 行 51	C
node.exe!p8info_to_encp8(pkcs8_priv_key_info_st * p8info, key2any_ctx_st * ctx) 行 120	C
node.exe!key_to_encp8(const void * key, int key_nid, void * params, int params_type, int(*)(const void *, unsigned char * *) k2d, key2any_ctx_st * ctx) 行 136	C
node.exe!key_to_epki_pem_priv_bio(bio_st * out, const void * key, int key_nid, const char * pemname, int(*)(const void *, int, int, void * *, int *) p2s, int(*)(const void *, unsigned char * *) k2d, key2any_ctx_st * ctx) 行 232	C
node.exe!key_to_pki_pem_priv_bio(bio_st * out, const void * key, int key_nid, const char * pemname, int(*)(const void *, int, int, void * *, int *) p2s, int(*)(const void *, unsigned char * *) k2d, key2any_ctx_st * ctx) 行 286	C
node.exe!key2any_encode(key2any_ctx_st * ctx, ossl_core_bio_st * cout, const void * key, int type, const char * pemname, int(*)(const void *, int) checker, int(*)(bio_st *, const void *, int, const char *, int(*)(const void *, int, int, void * *, int *), int(*)(const void *, unsigned char * *), key2any_ctx_st *) writer, int(*)(char *, unsigned __int64, unsigned __int64 *, const ossl_param_st *, void *) pwcb, void * pwcbarg, int(*)(const void *, int, int, void * *, int *) key2paramstring, int(*)(const void *, unsigned char * *) key2der) 行 1054	C
node.exe!rsa_to_PrivateKeyInfo_pem_encode(void * ctx, ossl_core_bio_st * cout, const void * key, const ossl_param_st * key_abstract, int selection, int(*)(char *, unsigned __int64, unsigned __int64 *, const ossl_param_st *, void *) cb, void * cbarg) 行 1350	C
node.exe!encoder_process(encoder_process_data_st * data) 行 635	C
node.exe!OSSL_ENCODER_to_bio(ossl_encoder_ctx_st * ctx, bio_st * out) 行 63	C
node.exe!do_pk8pkey(bio_st * bp, const evp_pkey_st * x, int isder, int nid, const evp_cipher_st * enc, const char * kstr, int klen, int(*)(char *, int, int, void *) cb, void * u, const char * propq) 行 125	C
node.exe!PEM_write_bio_PKCS8PrivateKey(bio_st * bp, const evp_pkey_st * x, const evp_cipher_st * enc, const char * kstr, int klen, int(*)(char *, int, int, void *) cb, void * u) 行 53	C
node.exe!node::crypto::`anonymous namespace'::WritePrivateKey(node::Environment * env, evp_pkey_st * pkey, const node::crypto::PrivateKeyEncodingConfig & config) 行 347	C++
node.exe!node::crypto::ManagedEVPPKey::ToEncodedPrivateKey(node::Environment * env, node::crypto::ManagedEVPPKey key, const node::crypto::PrivateKeyEncodingConfig & config, v8::Local<v8::Value> * out) 行 673	C++
node.exe!node::crypto::KeyPairGenTraits<node::crypto::RsaKeyGenTraits>::EncodeKey(node::Environment * env, node::crypto::KeyPairGenConfig<node::crypto::RsaKeyPairParams> * params, v8::Local<v8::Value> * result) 行 196	C++
node.exe!node::crypto::KeyGenJob<node::crypto::KeyPairGenTraits<node::crypto::RsaKeyGenTraits>>::ToResult(v8::Local<v8::Value> * err, v8::Local<v8::Value> * result) 行 107	C++
node.exe!node::crypto::CryptoJob<node::crypto::KeyPairGenTraits<node::crypto::RsaKeyGenTraits>>::Run(const v8::FunctionCallbackInfo<v8::Value> & args) 行 412	C++
node.exe!v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo handler) 行 153	C++
node.exe!v8::internal::`anonymous namespace'::HandleApiCallHelper<0>(v8::internal::Isolate * isolate, v8::internal::Handle<v8::internal::HeapObject> function, v8::internal::Handle<v8::internal::HeapObject> new_target, v8::internal::Handle<v8::internal::FunctionTemplateInfo> fun_data, v8::internal::Handle<v8::internal::Object> receiver, v8::internal::BuiltinArguments args) 行 114	C++
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@liuxingbaoyu

liuxingbaoyu commented Apr 20, 2022

Copy link
Copy Markdown
Contributor Author

Also, do we need to add an assertion or something?

Once any method is called and the error is left, it is easy to affect other methods.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@liuxingbaoyu
modify test
9a7c063
@liuxingbaoyu
liuxingbaoyu force-pushed the fix-crypto branch from 96487d5 to 9a7c063 Compare April 20, 2022 18:41
jasnell
jasnell approved these changes Apr 20, 2022
View reviewed changes
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 21, 2022
panva
panva approved these changes Apr 21, 2022
View reviewed changes
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 21, 2022
@nodejs-github-bot

nodejs-github-bot commented Apr 21, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/43624/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@panva

panva commented Apr 22, 2022

Copy link
Copy Markdown
Member

@liuxingbaoyu I believe there's a consistent failure when using linked OpenSSL 1.1.1

  • ubi81_sharedlibs_openssl111fips_x64
  • ubuntu1804_sharedlibs_openssl111_x64
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@liuxingbaoyu

liuxingbaoyu commented Apr 23, 2022

Copy link
Copy Markdown
Contributor Author

This looks to be another problem with crypto.privateEncrypt, not crypto.generateKeyPairSync.

In openssl3, the error is cleared inside openssl, but not in openssl1.1.1.

I don't know if this is a bug or a behavior change.

Exception source:

    ERR_set_mark();
    ret = pem_read_bio_key_decoder(bp, x, ossl_pw_pem_password, &pwdata,
                                   libctx, propq, selection);
    if (ret == NULL
        && (BIO_seek(bp, pos) < 0
            || (ret = pem_read_bio_key_legacy(bp, x,
                                              ossl_pw_pem_password, &pwdata,
                                              libctx, propq,
                                              selection)) == NULL))
        ERR_clear_last_mark();
    else
        ERR_pop_to_mark();

node/src/crypto/crypto_keys.cc

Lines 257 to 258 in 6b004f1

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

Also, does anyone know what's the reason here?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@liuxingbaoyu liuxingbaoyu mentioned this pull request May 3, 2022
pem_read_bio_key has additional exceptions in openssl1.1.1 than openssl3 openssl/openssl#18228
Closed
@liuxingbaoyu
fix test
12c8ba7
@tniessen tniessen added the review wanted PRs that need review. label Sep 11, 2022
@aduh95

aduh95 commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

Ping @nodejs/crypto for (re)reviews.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis
bnoordhuis approved these changes Oct 29, 2022
View reviewed changes
@bnoordhuis bnoordhuis added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed needs-ci PRs that need a full CI run. labels Oct 29, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 29, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 29, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47557/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. needs-ci PRs that need a full CI run. labels Oct 29, 2022
@aduh95 aduh95 added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Oct 29, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Oct 29, 2022
@nodejs-github-bot

nodejs-github-bot commented Oct 29, 2022

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/42793
FetchError: Invalid response body while trying to fetch https://api.github.com/graphql: Premature close
    at consumeBody (file:///opt/hostedtoolcache/node/18.12.0/x64/lib/node_modules/node-core-utils/node_modules/node-fetch/src/body.js:234:60)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Response.text (file:///opt/hostedtoolcache/node/18.12.0/x64/lib/node_modules/node-core-utils/node_modules/node-fetch/src/body.js:158:18)
    at async Request.json (file:///opt/hostedtoolcache/node/18.12.0/x64/lib/node_modules/node-core-utils/lib/request.js:51:18)
    at async Request.query (file:///opt/hostedtoolcache/node/18.12.0/x64/lib/node_modules/node-core-utils/lib/request.js:109:20)
    at async Request.queryAll (file:///opt/hostedtoolcache/node/18.12.0/x64/lib/node_modules/node-core-utils/lib/request.js:136:20)
    at async Request.gql (file:///opt/hostedtoolcache/node/18.12.0/x64/lib/node_modules/node-core-utils/lib/request.js:66:22)
    at async PRData.getComments (file:///opt/hostedtoolcache/node/18.12.0/x64/lib/node_modules/node-core-utils/lib/pr_data.js:97:21)
    at async Promise.all (index 2)
    at async Promise.all (index 1) {
  type: 'system',
  errno: 'ERR_STREAM_PREMATURE_CLOSE',
  code: 'ERR_STREAM_PREMATURE_CLOSE',
  erroredSysCall: undefined
}
https://github.com/nodejs/node/actions/runs/3352810353
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95
aduh95 merged commit e512786 into nodejs:main Oct 29, 2022
@aduh95

aduh95 commented Oct 29, 2022

Copy link
Copy Markdown
Contributor

Landed in e512786

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@liuxingbaoyu
liuxingbaoyu deleted the fix-crypto branch October 30, 2022 03:45
@liuxingbaoyu
liuxingbaoyu restored the fix-crypto branch October 30, 2022 05:49
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
@liuxingbaoyu @RafaelGSS
src: fix crypto.privateEncrypt fails first time
ffc95bc
`crypto.privateEncrypt` fails for the first time after
`crypto.generateKeyPairSync` with certain parameters
because the error stack is not cleaned up
when `crypto.generateKeyPairSync` exits.

Fixes: #40814
PR-URL: #42793
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
v19.1.0 proposal #45269
Merged
@PANCHO7532B PANCHO7532B mentioned this pull request Nov 2, 2022
Exceptions when using privateEncrypt/privateDecrypt with certain ciphers #45292
Closed
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@liuxingbaoyu @RafaelGSS
src: fix crypto.privateEncrypt fails first time
aa4152a
`crypto.privateEncrypt` fails for the first time after
`crypto.generateKeyPairSync` with certain parameters
because the error stack is not cleaned up
when `crypto.generateKeyPairSync` exits.

Fixes: #40814
PR-URL: #42793
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@liuxingbaoyu @danielleadams
src: fix crypto.privateEncrypt fails first time
afd90f5
`crypto.privateEncrypt` fails for the first time after
`crypto.generateKeyPairSync` with certain parameters
because the error stack is not cleaned up
when `crypto.generateKeyPairSync` exits.

Fixes: #40814
PR-URL: #42793
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@liuxingbaoyu @danielleadams
src: fix crypto.privateEncrypt fails first time
614d646
`crypto.privateEncrypt` fails for the first time after
`crypto.generateKeyPairSync` with certain parameters
because the error stack is not cleaned up
when `crypto.generateKeyPairSync` exits.

Fixes: #40814
PR-URL: #42793
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
@etray7 etray7 mentioned this pull request Oct 12, 2026
crypto: scrypt with params over maxmem leaves an OpenSSL error queued; the next createPrivateKey fails #66668
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen left review comments
@panva panva panva approved these changes
@bnoordhuis bnoordhuis bnoordhuis approved these changes
@jasnell jasnell jasnell approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. commit-queue-squash PRs the Commit Queue should land as one squashed commit. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. review wanted PRs that need review.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Exception when using privateEncrypt/privateDecrypt with certain encrypted keys

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@liuxingbaoyu @nodejs-github-bot @panva @aduh95 @bnoordhuis @jasnell @tniessen
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.