Skip to content

Support ChaCha20 Poly1305 on Unix - #52522

Merged
bartonjs merged 8 commits into
dotnet:mainfrom
vcsjones:52482-chacha-unix
May 11, 2021
Merged

Support ChaCha20 Poly1305 on Unix#52522
bartonjs merged 8 commits into
dotnet:mainfrom
vcsjones:52482-chacha-unix

Conversation

@vcsjones

@vcsjones vcsjones commented May 9, 2021

Copy link
Copy Markdown
Member

Contributes to #52482

@ghost ghost added the area-System.Security label May 9, 2021
@ghost

ghost commented May 9, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #52482

Author: vcsjones
Assignees: -
Labels:

area-System.Security
Milestone: -

@vcsjones

vcsjones commented May 9, 2021

Copy link
Copy Markdown
Member Author

Draft for now, I suspect there will be compilation failures on older Linux.

am11
am11 reviewed May 9, 2021
View reviewed changes
Comment thread src/libraries/Native/Unix/System.Security.Cryptography.Native/opensslshim.h
am11
am11 reviewed May 9, 2021
View reviewed changes
Comment thread src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_evp_cipher.c Outdated
vcsjones and others added 3 commits May 9, 2021 17:09
@vcsjones
Maybe
81c9a4f
@vcsjones
Do not ask macOS for its OpenSSL version
c73c744
@vcsjones @am11
Simplify conditional.
fa37b9b 
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@vcsjones

vcsjones commented May 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

I'm guessing some of our macOS machines don't have OpenSSL 1.1.0 on them?

Looks like the 10.15 machines do, and <= 10.14 don't.

@vcsjones

vcsjones commented May 10, 2021

Copy link
Copy Markdown
Member Author

Note, I considered refactoring this to an AeadCommon pattern as Windows was, but left it as-is for now. The CCM, GCM, and ChaChaPoly implementations all have some notable differences. ChaChaPoly1305 uses a slightly newer API to get and set the authentication tag. CCM does not call FinalizeEx. CCM needs the input length set via an API call, etc. I felt like trying to centralize that would lead to a method which is just a nest of if (ccmquirk)... if (gcmquirk)... if(chachcaquirk)... that ultimately ended up more difficult to read to save a small amount of duplication.

@vcsjones
vcsjones marked this pull request as ready for review May 10, 2021 03:13
@GrabYourPitchforks GrabYourPitchforks mentioned this pull request May 10, 2021
Closed
4 tasks
GrabYourPitchforks
GrabYourPitchforks reviewed May 10, 2021
View reviewed changes
Comment thread src/libraries/Native/Unix/System.Security.Cryptography.Native/configure.cmake
Comment thread ...m.Security.Cryptography.Algorithms/src/System/Security/Cryptography/ChaCha20Poly1305.Unix.cs

if (associatedData.Length != 0)
{
if (!Interop.Crypto.EvpCipherUpdate(_ctxHandle, Span<byte>.Empty, out _, associatedData))

@GrabYourPitchforks GrabYourPitchforks May 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Out of morbid curiosity, what happens if multiple threads call this at the same time on the same instance? Is there a potential for buffer overrun?

(I know this is copied from the existing AEAD code, so I'm not suggesting changes at this time. Just trying to calculate risk exposure.)

@bartonjs
bartonjs merged commit 31c5a7c into dotnet:main May 11, 2021
@vcsjones
vcsjones deleted the 52482-chacha-unix branch May 12, 2021 00:44
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@bartonjs bartonjs mentioned this pull request Jun 4, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks left review comments
@bartonjs bartonjs bartonjs approved these changes
+1 more reviewer
@am11 am11 am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Security

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@vcsjones @GrabYourPitchforks @am11 @bartonjs @karelz