Skip to content

crypto: fix race breaking RSA-PSS key decoding - #66935

Closed
christianaurichzm wants to merge 1 commit into
nodejs:mainfrom
christianaurichzm:crypto-off-thread-name-map
Closed

christianaurichzm wants to merge 1 commit into
nodejs:mainfrom
christianaurichzm:crypto-off-thread-name-map

Conversation

@christianaurichzm

Copy link
Copy Markdown
Contributor

An RSA-PSS public key can fail to decode when the first OpenSSL fetch of the process happens on a thread that Node.js started, and from then on every RSA-PSS decode in the process fails:

const { X509Certificate, pbkdf2 } = require('crypto');
const pem = require('fs').readFileSync('test/fixtures/keys/rsa_pss_cert_2048.pem');
pbkdf2('', '', 1, 32, 'sha256', () => {});
new X509Certificate(pem).publicKey;
$ for i in $(seq 30); do ./node repro.js; done 2>&1 | grep -c ERR_OSSL_EVP_DECODE_ERROR
11

The same happens with require('tls'), which loads the root certificates on another thread, and with a Worker that hashes something as it starts. It is also the intermittent parallel/test-crypto-x509 failure on GitHub Actions since 697ecce added require('tls') to that test (1, 2, 3). [2] ran on v26.x-staging.

On the first fetch, OpenSSL 3 copies its legacy algorithm names into the name map (ossl_namemap_stored()). The empty check and the copy are not under one lock, so another thread can look names up while the copy is still running. The SPKI decoder looks the key type up by the long name of its OID (x_pubkey.c), which for RSA-PSS is rsassaPss, and the 3.5 default provider doesn't register that name. In that window it resolves to nothing, and the empty decoder set gets cached. Until #65796 the CSPRNG check at startup did the first fetch before any other thread existed, and v22.23.1 doesn't fail.

This adds EnsureOpenSSLNameMap(), which does one fetch on the calling thread, once per process. It runs before a crypto job is scheduled, before the certificate loading threads start and before a Worker thread starts. Startup still does no fetch. The first pbkdf2(), require('tls') or new Worker() gets 0.3 to 0.5 ms slower, once.

Over 3000 runs, 16 at a time, failures go from 832 to 0 with pbkdf2(), from 44 to 0 with require('tls') and from 264 to 0 with a Worker. test/parallel/test-crypto-off-thread-first-use.js fails on main and passes with this change, and parallel, sequential and cctest pass locally. Threads that a native addon starts itself are still not covered.

Refs: #65796

Since 43d3fe9, startup no longer fetches anything from OpenSSL, so
the first fetch can happen on a thread pool crypto job, on the root
certificate loader that `require('tls')` starts, or in a Worker.
OpenSSL 3 copies its legacy algorithm names into the name map on that
first fetch, and other threads can look names up before the copy is
done. An RSA-PSS key decoded on the main thread meanwhile resolves its
OID name, `rsassaPss`, to nothing, and OpenSSL 3.5 caches the
resulting empty decoder set. From then on every RSA-PSS public key
decode in the process fails with `ERR_OSSL_EVP_DECODE_ERROR`.

Do one fetch on the calling thread before a crypto job runs, before
the certificate loading threads start, and before a Worker thread
starts.

Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@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. worker Issues and PRs related to the worker_threads module and Worker API. labels Oct 22, 2026
@christianaurichzm

Copy link
Copy Markdown
Contributor Author

@panva @jasnell, since you reviewed #65796, could you take a look at this? If it looks good, could you also start a Jenkins CI run? Thanks!

@panva

panva commented Oct 22, 2026

Copy link
Copy Markdown
Member

@christianaurichzm see #66903, i think this isn't needed when we revert the PRs that triggered this.

@christianaurichzm

Copy link
Copy Markdown
Contributor Author

Thanks, I hadn't seen #66903. I tried main with the revert and all three cases here pass (0 of 3000), so I'm closing this one.

The revert doesn't add a test for this though. Want me to send test-crypto-off-thread-first-use.js as a test-only PR after it lands?

@panva

panva commented Oct 22, 2026

Copy link
Copy Markdown
Member

That would be nice, i also have a repro script in that PR's comment, see if you can combine yours with mine (if that makes sense), up to you.

@codecov

codecov Bot commented Oct 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.58%. Comparing base (53f037e) to head (38c9121).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66935      +/-   ##
==========================================
- Coverage   90.59%   90.58%   -0.02%     
==========================================
  Files         797      797              
  Lines      278475   278482       +7     
  Branches    53533    53539       +6     
==========================================
- Hits       252293   252255      -38     
- Misses      16532    16560      +28     
- Partials     9650     9667      +17     
Files with missing lines Coverage Δ
src/crypto/crypto_context.cc 72.49% <100.00%> (+0.04%) ⬆️
src/crypto/crypto_util.cc 51.54% <100.00%> (+0.06%) ⬆️
src/crypto/crypto_util.h 66.75% <100.00%> (+0.09%) ⬆️
src/node_worker.cc 81.89% <100.00%> (-0.48%) ⬇️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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. worker Issues and PRs related to the worker_threads module and Worker API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants