Skip to content

Refactor raw public key handling and legacy EVP_PKEY methods - #669

Open
olszomal wants to merge 1 commit into
OpenSC:masterfrom
olszomal:ops_refactor
Open

Refactor raw public key handling and legacy EVP_PKEY methods#669
olszomal wants to merge 1 commit into
OpenSC:masterfrom
olszomal:ops_refactor

Conversation

@olszomal

@olszomal olszomal commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

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.

Comment thread src/p11_ecx.c
if (EVP_PKEY_get_raw_public_key(peerkey, NULL,
&peer_public_len) != 1 ||
peer_public_len != required ||
peer_public_len > sizeof(peer_public))
@mtrojnar

mtrojnar commented Aug 6, 2026

Copy link
Copy Markdown
Member

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:340EVP_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).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants