Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ChaCha20Poly1305.IsSupported always returns false on non-Windows #52482

Open
3 of 4 tasks
GrabYourPitchforks opened this issue May 7, 2021 · 21 comments
Open
3 of 4 tasks

Comments

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented May 7, 2021

Follow-up to #45130. We checked in an implementation of ChaCha20Poly1305 for Windows, but we don't have one for Mac / Linux scenarios.

This work item tracks calling in to the existing CryptoKit / OpenSSL implementation for Mac / Linux scenarios so that we don't forget to do this work.

Implementations needed for:

Marked up for grabs.

@GrabYourPitchforks GrabYourPitchforks added area-System.Security help wanted [up-for-grabs] Good issue for external contributors labels May 7, 2021
@ghost
Copy link

ghost commented May 7, 2021

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

Issue Details

Follow-up to #45130. We checked in an implementation of ChaCha20Poly1305 for Windows, but we don't have one for Mac / Linux scenarios.

This work item tracks calling in to the existing CryptoKit / OpenSSL implementation for Mac / Linux scenarios so that we don't forget to do this work.

Marked up for grabs.

Author: GrabYourPitchforks
Assignees: -
Labels:

area-System.Security, up-for-grabs

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 7, 2021
@GrabYourPitchforks GrabYourPitchforks added this to the 6.0.0 milestone May 7, 2021
@jkoritzinsky
Copy link
Member

It looks like ChaCha20Poly1305 is also available on Android for newer API levels, so it might be worth considering conditionally providing an implementation there as well.

@vcsjones
Copy link
Member

vcsjones commented May 8, 2021

@bartonjs what is your feeling on waiting for OpenSSL 3.0 work to be done? Do you think much or any work in this area would be impacted by it?

@bartonjs
Copy link
Member

bartonjs commented May 9, 2021

what is your feeling on waiting for OpenSSL 3.0 work to be done? Do you think much or any work in this area would be impacted by it?

No concerns. Since we now work with OSSL3 using both direct and portable builds everything else is just preemptive cleanup (e.g. get wholly onto EVP_PKEY), which is why the effort got slightly backburnered.

@filipnavara
Copy link
Member

filipnavara commented May 9, 2021

I started a proof of concept of the CryptoKit part (https://github.com/dotnet/runtime/compare/main...filipnavara:cryptokit?expand=1) but it's huge fight with CMake and Apple tools so far.

image

@jkoritzinsky
Copy link
Member

@filipnavara instead of discovering the Swift toolchain on your own, you can try adding an enable_language(Swift) line in the CMakeLists.txt file. I think we use a new enough CMake on Mac that Swift support is built-in.

@filipnavara
Copy link
Member

@jkoritzinsky Unfortunately it's only supported on Ninja and Xcode, not on Unix makefiles. Even on Ninja it doesn't quite mix well with other languages.

I am still evaluating how to improve the situation for static libraries consumed by apphost and Xamarin.

@jkoritzinsky
Copy link
Member

That sucks. Can you make sure to include a comment in the file that discovers the Swift tool chain explaining that so we know when looking back why we manually discover?

@filipnavara
Copy link
Member

Sure. I don't even know if I will be able to make it work reliably for all use cases yet. I am trying different approaches and this was just first one that got far enough to produce some visible results.

@GrabYourPitchforks
Copy link
Member Author

I updated the issue text with a bunch of check boxes for the various OSes. That way we can do the PRs piecemeal if that'd be easier.

If I missed any OS please feel free to update the issue text directly.

@vcsjones
Copy link
Member

vcsjones commented May 9, 2021

@filipnavara you've gotten much further than I have, I think you found my branch (there are a couple) so feel free to take any scraps of code there that are helpful.

OpenSSL seems straight forward, so I will tackle that this afternoon.

@filipnavara
Copy link
Member

Sure enough the approach I took was fundamentally broken :-) The stable Swift runtime is only shipped on iOS 12.2+ and macOS 10.14.4+. Lot of the hacks from the CMake script are unnecessary when explicitly targeting newer versions. It would not work on downlevel platforms though and my hacked version doesn't work either.

It quite easy to solve for the case with dynamic libraries but it's much more tricky for the static libraries. I will try to play a bit with weak linking to see if I can get something running.

Not sure whether the dynamic-only version would be worth the trouble, especially since it cannot be used on iOS [where OpenSSL is not available].

@filipnavara
Copy link
Member

filipnavara commented May 11, 2021

I'm leaning towards abandoning the CryptoKit experiment.

The last version is available at https://github.com/dotnet/runtime/compare/main...filipnavara:cryptokit?expand=1. It enables the weak linking of Swift runtime and the library loads on down-level platforms to the extent that graceful fallback is possible in the managed code. However, there are few reasons why I think it's not worth the trouble:

  • External consumers of the library (Xamarin) would need a way to pass the necessary linker flags. Unfortunately the auto-link functionality in Mach-O / LLVM / Apple Linker is limited to very narrow set of flags (-l, -need-l, -framework) and cannot be used to transport the flags necessary for external consumers of the static library (-L/usr/bin/swift, -weak-l and -rpath).
  • The linking of the static library into an executable trips an assert in the Apple linker. After one day of debugging I figured a workaround of passing -lobjc to linker (despite the same flag already being present in the auto-link sections of the object files).
  • AES-GCM implementation in CryptoKit is limited to 16-byte tag sizes. That would make the macOS implementation awkwardly limited especially if it was to fallback to OpenSSL.

For additional reference I also included experiment with using Apple private CommonCrypto APIs which has different drawbacks:

  • It's a [semi-documented] private API which could be problem on App Store. It can be linked out if the algorithms are unused though.
  • AES-GCM is available on earlier platforms (possibly even all supported iOS, macOS versions)
  • AES-GCM works with all valid tag sizes
  • ChaCha20Poly1305 is available only on macOS 11+/iOS 14+ (CryptoKit has it on macOS 10.15+, iOS 13+).

@vcsjones
Copy link
Member

I'm leaning towards abandoning the CryptoKit experiment.

Thanks again for your very deep research in to this. Unfortunate that CryptoKit is problematic in the static linking scenario.

AES-GCM implementation in CryptoKit is limited to 16-byte tag sizes.

Hm. Yeah that would be a breaking change for folks that are using it just fine on a Mac with OpenSSL. Though 128 bit tags is by far the most common tag size used.

We know what tag size should be used at invocation time, we could just fall back to OpenSSL again if they are requesting or using a tag length that is not 128 bits.

Using Apple private CommonCrypto APIs

I'm having trouble finding the issue in which this was discussed, but the conclusion then was that we could not use the private APIs (even if they are "documented" at open source.apple.com)

The main issue I am seeing then is with static linking and CryptoKit.

@filipnavara
Copy link
Member

filipnavara commented May 11, 2021

The static linking scenario is a solvable one even if it requires some cooperation on the Xamarin side. Ideally we would need to embed the necessary linker flags in some way in the runtime NuGet consumed by Xamarin and implement a way to use them there. It's non-trivial but very much doable and possibly there are other scenarios where this would be useful (cc @rolfbjarne).

UPD: It would be pretty trivial to put a static file in the NuGet and read it here and append it all to _MainLinkerFlags:
https://github.com/xamarin/xamarin-macios/blob/5ef7fb27847109973998f8a925f3e7cdd1972829/dotnet/targets/Xamarin.Shared.Sdk.targets#L627-L629

Though 128 bit tags is by far the most common tag size used. We know what tag size should be used at invocation time, we could just fall back to OpenSSL again if they are requesting or using a tag length that is not 128 bits.

We could do that but at the moment I am slightly tipped on the benefits/drawbacks scale towards not doing that. However, on platforms like iOS where OpenSSL is not available a limited implementation may be a tiny bit better than no implementation at all.

Using Apple private CommonCrypto APIs

I'm having trouble finding the issue in which this was discussed, but the conclusion then was that we could not use the private APIs (even if they are "documented" at open source.apple.com)

It's a slippery slope here. I did the experiment only to see if it could be done but I don't think that is a code that should be included in the .NET runtime. There's anecdotal evidence that Apple doesn't reject apps using these APIs in app store reviews but that's likely not good enough. The only good thing about the approach is that it's super simple and it can be linked out by the managed linker. I am considering wrapping this part in a C# library and just publishing it as NuGet.

@vcsjones
Copy link
Member

In an effort to learn something new, I thought I would give the Android implementation a shot. Looks like there are at least some existing patterns I can follow for lighting-up functionality depending on the API Level (28+ for ChaCha20Poly1305).

@jkoritzinsky
Copy link
Member

If you have any questions on it, let me know and I can probably help out.

@iSazonov
Copy link
Contributor

It seems the issue can be closed.

@vcsjones
Copy link
Member

This is still tracking iOS/tvOS support for ChaCha20Poly1305 which currently has some blockers in to implement.

@PiotrKowalski93
Copy link

PiotrKowalski93 commented Jun 19, 2023

I have this problem on Window 10.
Using VisualStudio 2022, .Net 6

Below are details:

image

image

image

@jkoritzinsky

@akoeplinger
Copy link
Member

@PiotrKowalski93 your Windows version is too old, the algorithm is only supported in 10.0.20142 and later (see #45130)

@vcsjones vcsjones modified the milestones: Future, 9.0.0 Dec 12, 2023
@vcsjones vcsjones removed the help wanted [up-for-grabs] Good issue for external contributors label Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants