Refactor prov-cipher to avoid using deprecated ENGINE/EVP_CIPHER_meth APIs#523
Conversation
… APIs This commit introduces a new cipher abstraction layer and removes dependencies on deprecated ENGINE-based interfaces. * Introduce `GOST_cipher_ctx` to replace `EVP_CIPHER_CTX` in cipher methods. The corresponding functions mirror the behavior of the `EVP_CIPHER_CTX`-based APIs. High-level functions `GOST_CipherInit_ex`, `GOST_CipherUpdate`, and `GOST_CipherFinal` are modeled after their EVP counterparts. * Provide a separate `GOST_cipher_ctx` implementation for the engine target as a wrapper around `EVP_CIPHER_CTX`. * Introduce the `GOST_cipher` interface to replace `EVP_CIPHER`. Its fields are not intended to be accessed directly. * Remove ENGINE initialization from `gost_prov` initialization. * Add patches for OpenSSL 3.6 to improve test coverage: fix `ASN1_item_verify_ctx` and `x509_sig_info_init` to work correctly with provided digests. Without these patches, CA functionality is not available when using OpenSSL 3.6. Additional fixes: * Fix DER-encoded ASN.1 cipher parameter handling in `gost_prov_cipher`. * Fix error propagation in `magma_get_asn1_parameters()`: return an error if `gost2015_get_asn1_params()` fails.
There was a problem hiding this comment.
Pull request overview
This PR refactors the cipher implementation to remove provider-side dependencies on deprecated ENGINE/EVP cipher method APIs by introducing an internal GOST_cipher descriptor plus a provider-native GOST_cipher_ctx, while keeping an ENGINE-target wrapper path for legacy integrations. It also adds OpenSSL 3.6 patch files intended to restore CA-related behavior when using provided digests.
Changes:
- Introduce
GOST_cipherandGOST_cipher_ctxabstractions and refactor cipher implementations to use them (provider-native, with an EVP-wrapping implementation for the engine build). - Update the provider cipher implementation to use
GOST_CipherInit_ex/GOST_CipherUpdate/GOST_CipherFinaland remove ENGINE initialization from provider startup. - Add CI scripts and patch files for OpenSSL 3.6 to improve x509/ASN.1 behavior with fetched digests, plus expand cipher tests around provider parameters.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test_ciphers.c | Adds provider-specific regression checks for cipher params and padded decrypt behavior. |
| patches/openssl-x509_sig_info_init.patch | Adds digest fetching fallback in OpenSSL x509 signature info init (patch file). |
| patches/openssl-asn1_item_verify_ctx.patch | Adds digest fetching fallback in OpenSSL ASN.1 verify ctx path (patch file). |
| gost_prov.h | Removes ENGINE dependency from provider context header and updates includes. |
| gost_prov.c | Removes ENGINE creation/teardown from provider initialization and teardown. |
| gost_prov_cipher.c | Refactors provider cipher plumbing to use GOST_cipher + GOST_cipher_ctx instead of EVP/ENGINE. |
| gost_lcl.h | Moves GOST_cipher definition out (now via new headers) and updates includes. |
| gost_keyexpimp.c | Updates cipher wrapper init/do/ctrl signatures to use GOST_cipher_ctx. |
| gost_grasshopper_cipher.h | Updates grasshopper cipher function signatures to use GOST_cipher_ctx. |
| gost_grasshopper_cipher.c | Refactors grasshopper cipher implementation to use GOST_cipher_ctx accessors. |
| gost_gost2015.h | Updates gost2015_final_call signature to accept GOST_cipher_ctx. |
| gost_gost2015.c | Implements updated gost2015_final_call signature and uses GOST_cipher_ctx_encrypting. |
| gost_eng.c | Switches engine cipher registry to GOST_eng_cipher wrapper instances. |
| gost_eng_cipher.h | Introduces engine-only wrapper API for exposing GOST_cipher as EVP_CIPHER. |
| gost_eng_cipher.c | Implements engine-only EVP_CIPHER wrappers backed by GOST_cipher descriptors. |
| gost_crypt.c | Refactors gost/magma cipher implementations to use GOST_cipher_ctx accessors. |
| gost_cipher.h | Adds public(ish) accessor API for treating GOST_cipher as opaque. |
| gost_cipher.c | Implements GOST_cipher accessors and template field resolution. |
| gost_cipher_details.h | Defines the internal struct gost_cipher_st layout (opaque to external users). |
| gost_cipher_ctx.h | Declares provider-native cipher context API and accessors. |
| gost_cipher_ctx.c | Implements provider-native GOST_cipher_ctx (EVP-like behavior without EVP types). |
| gost_cipher_ctx_evp.c | Implements engine-target GOST_cipher_ctx as a thin wrapper over EVP_CIPHER_CTX. |
| CMakeLists.txt | Adjusts build composition: provider no longer links engine sources; adds new cipher/ctx sources. |
| .github/workflows/windows.yml | Applies new OpenSSL patch files in Windows CI. |
| .github/before_script.sh | Applies new OpenSSL patch files in CI bootstrap script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ((p = OSSL_PARAM_locate(params, "alg_id_param")) != NULL) { | ||
| ASN1_TYPE *algidparam = NULL; | ||
| unsigned char *der = NULL; | ||
| int derlen = 0; | ||
| int ret; | ||
|
|
||
| ret = (algidparam = ASN1_TYPE_new()) != NULL | ||
| && EVP_CIPHER_param_to_asn1(gctx->cctx, algidparam) > 0 | ||
| && (GOST_cipher_set_asn1_parameters_fn(gctx->cipher) == NULL | ||
| || GOST_cipher_set_asn1_parameters_fn(gctx->cipher)(gctx->cctx, | ||
| algidparam) > 0) | ||
| && (derlen = i2d_ASN1_TYPE(algidparam, &der)) >= 0 | ||
| && OSSL_PARAM_set_octet_string(p, &der, (size_t)derlen); | ||
| && OSSL_PARAM_set_octet_string(p, der, (size_t)derlen); | ||
|
|
||
| OPENSSL_free(der); | ||
| ASN1_TYPE_free(algidparam); |
There was a problem hiding this comment.
i2d_ASN1_TYPE(algidparam, &der) advances der to the end of the encoded buffer when allocating. The current code passes this advanced pointer to OSSL_PARAM_set_octet_string() and then frees it, which can both return incorrect bytes and call OPENSSL_free() on a non-base pointer. Preserve the original allocated pointer (e.g., unsigned char *der = NULL, *derp = NULL; derp = der; i2d(...,&derp);) and free the base pointer after setting the param.
There was a problem hiding this comment.
I don't think this is an issue.
According to the OpenSSL documentation:
https://docs.openssl.org/3.1/man3/d2i_X509/#description
If *ppout is NULL, memory will be allocated for a buffer and the encoded data written to it. In this case *ppout is not incremented and it points to the start of the data just written.
In our case, der is initialized to NULL and we pass &der to
i2d_ASN1_TYPE(). Therefore, the function allocates a new buffer and
assigns its base address to der without advancing the pointer.
As a result, der always points to the beginning of the allocated
buffer, and it is safe to pass it to OSSL_PARAM_set_octet_string() and
later free it with OPENSSL_free().
|
|
||
| if (in->cipher_data && GOST_cipher_ctx_size(in->cipher)) { | ||
| out->cipher_data = OPENSSL_malloc(GOST_cipher_ctx_size(in->cipher)); | ||
| if (out->cipher_data == NULL) { |
There was a problem hiding this comment.
GOST_cipher_ctx_copy() does memcpy(out, in, ...) before allocating a new out->cipher_data. If that allocation fails, it sets out->cipher = NULL but leaves out->cipher_data still pointing at in->cipher_data (copied by the memcpy). When the destination context is later freed/reset, it will OPENSSL_free(out->cipher_data), potentially freeing the source context’s memory (double-free / UAF). Ensure out->cipher_data is set to NULL (or restored) on allocation failure before returning, and avoid leaving out in a state where it owns pointers copied from in.
| if (out->cipher_data == NULL) { | |
| if (out->cipher_data == NULL) { | |
| out->cipher_data = NULL; |
There was a problem hiding this comment.
This looks like a mistaken suggestion from Copilot. The proposed change effectively reduces to something like:
if (x == NULL) {
x = NULL;
}
which does not change the state and therefore does not address the reported issue.
The current behavior is consistent with the original implementation of EVP_CIPHER_CTX_copy():
https://github.com/openssl/openssl/blob/openssl-3.6.0/crypto/evp/evp_enc.c#L1827
Our implementation follows the same pattern, including the handling of allocation failures.
|
@chipitsine, I'm sorry for responding to Copilot's comments separately rather than in a single review. It looks like Copilot is mostly mistaken this time. |
This PR introduces a new cipher abstraction layer and removes dependencies on deprecated ENGINE-based interfaces.
Introduce
GOST_cipher_ctxto replaceEVP_CIPHER_CTXin cipher methods. The corresponding functions mirror the behavior of theEVP_CIPHER_CTX-based APIs. High-level functionsGOST_CipherInit_ex,GOST_CipherUpdate, andGOST_CipherFinalare modeled after their EVP counterparts.Provide a separate
GOST_cipher_ctximplementation for the engine target as a wrapper aroundEVP_CIPHER_CTX.Introduce the
GOST_cipherinterface to replaceEVP_CIPHER. Its fields are not intended to be accessed directly.Remove ENGINE initialization from
gost_provinitialization.Add patches for OpenSSL 3.6 to improve test coverage: fix
ASN1_item_verify_ctxandx509_sig_info_initto work correctly with provided digests. Without these patches, CA functionality is not available when using OpenSSL 3.6.Additional fixes:
Fix DER-encoded ASN.1 cipher parameter handling in
gost_prov_cipher.Fix error propagation in
magma_get_asn1_parameters(): return an error ifgost2015_get_asn1_params()fails.This PR addresses several issues: resolves #501, resolves #504, resolves #506.