Fix up Aes CCM/GCM support #48728
Merged
Fix up Aes CCM/GCM support #48728
Conversation
This file contains 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
Fixes dotnet#48471 This gets 49 more tests passing (split between Ccm and Gcm tests and sadly no full test suites).
…pl has a lot of special cases that we don't need to handle (and handling them makes the code significantly harder to read).
…he shim. This fixes cases where the ciphertext returned from doFinal is either part or all of the tag (and no ciphertext).
jkoritzinsky
requested review from
steveisok,
AaronRobinsonMSFT and
elinor-fung
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks Issue DetailsFixes #48471 This gets all the tests passing in the AES/CCM and AES/GCM test suites. This PR splits the managed implementation for AES/CCM and AES/GCM from OpenSSL for Android since the authentication tag handling required for Android is different enough from OpenSSL to make the code to correctly handle it with the OpenSSL shim surface area extremely confusing.
|
jkoritzinsky
changed the title
...s/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/AesGcm.Android.cs
Show resolved
Hide resolved
...s/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/AesGcm.Android.cs
Outdated
Show resolved
Hide resolved
...ries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/AesCcm.Unix.cs
Outdated
Show resolved
Hide resolved
vcsjones
reviewed
Feb 25, 2021
...s/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/AesCcm.Android.cs
Outdated
Show resolved
Hide resolved
...s/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/AesGcm.Android.cs
Outdated
Show resolved
Hide resolved
bartonjs
reviewed
Feb 25, 2021
...Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.EVP.Cipher.cs
Outdated
Show resolved
Hide resolved
bartonjs
reviewed
Feb 25, 2021
...Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.EVP.Cipher.cs
Outdated
Show resolved
Hide resolved
bartonjs
reviewed
Feb 25, 2021
src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_evp_cipher.c
Outdated
Show resolved
Hide resolved
bartonjs
reviewed
Feb 25, 2021
...s/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/AesCcm.Android.cs
Outdated
Show resolved
Hide resolved
…hicExceptions. Check for overflow.
src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_cipher.c
Outdated
Show resolved
Hide resolved
src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_cipher.c
Outdated
Show resolved
Hide resolved
| if(tagLength > TAG_MAX_LENGTH) | ||
| return FAIL; | ||
|
|
||
| ctx->tagLength = tagLength; |
There was a problem hiding this comment.
Does this permit resetting? Perhaps there is some invariant we should assert prior.
There was a problem hiding this comment.
Yes, this can be reset when the cipher is reused.
…nter-sized values.
AaronRobinsonMSFT
approved these changes
Feb 26, 2021
bartonjs
reviewed
Feb 26, 2021
src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_cipher.c
Show resolved
Hide resolved
bartonjs
reviewed
Feb 26, 2021
| return ReinitializeCipher(ctx); | ||
| } | ||
|
|
||
| CipherCtx* AndroidCryptoNative_CipherCreate2(CipherInfo* type, uint8_t* key, int32_t keyLength, int32_t effectiveKeyLength, uint8_t* iv, int32_t enc) |
There was a problem hiding this comment.
"CipherCreate2", already? 😄. If our OpenSSL shim has CIpherCreate2 it's because we created it during complex SxS upgrade time and didn't want to change CipherCreate.
There was a problem hiding this comment.
I’ll remove the 2 suffix.
bartonjs
reviewed
Feb 26, 2021
...s/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/AesCcm.Android.cs
Outdated
Show resolved
Hide resolved
bartonjs
reviewed
Feb 26, 2021
...s/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/AesCcm.Android.cs
Outdated
Show resolved
Hide resolved
bartonjs
reviewed
Feb 26, 2021
...ies/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Cipher.cs
Outdated
Show resolved
Hide resolved
bartonjs
reviewed
Feb 26, 2021
...s/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/AesGcm.Android.cs
Outdated
Show resolved
Hide resolved
bartonjs
approved these changes
Feb 26, 2021
|
Hello @jkoritzinsky! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fixes #48471
This gets all the tests passing in the AES/CCM and AES/GCM test suites.
This PR splits the managed implementation for AES/CCM and AES/GCM from OpenSSL for Android since the authentication tag handling required for Android is different enough from OpenSSL to make the code to correctly handle it with the OpenSSL shim surface area extremely confusing.