Refactor raw public key handling and legacy EVP_PKEY methods

[olszomal]

Pull Request Type

• Bug fix
• New feature
• Code style / formatting / renaming
• Refactoring (no functional or API changes)
• Build / CI related changes
• Documentation
• Other (please describe):

Related Issue

Issue number: N/A

Scope of Changes

This PR consolidates raw public key handling into a shared module and reorganizes legacy OpenSSL EVP_PKEY_METHOD wrappers.

It removes duplicated EdDSA, XDH and PQC key code, centralizes RSA and ECX method registration, reuses the existing EVP_PKEY ex-data association, and updates build files and tests for OpenSSL 3.x compatibility.

Testing

• Existing tests
• New tests added
• Manual testing

Additional Notes

License Declaration

• I hereby agree to license my contribution under the project's license.

[mtrojnar]

Reviewed at 55c9aaf against master (8f8f9c4). The consolidation looks good and the SoftHSM suite is green on OpenSSL 3.6.3 (48 pass / 2 skip, identical to master), but I found two blockers that only show up outside the 3.x path.

1. Global RSA EVP_PKEY_METHOD wrapper is registered where it can never be removed (crash)

src/libp11-int.h:623-629 widens the guard from >= 3.0 && < 4.0 to < 4.0, so pkcs11_rsa_method_enable() (src/p11_rsa.c:250-254) now installs the global wrapper on OpenSSL 1.0.2/1.1.0 and LibreSSL as well. On those versions pkcs11_rsa_pkey_method_free() is an intentional no-op (src/p11_pkey.c:2009-2027), so after the last PKCS11_CTX_free():

• the wrapper stays in OpenSSL's app method stack, but
• pkcs11_rsa_method_free() has already called free_rsa_ex_index(),

so pkcs11_get_ex_data_rsa() inside the still-registered pkcs11_try_pkey_rsa_sign()/pkcs11_try_pkey_rsa_decrypt() reads ex_data index 0 (i.e. RSA_get_app_data()) of foreign RSA keys and dereferences it as PKCS11_OBJECT_private.

Reproducer through the public API only (SoftHSM token with one RSA key, built against OpenSSL 1.1.0):

/* ... PKCS11_CTX_load, enumerate, PKCS11_login ... */
p11key = PKCS11_get_private_key(&keys[i]);   /* registers the global wrapper */
EVP_PKEY_free(p11key);
PKCS11_release_all_slots(ctx, slots, nslots);
PKCS11_CTX_unload(ctx);
PKCS11_CTX_free(ctx);                        /* libp11_global_free(): frees rsa_ex_index,
                                                cannot unregister the pkey method */

rsa = RSA_new(); RSA_generate_key_ex(rsa, 1024, e, NULL);
RSA_set_app_data(rsa, (void *)0x1);          /* index 0 == app_data now */
/* plain software RSA signature */
EVP_PKEY_sign(pctx, sig, &siglen, tbs, sizeof(tbs));

Result: master exits 0, this branch SIGSEGVs. The same program on OpenSSL 3.6.3 exits 0 on both branches (there EVP_PKEY_meth_remove() works). The same stale pointer is also a dangling call target if libp11 gets dlclose()d (engine/provider unload) — note the atexit(pkcs11_rsa_key_method_free) safety net that master had was dropped here.

Suggestion: only register when the method can be unregistered (keep >= 0x10101000L, LibreSSL excluded), or don't release the RSA ex_data index when removal was not possible.

2. LibreSSL / OpenSSL < 1.0.2d no longer compiles

EVP_PKEY_meth_get_sign()/EVP_PKEY_meth_get_decrypt() moved to src/p11_pkey.c:1934-1959, but the private struct evp_pkey_method_st they dereference stayed behind in src/p11_key.c:62-101. Additionally EVP_PKEY_meth_remove() (src/p11_pkey.c:2015) is guarded by OPENSSL_VERSION_NUMBER >= 0x10101000L, which LibreSSL satisfies (0x20000000L) but does not implement.

With LibreSSL 3.7.3 headers, p11_pkey.c compiles on master and fails here:

p11_pkey.c:1942: error: invalid use of incomplete typedef 'EVP_PKEY_METHOD'
p11_pkey.c:2015: error: implicit declaration of function 'EVP_PKEY_meth_remove'

Suggestion: move the compat struct/block together with its users and add && !defined(LIBRESSL_VERSION_NUMBER) to the EVP_PKEY_meth_remove() guard.

Minor

• src/p11_ecx.c:340 — EVP_PKEY_meth_remove() return value ignored before EVP_PKEY_meth_free(), unlike the new RSA path (src/p11_pkey.c:2015); double-free if the method was already popped (e.g. app-invoked OPENSSL_cleanup() before the last PKCS11_CTX_free()). Worth making both consistent.
• src/p11_ec.c:410-419 — the new EVP_PKEY_set1_EC_KEY() failure path relies on pkcs11_ec_finish() to drop the extra pkcs11_object_ref(), but that hook is only installed for OpenSSL >= 1.1.0, so on 1.0.2/LibreSSL this OOM path leaks a key-object reference.
• src/p11_ecx.c:230 — the CodeQL warning is valid: peer_public_len > sizeof(peer_public) is dead after the != required check.
• src/p11_pkey.c:1962 — the "Attempt to sign using the PKCS#11-backed RSA implementation" comment now describes the method constructor; src/p11_pkey.c:1929 has a stray double blank line.

Nice side effect worth mentioning in the PR description

Master's pkcs11_x25519_method_new()/pkcs11_x448_method_new() required EVP_PKEY_FLAG_SIGCTX_CUSTOM on the original XDH method (src/p11_eddsa.c:437,476), which OpenSSL never sets — so X25519/X448 private keys always failed to load. p11_ecx.c applies that check to EdDSA only, so those keys now load. This is a behavioural change, not just a refactor. I could not exercise the derive path (SoftHSM has no EC-MONTGOMERY-KEY-PAIR-GEN), which matches the TODO in pkcs11_evp_pkey_xdh_derive().

What I ran

Environment	Result
OpenSSL 3.6.3, --enable-strict, make check	48 pass / 2 skip — same as master
OpenSSL 1.1.0, build + rsa-evp-sign/rsa-pss-sign/rsa-oaep/rsa-keygen	pass on both branches
OpenSSL 1.1.0, lifetime reproducer above	master 0, PR SIGSEGV
LibreSSL 3.7.3, compile p11_pkey.c	master OK, PR fails
OpenSSL 3.6.3, direct libp11 Ed25519 EVP_DigestSign/Verify vs SoftHSM	pass on both (new p11_rawkey.c path)
./testall.sh	stops at OpenSSL 1.0.2 — pre-existing on master too (OPENSSL_zalloc, X509_SIG_getm, OPENSSL_clear_free)

Not covered: MSVC/Windows (Makefile.mak updated but not built), valgrind, ML-DSA/SLH-DSA/Falcon raw-key paths (no SoftHSM support), and thread-safety of the new global ecx_methods[]/rsa_pkey_method state (same unlocked pattern as master).

[olszomal]

I’ve pushed another version of the refactoring with the changes discussed above. Could you please take another look and review it?

[mtrojnar]

Maintainer review — PR head 0488c31 vs. base e72a201. Tested on OpenSSL 3.5.7 (--enable-strict && make && make check — both branches green), cross-checked against OpenSSL 1.0.2/1.1.x engine sources, OASIS PKCS#11 v3.2 and RFC 7748.

Thanks for the rework — it addresses both blockers from my review at 55c9aaf, and it fixes a real usability bug: X25519/X448 keys can now actually be attached to the ENGINE. Before I merge, two issues remain in the new unified method cache in src/p11_pkey.c, both empirically reproduced at this head.

Previous blockers — resolved ✅

1. Global RSA EVP_PKEY_METHOD registered where it can never be removed (the OpenSSL 1.1.0 SIGSEGV reproducer): fixed — no EVP_PKEY_meth_add0/EVP_PKEY_meth_remove remains; everything now goes through the ENGINE PKCS11_pkey_meths callback. I checked the 1.0.2/1.1.x sources: int_ctx_new() consults pkey->engine → ENGINE_get_pkey_meth(), so the path is functional on those versions, not just
   crash-free.
2. OpenSSL < 1.0.2d compile failure: fixed — the struct evp_pkey_method_st compat block and the EVP_PKEY_meth_get_sign/get_decrypt shims moved with their users.

Finding 1 — must fix (new): unsynchronized cache construction publishes partially-built methods

pkcs11_pkey_method() (src/p11_pkey.c:2122-2211) publishes state->method into the process-global pkey_methods[] cache immediately after EVP_PKEY_meth_new() and before EVP_PKEY_meth_copy() and EVP_PKEY_meth_set_sign() complete:

if (state->method != NULL)                                                                                                                                                                                                                                                                                                                                                                                                       
    return state->method;                                                                                                                                                                                                                                                                                                                                                                                                        
...                                                                                                                                                                                                                                                                                                                                                                                                                              
state->method = EVP_PKEY_meth_new(state->type, original_flags); /* published NOW */                                                                                                                                                                                                                                                                                                                                              
EVP_PKEY_meth_copy(state->method, original_meth);               /* filled LATER */                                                                                                                                                                                                                                                                                                                                               
... EVP_PKEY_meth_set_sign(...) ...                                                                                                                                                                                                                                                                                                                                                                                              

PKCS11_pkey_meths() (line 2258) is called by OpenSSL from EVP_PKEY_CTX_new() with no locking, so concurrent ctx creation in a multithreaded app can observe the method mid-construction.

Reproduced (head 0488c31, OpenSSL 3.5.7):

• With an LD_PRELOAD shim delaying EVP_PKEY_meth_copy by 200 ms, 7 of 16 racing threads got the same method pointer with sign == NULL — a method OpenSSL will happily use for signing.
• A 64-thread run produced multiple distinct method pointers across callers; the cache overwrite leaks the loser while earlier callers keep the old pointer.
• The error: path (EVP_PKEY_meth_free(state->method) + pkey_method_reset()) can additionally free a method another thread already handed out.

This is new — base's builders assign a fully-constructed local only after returning, so base's window is a harmless duplicate-build/leak, never a half-built method.

Please fix: build into a local and publish once under a lock — e.g. CRYPTO_THREAD_run_once() per type, or a short critical section around check/build/publish; on failure, free the local only if it was never published.

Finding 2 — must fix (pre-existing, but this PR extends it): OpenSSL frees the cached method at ENGINE destruction → use-after-free/double-free

OpenSSL teardown, verified identical in crypto/engine/eng_lib.c + tb_pkmeth.c of openssl-3.5, OpenSSL_1_1_1-stable and OpenSSL_1_0_2-stable:

ENGINE_free → engine_free_util → engine_pkey_meths_free(e)                                                                                                                                                                                                                                                                                                                                                                       
  → for each nid: e->pkey_meths(e, &pkm, NULL, nid) → EVP_PKEY_meth_free(pkm)                                                                                                                                                                                                                                                                                                                                                    

There is no copy and no refcount: ENGINE_get_pkey_meth() returns the callback's raw pointer, and engine_pkey_meths_free() frees whatever the callback returns at destruction time. EVP_PKEY_meth_new() sets EVP_PKEY_FLAG_DYNAMIC, so the free is real. Your callback ignores the engine ((void)e) and returns the global cached pointer — so destroying any libp11 ENGINE frees the globally cached method while
pkey_methods[] still points at it. The next engine lifecycle hands out the freed pointer.

Reproduced (standalone reproducer of the exact libp11 pattern, OpenSSL 3.5.7):

• After ENGINE_free, the next EVP_PKEY_meth_new returned the cached method's address (glibc tcache LIFO) → the cached method was freed.
• The next cycle handed out the same dangling pointer; reads through it aliased later allocations (pkey_id read back 0x41424344 from a probe).
• The following ENGINE_free re-frees the same address — a real double-free (glibc's tcache next-pointer often clobbers flags first and silently skips the DYNAMIC check; the live-ctx use-after-free is the deterministic hazard).

Reachability: engine_pkcs11 is normally ENGINE_add()-ed, so the structural refcount hits zero on ENGINE_cleanup()/unload-reload cycles, or whenever a caller uses the ENGINE_new-based API and frees the handle. Any still-live EVP_PKEY_CTX then uses freed memory, and the next load cycle reuses it.

Base has the same static-cache pattern for RSA/EC/Ed25519/Ed448 (same-pointer-per-cycle behavior reproduced there too), so this is pre-existing — but this PR is exactly the refactor that consolidates this path and adds X25519/X448 to it, so let's fix it here rather than carrying it forward.

Please fix: scope the cache per ENGINE via ENGINE_set_ex_data (readable during engine_pkey_meths_free, which runs before ex_data is freed), built under the same lock as Finding 1. The engine's destruction then frees exactly its own method and the cache dies with the engine. A process-global cache is only safe if libp11 retains ownership forever (deliberate leak), which contradicts OpenSSL's free
semantics.

Finding 3 — recommended (pre-existing code, newly activated): XDH derive has no all-zero shared-secret check

pkcs11_evp_pkey_xdh_derive() (src/p11_pkey.c:1527) is byte-identical to base (p11_pkey.c:1365): it forwards the raw RFC 7748 peer key to C_DeriveKey with CKM_ECDH1_DERIVE + CKD_NULL and returns whatever the token produces — no low-order point validation and no all-zero result check. The new peer type/length validation in pkcs11_xdh_pmeth_derive() (line 1949) is good, but it doesn't close the result
check.

• OpenSSL's own X25519/X448 rejects all-zero outputs — verified empirically on 3.5.7 (EVP_PKEY_derive with an all-zero peer fails).
• OASIS PKCS#11 v3.2 does not require tokens to reject low-order points for CKM_ECDH1_DERIVE with Montgomery keys, and RFC 7748 allows but does not require aborting on all-zero output — so this is a hardening divergence, not a spec violation.
• The PR makes this path live for the first time: on base, X25519/X448 private keys could never be attached to the ENGINE at all (base required EVP_PKEY_FLAG_SIGCTX_CUSTOM on XDH methods, which OpenSSL never sets — verified: base EVP_PKEY_set1_engine fails, PR succeeds). A non-conforming token would now silently yield all-zero secrets where OpenSSL software would have errored.

Please consider: after a successful derive, constant-time-compare the result against all-zero and fail the operation if it matches, mirroring OpenSSL; document the assumption that tokens perform RFC 7748 checks.

What I liked

• X25519/X448 ENGINE support fixed; software-key fallback harness passes on the PR ("ed ok / x ok") and fails on base ("x set engine fail").
• p11_rawkey.c is clean; strip_der_octet_string_alloc() correctly tolerates both DER-wrapped and raw CKA_EC_POINT, with properly scoped error marks.
• The new tests/ed25519-software-key regression test (with .softhsm config and strict version guards) is a worthwhile net for this refactor.
• diff --check/CodeQL/secret hygiene clean; the earlier CodeQL dead-comparison note is fixed by the current head's single != required check.

Action items

1. Must fix: serialize construction and publication in pkcs11_pkey_method() (Finding 1).
2. Must fix: make the method cache per-ENGINE via ENGINE_set_ex_data so OpenSSL's engine_pkey_meths_free frees only that engine's method (Finding 2).
3. Recommended: add the all-zero shared-secret check to pkcs11_evp_pkey_xdh_derive() before the XDH path ships enabled (Finding 3).
4. Optional: fold the error:-path pkey_method_reset() into the locking fix, and document the shared-cache contract in PKCS11_pkey_meths().

Happy to share the reproducers: standalone ENGINE cache-lifetime test, 64-thread race test, 200 ms EVP_PKEY_meth_copy delay shim, X25519 all-zero-peer test, Ed25519/X25519 fallback harness.