Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cbModulus = SymCryptRsakeySizeofModulus(ctx->keyCtx->key); | ||
| if (out == NULL) | ||
| { | ||
| if (cbModulus == 0) | ||
| { | ||
| ERR_raise(ERR_LIB_PROV, PROV_R_INVALID_KEY); | ||
| return SCOSSL_FAILURE; | ||
| } | ||
|
|
||
| *outlen = cbModulus; | ||
| return SCOSSL_SUCCESS; | ||
| } | ||
|
|
||
| if (outsize < cbModulus) | ||
| { | ||
| ERR_raise(ERR_LIB_PROV, PROV_R_OUTPUT_BUFFER_TOO_SMALL); | ||
| return SCOSSL_FAILURE; | ||
| } |
There was a problem hiding this comment.
In the non-TLS decrypt path, cbModulus is used for output sizing but is only validated for the out == NULL (size query) case. If SymCryptRsakeySizeofModulus returns 0 for an invalid key and out != NULL, the outsize check is bypassed and the code proceeds to scossl_rsa_decrypt without raising PROV_R_INVALID_KEY. Consider checking cbModulus == 0 immediately after retrieval (as encrypt does) and returning PROV_R_INVALID_KEY consistently.
There was a problem hiding this comment.
So this is technically the OpenSSL behavior. I think raising the error outside of the out == NULL check is better though. I suppose this will still fail later in the call so moving this check should be safe.
This PR fixes a number of minor bugs and behavior issues with the SymCrypt provider. These bugs are either normally not triggerable through the EVP layer or normal calling patterns, or impact uncommonly used functions.
OSSL_CIPHER_PARAM_TLS_MAC_SIZEp_scossl_dh_keymgmt_dup_key_ctxwould allocate an incorrectly sized buffer when copying a context using a custom group and with no key set yet.