Use OpenSSL implementation of AES Key Wrap with Padding on Linux #117256
+213
−1
Conversation
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
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the OpenSSL AES Key Wrap with Padding (EVP_aes_##_wrap_pad) on Linux/Unix and introduces a corresponding managed implementation along with an Android-specific AES class split from the OpenSSL one.
- Native shims and exports for
EVP_aes_*_wrap_padwere added across the PAL headers, sources, shim, and entrypoints. - Managed wrappers (
EncryptKeyWrapPaddedCore,DecryptKeyWrapPaddedCore, andKeyWrap) were implemented inAesImplementation.OpenSsl.cs, and an Android-specific AES implementation file was introduced. - Project and interop files were updated to include the new native and managed methods.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pal_evp_cipher.h, pal_evp_cipher.c | Added declarations and definitions for CryptoNative_EvpAes*WrapPad |
| opensslshim.h | Registered EVP_aes_*_wrap_pad as a required OpenSSL symbol |
| entrypoints.c | Exported the new native wrap-pad functions for P/Invoke |
| AesImplementation.OpenSsl.cs | Added managed AES key-wrap-pad methods and key-wrap algorithm lookup |
| AesImplementation.Android.cs | Introduced Android‐specific AES implementation split |
| System.Security.Cryptography.csproj | Replaced OpenSSL AES implementation include with Android one |
| Interop.EVP.Cipher.cs | Imported new native wrap-pad entrypoints for Unix interop |
Comments suppressed due to low confidence (2)
src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj:1223
- The csproj entry for
AesImplementation.OpenSsl.cswas removed and replaced with only the Android implementation, causing the OpenSSL-based AES code to no longer be compiled. You should include bothAesImplementation.OpenSsl.csandAesImplementation.Android.cs, or add a condition so that each file is compiled on the appropriate platform.
Link="Common\System\Security\Cryptography\SP800108HmacCounterKdfImplementationManaged.cs" />
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/AesImplementation.OpenSsl.cs:42
- There are no unit tests covering the new
EncryptKeyWrapPaddedCoreandDecryptKeyWrapPaddedCoremethods. Consider adding tests in the existing AES or OpenSSL test suite to verify key-wrap padding behavior for 128, 192, and 256-bit keys.
protected override void EncryptKeyWrapPaddedCore(ReadOnlySpan<byte> source, Span<byte> destination)
vcsjones
reviewed
Jul 3, 2025
...s/System.Security.Cryptography/src/System/Security/Cryptography/AesImplementation.Android.cs
Show resolved
Hide resolved
...s/System.Security.Cryptography/src/System/Security/Cryptography/AesImplementation.OpenSsl.cs
Show resolved
Hide resolved
This was referenced Jul 3, 2025
|
I also want to point out the
It seems that 1.1.1 will check for this and not allow wrapping. https://github.com/openssl/openssl/blob/b372b1f76450acdfed1e2301a39810146e28b02c/crypto/evp/evp_enc.c#L160-L164 |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This was referenced Jul 3, 2025
Open
This also adds error checking when creating the context.
|
The native crashes on OpenSSL 1.1 are an interesting confluence of things.
Here is a pull request to your branch with my suggested changes. bartonjs#10 |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
I'll see if pulling in a newer main build happens to have a mono fix that makes the problem go away. Otherwise, probably need someone familiar with mono to take a look. |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@steveisok Would you happen to have any insight as to the mono-only crashes this PR is seeing? When it crashes, the new code isn't even involved. e.g. Expand for very big stackWhere nothing looks like either OpenSSL or the crypto shim. I haven't tried slapping SkipOnMono on the tests that would exercise this code. But I have tried over the last couple of weeks a few "time has passed, maybe this was a bug already fixed in main" merges from main, always yielding the same mysterious mono-only failures. |
Closed
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@bartonjs I will try again to figure this one out now that I have some vacation rest in me. |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
vcsjones
added
the
NO-MERGE
|
Symbolicated stack: |
|
While I can solidly reproduce this issue now, I don't really know how to proceed with it. It seems very much likely there is something mono specific going on here. Some notes. This does not reproduce with Steps to reproduce:
|
This reverts commit f6d9563.
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
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.
EVP_aes_###_wrap_pad is available starting from 1.0.1, so here we treat it as a Required import.
Android's copy of AesImplementation is split off (as a copy) since it doesn't really use OpenSSL, just piggy-backed off of that PAL.
Fixes #108332