Skip to content

[MacCatalyst] Make AppleCryptoNative_SslSetEnabledCipherSuites check for 32 bit & 16 bit SSLCipherSuite #57623

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.

Sign up for GitHub

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

Merged
steveisok merged 2 commits into from
Aug 26, 2021
Merged

[MacCatalyst] Make AppleCryptoNative_SslSetEnabledCipherSuites check for 32 bit & 16 bit SSLCipherSuite #57623

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8336312
[MacCatalyst] Make AppleCryptoNative_SslSetEnabledCipherSuites check …
Aug 18, 2021
82d1a43
Feedback
Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,19 +596,19 @@ int32_t AppleCryptoNative_SslSetEnabledCipherSuites(SSLContextRef sslContext, co
// Max numCipherSuites is 2^16 (all possible cipher suites)
assert(numCipherSuites < (1 << 16));

#if !defined(TARGET_MACCATALYST) && !defined(TARGET_IOS) && !defined(TARGET_TVOS)
#if !defined(TARGET_IOS) && !defined(TARGET_TVOS)
Copy link
Member

@filipnavara filipnavara Aug 18, 2021

Choose a reason for hiding this comment

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

Suggested change
#if !defined(TARGET_IOS) && !defined(TARGET_TVOS)
#if !defined(TARGET_CPU_ARM64) && !defined(TARGET_IOS) && !defined(TARGET_TVOS)

So, Apple headers are still broken and return sizeof(SSLCipherSuite) == sizeof(uint32_t) for MacCatalyst arm64. The simplest fix is to use the else (uint16_t) branch on any arm64 architecture.

Here's the additional rationale why the headers are broken: The system libraries that the app links to at runtime are identical for macOS and Mac Catalyst. Some of the APIs do runtime checks to adjust behavior but that's not the case for SSLSetEnabledCiphers. On macOS the API provably takes uint16_t and that's what is specified correctly in the system headers. For Mac Catalyst arm64 the SSLSetEnabledCiphers was already long obsolete by the time the arm64 architecture was added so Apple probably didn't bother with changing the type in the header.

Copy link
Member

@akoeplinger akoeplinger Aug 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good though TARGET_CPU_ARM64 is coming from TargetConditionals.h and I'm not sure it's available here. We already have our own define TARGET_ARM64 that we're already using in other parts of the PAL so better to use that one.

Copy link
Member

@filipnavara filipnavara Aug 18, 2021

Choose a reason for hiding this comment

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

TargetConditionals.h is included but I am fine with either one as long as it works. I knew there was some other #define but I was not able to find where it is defined :-)

if (sizeof(SSLCipherSuite) == sizeof(uint32_t))
{
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
// macOS
// macOS & MacCatalyst x64
return SSLSetEnabledCiphers(sslContext, (const SSLCipherSuite *)cipherSuites, (size_t)numCipherSuites);
#pragma clang diagnostic pop
}
else
#endif
{
// MacCatalyst, iOS, tvOS, watchOS
// MacCatalyst arm64, iOS, tvOS, watchOS
SSLCipherSuite* cipherSuites16 = (SSLCipherSuite*)calloc((size_t)numCipherSuites, sizeof(SSLCipherSuite));

if (cipherSuites16 == NULL)
Expand Down