Skip to content

Bugfixes for the SymCrypt provider#165

Merged
mamckee merged 13 commits into
scossl-1.9from
mamckee-provider-bugfixes
May 8, 2026
Merged

Bugfixes for the SymCrypt provider#165
mamckee merged 13 commits into
scossl-1.9from
mamckee-provider-bugfixes

Conversation

@mamckee
Copy link
Copy Markdown
Collaborator

@mamckee mamckee commented Apr 21, 2026

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.

  • Fixed an incorrect size comparison in AES when setting OSSL_CIPHER_PARAM_TLS_MAC_SIZE
  • Fixed an issue where p_scossl_dh_keymgmt_dup_key_ctx would allocate an incorrectly sized buffer when copying a context using a custom group and with no key set yet.
  • Added initialization state checks to HMAC to prevent usage of context with no key set, but still permit partially initialized contexts for Allow MAC context duplication for partially initialized contexts #156
  • Added check for size of x25519 keys passed by parameter to the provider
  • Added checks for outlen in RSA encrypt/decrypt. This was previously not checked in OpenSSL for RSA encrypt but was hardened in 3.3.7. These checks bring SymCrypt-OpenSSL in line with the default behavior.
  • Fixed check in RSA key match, where the private exponent was not compared correctly.

Thanks @vcsjones for discovering these!

@mamckee mamckee requested a review from Copilot April 21, 2026 21:59
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread ScosslCommon/src/scossl_mac.c
Comment thread ScosslCommon/src/scossl_mac.c
Comment thread SymCryptProvider/src/asymcipher/p_scossl_rsa_cipher.c
Comment thread SymCryptProvider/src/keymgmt/p_scossl_ecc_keymgmt.c
@mamckee mamckee requested a review from Copilot April 23, 2026 21:35
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread SymCryptProvider/src/asymcipher/p_scossl_rsa_cipher.c
Comment thread SymCryptProvider/src/keymgmt/p_scossl_ecc_keymgmt.c
@mamckee mamckee marked this pull request as ready for review April 23, 2026 23:22
Copy link
Copy Markdown
Contributor

@MS-megliu MS-megliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing issue in touched code: p_scossl_rsa_cipher_dupctx() shallow-copies the SCOSSL_RSA_CIPHER_CTX struct via OPENSSL_memdup, which includes the pbLabel pointer. Both the original and duplicate contexts will call OPENSSL_free(ctx->pbLabel) on cleanup — double-free / use-after-free. Since this file is already being touched for the outlen hardening, consider deep-copying pbLabel in dupctx.

Comment thread ScosslCommon/src/scossl_mac.c
@mamckee mamckee requested a review from MS-megliu May 7, 2026 18:59
Comment thread ScosslCommon/src/scossl_mac.c
MS-megliu
MS-megliu previously approved these changes May 7, 2026
Copy link
Copy Markdown
Contributor

@MS-megliu MS-megliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Contributor

@MS-megliu MS-megliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mamckee mamckee merged commit 0ac9346 into scossl-1.9 May 8, 2026
3 checks passed
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