Uh oh!
There was an error while loading. Please reload this page.
crypto: support provider-only SM4 cipher modes - #65399
Conversation
nodejs-github-bot
commented
Aug 19, 2026
Review requested:
|
Fixes: nodejs#64866 Co-authored-by: StefanStojanovic <stefan.stojanovic@janeasystems.com> Signed-off-by: Kirill Saied <sayed.kirill@gmail.com>
6cb1a8a to
491dc6dCompareCodecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #65399 +/- ##
==========================================
- Coverage 90.30% 90.11% -0.20%
==========================================
Files 759 752 -7 Lines 247648 251864 +4216 Branches 46696 47355 +659 ==========================================
+ Hits 223644 226970 +3326 - Misses 15466 16230 +764 - Partials 8538 8664 +126 🚀 New features to boost your workflow:
|
panva
left a comment
There was a problem hiding this comment.
This needs to wait for and be rebased on #63411 before landing. Both PRs modify the fetched-cipher lookup, ownership, metadata, and provider-only enumeration paths. #63411 now provides owned fetched handles and the authenticated-mode handling required for AES-SIV and AES-GCM-SIV.
The unrestricted fallback here currently makes every provider-fetchable cipher appear supported. On this head, AES-SIV resolves through createCipheriv() even though setAAD() and getAuthTag() fail, and provider algorithms without an OBJ NID can be reported as name: 'undef', nid: 0.
Please retain #63411's ownership and metadata handling, then extend its explicit eligibility and enumeration paths for SM4-GCM, SM4-CCM, and SM4-XTS. The process-lifetime fetched-cipher cache should not be carried over.
The FIPS test also needs correction: getCiphers() is memoized before the FIPS transition, and the test can run with OpenSSL 3.0 even though SM4-GCM requires OpenSSL 3.1.
| if (auto it = fetched_ciphers.find(key); it != fetched_ciphers.end()) { | ||
| return Cipher(it->second); | ||
| } | ||
| if (const EVP_CIPHER* fetched = EVP_CIPHER_fetch(nullptr, name, nullptr)) { |
There was a problem hiding this comment.
This cannot be an unrestricted fallback. It makes every fetchable provider cipher a Node-supported cipher before CipherBase has implemented that mode's contract. With this head on OpenSSL 3.0, createCipheriv('aes-128-siv', ...) succeeds, while setAAD() and getAuthTag() both throw ERR_CRYPTO_INVALID_STATE; the cipher is also absent from getCiphers().
Please rebase on #63411 and only return a fetched cipher when its mode or name is explicitly supported by Node. The SM4 additions should extend that eligibility rather than accepting every successful EVP_CIPHER_fetch().
| #if NCRYPTO_USE_OPENSSL3_PROVIDER | ||
| // EVP_CIPHER_do_all_sorted() walks the legacy name table, so provider-only | ||
| // algorithms have to be probed for by name. | ||
| static constexpr const char* kProviderOnlyCiphers[] = { |
There was a problem hiding this comment.
| return EVP_CIPHER_nid(cipher_); | ||
| int nid = EVP_CIPHER_nid(cipher_); | ||
| #if NCRYPTO_USE_OPENSSL3_PROVIDER | ||
| if (nid == NID_undef) { |
There was a problem hiding this comment.
This recovery only works when the provider algorithm's name is registered in the OBJ database. The generic fallback above also exposes algorithms without an OBJ NID: with OpenSSL 3.0, getCipherInfo('aes-128-cbc-cts') resolves but reports name: 'undef' and nid: 0.
Please preserve #63411's behavior of using EVP_CIPHER_get0_name() when no NID exists and omitting nid from the JavaScript result when it remains NID_undef.
| // A fetch is resolved against the library context's default properties, | ||
| // which setFipsEnabled() changes at runtime. Key on that state as well so | ||
| // that a cipher fetched before the switch cannot outlive it. | ||
| std::string key(EVP_default_properties_is_fips_enabled(nullptr) ? "fips:" |
There was a problem hiding this comment.
Reading the FIPS state and performing the fetch are not synchronized with setFipsCrypto(). This code uses fetched_mutex, while FIPS transitions use a separate fips_mutex.
A worker can read the state as enabled, the main thread can disable FIPS, and the worker can then fetch a non-FIPS implementation and store it under the fips: key. If FIPS is enabled again, that cached implementation is reused.
Please retain #63411's owned, per-Cipher fetched handles when rebasing instead of introducing this process-lifetime property cache.
| const iv = Buffer.alloc(12); | ||
| // Populate the cache while FIPS is still disabled. | ||
| assert(crypto.getCiphers().includes('sm4-gcm')); |
There was a problem hiding this comment.
test-crypto-sm4-aead.js correctly gates these modes on OpenSSL 3.1, but this test only checks for sm4-cbc. A FIPS-capable OpenSSL 3.0 build has SM4-CBC but not SM4-GCM, so it reaches this assertion and fails before exercising the FIPS transition.
Please import hasOpenSSL from ../common/crypto and add the same hasOpenSSL(3, 1) skip used by the AEAD test.
| crypto.setFips(true); | ||
| assert.strictEqual(crypto.getFips(), 1); | ||
| assert(!crypto.getCiphers().includes('sm4-gcm')); |
There was a problem hiding this comment.
This assertion cannot observe the transition as written. crypto.getCiphers() is wrapped in cachedResult() in lib/internal/crypto/util.js, and the calls above populate that JavaScript cache before setFips(true). This call therefore returns the same pre-FIPS list.
Please either make the public cipher-list cache FIPS-state-aware, or remove the dynamic getCiphers() assertions and retain the direct getCipherInfo() / createCipheriv() checks for the in-process transition.
panva
commented
Aug 20, 2026
Blocked by #63411 |
I believe this is now superseded by a broader refactor of the cipher discovery in #65484 |
PickBas
commented
Aug 24, 2026
@panva Thank you for looking into this and for the feedback! |
crypto: support provider-only SM4 cipher modes
sm4-gcm,sm4-ccmandsm4-xtsexist only as fetchable provider algorithms in OpenSSL 3. There is no legacyEVP_CIPHERfor them, soEVP_get_cipherbyname()/EVP_get_cipherbynid()cannot resolve them andEVP_CIPHER_do_all_sorted(), which walks the legacy name table, never reports them.createCipheriv()threwUnknown cipher,getCipherInfo()returnedundefined, andgetCiphers()omitted them.Changes
Cipher::FromName()/Cipher::FromNid()fall back toEVP_CIPHER_fetch().Cipher::getNid()recovers the nid from the algorithm name, since a fetched cipher inherits its nid from the legacy implementation it does not have. Without this,getCipherInfo(name)reports no nid and cannot round-trip throughgetCipherInfo(nid).Cipher::ForEach()probes for the provider-only modes sogetCiphers()lists them.Cipheris a non-owning wrapper, so they are cached for the process lifetime. The cache key includes the library context's default property state, becausecrypto.setFips()changes it at runtime and a cipher fetched beforehand must not stay usable afterwards.Tests
test-crypto-sm4-aead.js- RFC 8998 A.1/A.2 known-answer vectors for GCM and CCM, XTS round-trip, tag tampering,getCipherInfoname/nid round-trip, case-insensitive lookup, and negative cases for unknown names and nids.test-crypto-sm4-fips.js- enabling FIPS at runtime must invalidate an already fetched SM4 cipher. Skipped unless a FIPS provider is available.Fixes: #64866