-
Notifications
You must be signed in to change notification settings - Fork 4.9k
CipherSuitePolicy implementation #36775
Changes from all commits
3102766
4323618
5e3b703
3031634
af30465
ef3519d
1a6f1f7
80fc8a1
9978be7
d402d7b
6e78270
fab7fd1
8c5e2d5
8a2a49e
ea000aa
60e1235
d106a33
0975c53
b1c3382
7bc25fb
550ad52
2ad5374
b6fce4f
6fdb6b4
3a62889
bb57ff8
314d2c9
0382355
9b7c610
85c1dd0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,31 +64,55 @@ internal static SafeSslHandle AllocateSslContext(SslProtocols protocols, SafeX50 | |
| throw CreateSslException(SR.net_allocate_ssl_context_failed); | ||
| } | ||
|
|
||
| // TLS 1.3 uses different ciphersuite restrictions than previous versions. | ||
| // It has no equivalent to a NoEncryption option. | ||
| if (policy == EncryptionPolicy.NoEncryption) | ||
| if (!Interop.Ssl.Tls13Supported) | ||
| { | ||
| if (protocols != SslProtocols.None && | ||
| CipherSuitesPolicyPal.WantsTls13(protocols)) | ||
| { | ||
| protocols = protocols & (~SslProtocols.Tls13); | ||
| } | ||
| } | ||
| else if (CipherSuitesPolicyPal.WantsTls13(protocols) && | ||
| CipherSuitesPolicyPal.ShouldOptOutOfTls13(sslAuthenticationOptions.CipherSuitesPolicy, policy)) | ||
| { | ||
| if (protocols == SslProtocols.None) | ||
| { | ||
| // we are using default settings but cipher suites policy says that TLS 1.3 | ||
| // is not compatible with our settings (i.e. we requested no encryption or disabled | ||
| // all TLS 1.3 cipher suites) | ||
| protocols = SslProtocols.Tls | SslProtocols.Tls11 | SslProtocols.Tls12; | ||
| } | ||
| else | ||
| { | ||
| protocols &= ~SslProtocols.Tls13; | ||
|
|
||
| if (protocols == SslProtocols.None) | ||
| { | ||
| throw new SslException( | ||
| SR.Format(SR.net_ssl_encryptionpolicy_notsupported, policy)); | ||
| } | ||
| // user explicitly asks for TLS 1.3 but their policy is not compatible with TLS 1.3 | ||
| throw new SslException( | ||
| SR.Format(SR.net_ssl_encryptionpolicy_notsupported, policy)); | ||
| } | ||
| } | ||
|
|
||
| if (CipherSuitesPolicyPal.ShouldOptOutOfLowerThanTls13(sslAuthenticationOptions.CipherSuitesPolicy, policy)) | ||
| { | ||
| if (!CipherSuitesPolicyPal.WantsTls13(protocols)) | ||
| { | ||
| // We cannot provide neither TLS 1.3 or non TLS 1.3, user disabled all cipher suites | ||
| throw new SslException( | ||
| SR.Format(SR.net_ssl_encryptionpolicy_notsupported, policy)); | ||
| } | ||
|
|
||
| protocols = SslProtocols.Tls13; | ||
| } | ||
|
|
||
| // Configure allowed protocols. It's ok to use DangerousGetHandle here without AddRef/Release as we just | ||
| // create the handle, it's rooted by the using, no one else has a reference to it, etc. | ||
| Ssl.SetProtocolOptions(innerContext.DangerousGetHandle(), protocols); | ||
|
|
||
| // Sets policy and security level | ||
| if (!Ssl.SetEncryptionPolicy(innerContext, policy)) | ||
| { | ||
| throw new SslException( | ||
| SR.Format(SR.net_ssl_encryptionpolicy_notsupported, policy)); | ||
| } | ||
|
|
||
| // The logic in SafeSslHandle.Disconnect is simple because we are doing a quiet | ||
| // shutdown (we aren't negotiating for session close to enable later session | ||
| // restoration). | ||
|
|
@@ -98,10 +122,23 @@ internal static SafeSslHandle AllocateSslContext(SslProtocols protocols, SafeX50 | |
| // https://www.openssl.org/docs/manmaster/ssl/SSL_shutdown.html | ||
| Ssl.SslCtxSetQuietShutdown(innerContext); | ||
|
|
||
| if (!Ssl.SetEncryptionPolicy(innerContext, policy)) | ||
| byte[] cipherList = | ||
| CipherSuitesPolicyPal.GetOpenSslCipherList(sslAuthenticationOptions.CipherSuitesPolicy, protocols, policy); | ||
|
|
||
| byte[] cipherSuites = | ||
| CipherSuitesPolicyPal.GetOpenSslCipherSuites(sslAuthenticationOptions.CipherSuitesPolicy, protocols, policy); | ||
|
|
||
| unsafe | ||
| { | ||
| Crypto.ErrClearError(); | ||
| throw new PlatformNotSupportedException(SR.Format(SR.net_ssl_encryptionpolicy_notsupported, policy)); | ||
| fixed (byte* cipherListStr = cipherList) | ||
| fixed (byte* cipherSuitesStr = cipherSuites) | ||
| { | ||
| if (!Ssl.SetCiphers(innerContext, cipherListStr, cipherSuitesStr)) | ||
| { | ||
| Crypto.ErrClearError(); | ||
| throw new PlatformNotSupportedException(SR.Format(SR.net_ssl_encryptionpolicy_notsupported, policy)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would hit this case?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i.e. all cipher suites provided by user are known by open ssl but disabled by default, in such case open ssl returns that none of the cipher suites can be used (i.e. NULL cipher suites).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps error can be improved (it will be wrapped by authentication exception anyway though) |
||
| } | ||
| } | ||
| } | ||
|
|
||
| bool hasCertificateAndKey = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -443,12 +443,44 @@ int32_t AppleCryptoNative_SslGetProtocolVersion(SSLContextRef sslContext, PAL_Ss | |
| return osStatus; | ||
| } | ||
|
|
||
| int32_t AppleCryptoNative_SslGetCipherSuite(SSLContextRef sslContext, uint32_t* pCipherSuiteOut) | ||
| int32_t AppleCryptoNative_SslGetCipherSuite(SSLContextRef sslContext, uint16_t* pCipherSuiteOut) | ||
| { | ||
| if (pCipherSuiteOut == NULL) | ||
| *pCipherSuiteOut = 0; | ||
| { | ||
| return errSecParam; | ||
| } | ||
|
|
||
| SSLCipherSuite cipherSuite; | ||
| OSStatus status = SSLGetNegotiatedCipher(sslContext, &cipherSuite); | ||
| *pCipherSuiteOut = (uint16_t)cipherSuite; | ||
|
|
||
| return status; | ||
| } | ||
|
|
||
| int32_t AppleCryptoNative_SslSetEnabledCipherSuites(SSLContextRef sslContext, const uint32_t* cipherSuites, int32_t numCipherSuites) | ||
| { | ||
| // Max numCipherSuites is 2^16 (all possible cipher suites) | ||
| assert(numCipherSuites < (1 << 16)); | ||
|
|
||
| return SSLGetNegotiatedCipher(sslContext, pCipherSuiteOut); | ||
| if (sizeof(SSLCipherSuite) == sizeof(uint32_t)) | ||
| { | ||
| // macOS | ||
| return SSLSetEnabledCiphers(sslContext, cipherSuites, (size_t)numCipherSuites); | ||
| } | ||
| else | ||
| { | ||
| // iOS, tvOS, watchOS | ||
| SSLCipherSuite* cipherSuites16 = (SSLCipherSuite*)malloc(sizeof(SSLCipherSuite) * (size_t)numCipherSuites); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Return value of malloc should be checked against nullptr for OOM conditions. (And honestly it's best practice to use calloc instead of malloc.)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree on OOM, for calloc comment: we fill it out anyway instruction later so not sure why use calloc - we will double initialize
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't a hot code path, so not concerned about double-initialization. In general the primary benefit of calloc is that you avoid possible integer overflow when computing the total required byte size. I can't imagine this particular call would overflow, but it's a good habit to get in to. |
||
| for (int i = 0; i < numCipherSuites; i++) | ||
| { | ||
| cipherSuites16[i] = (SSLCipherSuite)cipherSuites[i]; | ||
| } | ||
|
|
||
| OSStatus status = SSLSetEnabledCiphers(sslContext, cipherSuites16, (size_t)numCipherSuites); | ||
|
|
||
| free(cipherSuites16); | ||
| return status; | ||
| } | ||
| } | ||
|
|
||
| __attribute__((constructor)) static void InitializeAppleCryptoSslShim() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should Debug.Assert that these are null terminated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already do the checks inside
ShouldOptOutOfTls13andShouldOptOutOfLowerThanTls13but can add another one