Harden parameter checks in provider functions#168
Draft
Conversation
Mirror NULL-parameter checks performed by the OpenSSL default provider in providers/implementations/digests/. - p_scossl_shake_set_ctx_params: check ctx and params for NULL - p_scossl_cshake_set_ctx_params: check ctx and params for NULL - p_scossl_cshake_settable_ctx_params: tolerate NULL ctx - p_scossl_digest_get_state_internal: check ctx and params for NULL - p_scossl_digest_set_state_internal: check ctx and params for NULL Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirror NULL-parameter checks performed by the OpenSSL default provider in providers/implementations/macs/. - p_scossl_hmac_set_ctx_params: check ctx and params for NULL - p_scossl_cmac_set_ctx_params: check ctx and params for NULL - p_scossl_kmac_set_ctx_params: check ctx and params for NULL Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirror NULL-parameter checks performed by the OpenSSL default provider in providers/implementations/kdfs/. - p_scossl_hkdf_set_ctx_params: check ctx and params for NULL - p_scossl_tls13kdf_set_ctx_params: check ctx and params for NULL - p_scossl_kbkdf_set_ctx_params: check ctx and params for NULL - p_scossl_pbkdf2_set_ctx_params: check ctx and params for NULL - p_scossl_srtpkdf_set_ctx_params: check ctx and params for NULL - p_scossl_sshkdf_set_ctx_params: check ctx and params for NULL - p_scossl_sskdf_set_ctx_params: check ctx and params for NULL - p_scossl_tls1prf_set_ctx_params: check ctx and params for NULL Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirror NULL-parameter checks performed by the OpenSSL default provider in providers/implementations/ciphers/. - p_scossl_aes_generic_set_ctx_params: check ctx and params for NULL - p_scossl_aes_gcm_set_ctx_params: check ctx and params for NULL - p_scossl_aes_ccm_set_ctx_params: check ctx and params for NULL - p_scossl_aes_xts_set_ctx_params: check ctx and params for NULL Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirror NULL-parameter checks performed by the OpenSSL default provider in providers/implementations/kem/. - p_scossl_mlkem_set_ctx_params: check params for NULL Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirror NULL-parameter checks performed by the OpenSSL default provider in providers/implementations/exchange/. - p_scossl_dh_set_ctx_params: check ctx and params for NULL - p_scossl_kdf_keyexch_set_ctx_params: check ctx and params for NULL Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirror NULL-parameter checks performed by the OpenSSL default provider in providers/implementations/asymciphers/. - p_scossl_rsa_cipher_get_ctx_params: check ctx for NULL - p_scossl_rsa_cipher_set_ctx_params: check ctx and params for NULL Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirror NULL-parameter checks performed by the OpenSSL default provider in providers/implementations/signature/. - p_scossl_rsa_set_ctx_params: check ctx and params for NULL - p_scossl_rsa_get_ctx_params: check ctx for NULL - p_scossl_ecdsa_set_ctx_params: check ctx and params for NULL - p_scossl_ecdsa_get_ctx_params: check ctx for NULL Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirror NULL-parameter checks performed by the OpenSSL default provider in providers/implementations/keymgmt/. - p_scossl_rsa_keygen_set_params: check genCtx and params for NULL - p_scossl_dh_keygen_set_params: check genCtx and params for NULL - p_scossl_dh_keymgmt_set_params: check ctx and params for NULL - p_scossl_ecc_keygen_set_params: check genCtx and params for NULL - p_scossl_ecc_keymgmt_set_params: check keyCtx and params for NULL - p_scossl_mlkem_keymgmt_set_params: check keyCtx and params for NULL - p_scossl_mlkem_hybrid_keymgmt_set_params: check keyCtx and params for NULL Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to harden SymCrypt provider entry points against unconventional EVP-layer usage by adding NULL-context checks, short-circuiting empty parameter arrays in many set-param paths, and cleaning up some compatibility/style code around the provider surface.
Changes:
- Added NULL
ctxchecks to many provider dup/get/set-param helpers across signatures, MACs, KDFs, ciphers, key exchange, KEM, digests, encoder, and key management code. - Introduced
p_scossl_is_params_empty()and used it in many set-param handlers to return early when no parameters are supplied. - Removed obsolete OpenSSL 3.0 compatibility declarations/definitions and did small include/style cleanups in affected files.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| SymCryptProvider/src/signature/p_scossl_rsa_signature.c | Hardened RSA signature ctx duplication and ctx/md param handling. |
| SymCryptProvider/src/signature/p_scossl_ecdsa_signature.c | Hardened ECDSA signature ctx duplication and ctx/md param handling. |
| SymCryptProvider/src/p_scossl_rand.c | Minor RAND ctx-param style cleanup and shared header include. |
| SymCryptProvider/src/p_scossl_ecc.c | Added NULL check in ECC key ctx duplication. |
| SymCryptProvider/src/p_scossl_base.c | Removed old OpenSSL 3.0 compatibility shims. |
| SymCryptProvider/src/mac/p_scossl_kmac.c | Added NULL/empty-param guards for KMAC ctx operations. |
| SymCryptProvider/src/mac/p_scossl_hmac.c | Added NULL/empty-param guards for HMAC ctx operations. |
| SymCryptProvider/src/mac/p_scossl_cmac.c | Added NULL/empty-param guards for CMAC ctx operations. |
| SymCryptProvider/src/keymgmt/p_scossl_rsa_keymgmt.c | Added extra NULL checks in RSA keymgmt duplication, keygen, get, has, and match paths. |
| SymCryptProvider/src/keymgmt/p_scossl_mlkem_keymgmt.c | Added NULL checks in ML-KEM keymgmt duplication and param accessors. |
| SymCryptProvider/src/keymgmt/p_scossl_mlkem_hybrid_keymgmt.c | Added NULL checks in hybrid ML-KEM keymgmt duplication and param accessors. |
| SymCryptProvider/src/keymgmt/p_scossl_ecc_keymgmt.c | Added NULL checks in ECC keygen/get/set/match helpers and reordered dependent locals. |
| SymCryptProvider/src/keymgmt/p_scossl_dh_keymgmt.c | Added NULL checks in DH key duplication, keygen, get/set, and match helpers. |
| SymCryptProvider/src/keyexch/p_scossl_kdf_keyexch.c | Added NULL/empty-param guards around wrapped KDF keyexch ctx helpers. |
| SymCryptProvider/src/keyexch/p_scossl_ecdh.c | Added NULL check in ECDH ctx duplication. |
| SymCryptProvider/src/keyexch/p_scossl_dh.c | Added NULL/empty-param guards in DH key exchange ctx helpers. |
| SymCryptProvider/src/kem/p_scossl_mlkem.c | Added NULL checks in ML-KEM KEM ctx duplication and set-param path. |
| SymCryptProvider/src/kem/p_scossl_mlkem_hybrid.c | Added NULL check in hybrid ML-KEM KEM ctx duplication. |
| SymCryptProvider/src/kdf/p_scossl_tls1prf.c | Added NULL/empty-param guards in TLS1 PRF ctx helpers. |
| SymCryptProvider/src/kdf/p_scossl_sskdf.c | Added NULL/empty-param guards in SSKDF ctx helpers plus whitespace cleanup. |
| SymCryptProvider/src/kdf/p_scossl_sshkdf.c | Added NULL/empty-param guards in SSHKDF ctx helpers plus formatting cleanup. |
| SymCryptProvider/src/kdf/p_scossl_srtpkdf.c | Added NULL/empty-param guards in SRTP KDF ctx helpers and shared header include. |
| SymCryptProvider/src/kdf/p_scossl_pbkdf2.c | Added NULL/empty-param guards in PBKDF2 ctx helpers. |
| SymCryptProvider/src/kdf/p_scossl_kbkdf.c | Added NULL/empty-param guards in KBKDF ctx helpers. |
| SymCryptProvider/src/kdf/p_scossl_hkdf.c | Added NULL/empty-param guards in HKDF/TLS13-KDF ctx helpers. |
| SymCryptProvider/src/encoder/p_scossl_encode_common.c | Added NULL/empty-param guards in encoder ctx param setter. |
| SymCryptProvider/src/digests/p_scossl_shake.c | Added NULL/empty-param guards for SHAKE ctx params. |
| SymCryptProvider/src/digests/p_scossl_digest_generic.c | Added NULL checks in generic digest state import/export helpers. |
| SymCryptProvider/src/digests/p_scossl_digest_common.c | Added digest dupctx NULL guard and empty-param fast path for generic get_params. |
| SymCryptProvider/src/digests/p_scossl_cshake.c | Added NULL/empty-param guards in cSHAKE ctx helpers and shared header include. |
| SymCryptProvider/src/ciphers/p_scossl_aes.c | Added NULL/empty-param guards in generic AES ctx helpers. |
| SymCryptProvider/src/ciphers/p_scossl_aes_xts.c | Added NULL guards in AES-XTS dup/init/get/set helpers and shared header include. |
| SymCryptProvider/src/ciphers/p_scossl_aes_aead.c | Added NULL/empty-param guards in AES-GCM/CCM ctx helpers and minor style cleanup. |
| SymCryptProvider/src/asymcipher/p_scossl_rsa_cipher.c | Added NULL/empty-param guards in RSA asymcipher ctx param helpers. |
| SymCryptProvider/inc/p_scossl_base.h.in | Added shared empty-params helper used by provider ctx param code. |
| ScosslCommon/src/scossl_mac.c | Added NULL guard in common MAC ctx duplication. |
| ScosslCommon/src/scossl_aes_aead.c | Minor style-only cleanup in common AES AEAD helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+534
to
542
| if ((p = OSSL_PARAM_locate(params, OSSL_CIPHER_PARAM_UPDATED_IV)) != NULL) | ||
| { | ||
| if (p->data_size < ctx->taglen) | ||
| { | ||
| ERR_raise(ERR_LIB_PROV, PROV_R_INVALID_TAG_LENGTH); | ||
| return SCOSSL_FAILURE; | ||
| } | ||
|
|
||
| if (!OSSL_PARAM_set_octet_string(p, &ctx->tag, ctx->taglen)) |
Comment on lines
1090
to
+1094
| static SCOSSL_STATUS p_scossl_rsa_set_ctx_md_params(_In_ SCOSSL_RSA_SIGN_CTX *ctx, _In_ const OSSL_PARAM params[]) | ||
| { | ||
| if (ctx->mdctx == NULL) | ||
| { | ||
| ERR_raise(ERR_LIB_PROV, PROV_R_MISSING_MESSAGE_DIGEST); |
Comment on lines
839
to
+848
| static SCOSSL_STATUS p_scossl_rsa_get_ctx_params(_In_ SCOSSL_RSA_SIGN_CTX *ctx, _Inout_ OSSL_PARAM params[]) | ||
| { | ||
| if (params == NULL) | ||
| { | ||
| return SCOSSL_SUCCESS; | ||
| } | ||
|
|
||
| OSSL_PARAM *p; | ||
| ASN1_STRING *pval = NULL; | ||
| X509_ALGOR *x509Alg = NULL; | ||
| SCOSSL_STATUS ret = SCOSSL_FAILURE; | ||
|
|
||
| if (ctx == NULL) | ||
| { | ||
| ERR_raise(ERR_LIB_PROV, ERR_R_PASSED_NULL_PARAMETER); |
Comment on lines
423
to
+432
| static SCOSSL_STATUS p_scossl_ecdsa_get_ctx_params(_In_ SCOSSL_ECDSA_CTX *ctx, _Inout_ OSSL_PARAM params[]) | ||
| { | ||
| if (params == NULL) | ||
| { | ||
| return SCOSSL_SUCCESS; | ||
| } | ||
|
|
||
| OSSL_PARAM *p; | ||
| X509_ALGOR *x509Alg = NULL; | ||
| SCOSSL_STATUS ret = SCOSSL_FAILURE; | ||
|
|
||
| if (ctx == NULL) | ||
| { | ||
| ERR_raise(ERR_LIB_PROV, ERR_R_PASSED_NULL_PARAMETER); | ||
| return SCOSSL_FAILURE; |
| ERR_raise(ERR_LIB_PROV, ERR_R_PASSED_NULL_PARAMETER); | ||
| return SCOSSL_FAILURE; | ||
| } | ||
|
|
Comment on lines
54
to
+60
| SCOSSL_STATUS success = SCOSSL_FAILURE; | ||
| SYMCRYPT_ECPOINT_FORMAT pointFormat = keyCtx->isX25519 ? SYMCRYPT_ECPOINT_FORMAT_X : SYMCRYPT_ECPOINT_FORMAT_XY; | ||
| SYMCRYPT_ERROR scError = SYMCRYPT_NO_ERROR; | ||
| SCOSSL_ECC_KEY_CTX *copyCtx; | ||
|
|
||
| SCOSSL_ECC_KEY_CTX *copyCtx = OPENSSL_zalloc(sizeof(SCOSSL_ECC_KEY_CTX)); | ||
| if (keyCtx == NULL) | ||
| return NULL; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Multiple places in the provider, especially ctx get/set param functions, implicitly rely on the context parameter supplied by the EVP layer being non-NULL. The default provider is inconsistent with these checks, but some interfaces do check whether the context is NULL.
This PR addresses the behavior gap with the default provider and hardens additional functions to check whether the supplied context is NULL. This makes the SymCrypt provider more resilient towards applications that call the OpenSSL APIs in unconventional ways.
This PR also adds a check in set context param functions to exit early if the
paramsarray is NULL. This is common for context initialization, where set context param functions are always called, but frequently don't pass any params.ctxparameter being null in dup ctx and get/set ctx param functions for consistency and resiliency.