retire TLS1.0/1.1 ciphers from default list #40746

wfurt · 2020-08-13T01:55:51Z

This is follow-up on #39633 and #40303. Product change is update to DOTNET_DEFAULT_CIPHERSTRING to have it the way how @bartonjs originally wanted. As #40303 fixed most tests, this should just work now.

Bulk of this change is update to PlatformDetection.SupportsTlsXX. In the past that would be based on OS version but since this impacts wide ranges of distributions, the static mapping is difficult to maintain and it created fragility for developers as their systems may be different.

So instead of static map, This change updated detection of Tls 1.0/1.1 to actually try to do handshake using our PAL CryptoNative_SslCtxCreate(). I originally wanted to do that using Interop but that is heavily intertwined with SslStream and build and dependencies were messy so I eventually give up. Also the platform detection is used in may places and build is special so I did not want to pull inn dependency on SslStream.

Instead, I added one function to OpenSSL PAL. That has minimal code to generate certificate on the flaw and test handshake using memory BIO. I had to pull in few more functions but the outcome is self contained and the decision only depends on OpenSSL it self. I may eventually updated other parts as well but for now I'm happy it coverts the two "problematic" version.

contributes to #30767

ghost · 2020-08-13T01:55:53Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.c

src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c

geoffkizer

This is only test changes right?

A few comments above but generally LGTM. I don't grok all the OpenSsl stuff, so you may want to get a review from someone who understands that better.

wfurt · 2020-08-13T02:12:39Z

This is only test changes right?

no. DOTNET_DEFAULT_CIPHERSTRING impacts the product. With that in place, SslStream may or may not be able to negotiate older TLS versions. (depending on openssl configuration) @bartonjs can probably explain better.

geoffkizer · 2020-08-13T02:21:25Z

Ok, seems like @bartonjs should review this.

bartonjs · 2020-08-13T04:17:50Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c

+    return ret;
+}
+
+int32_t CryptoNative_OpenSslGetProtocolSupport(SslProtocols protocol)


This is a lot of native code to add for a test probe, especially with needing to add creating a self-signed cert. If it's just for tests can we detect the state somehow else? Maybe move the cert generation to managed code and leave this as passing in a cert and a protocol? (Then it's easier to expand to test for the combinatorics of {RSA,DSA,ECDSA} x { TLS protocol versions })

that is where I started. But the PlatfdormDetection build did not work for me the way other tests projects work and I was getting unresolved references. I can try to ping some build experts.

While I'd prefer to see this as

Suggested change

int32_t CryptoNative_OpenSslGetProtocolSupport(SslProtocols protocol)

int32_t CryptoNative_OpenSslGetProtocolSupport(SslProtocols protocol, X509* cert, EVP_PKEY* key)

and the cert generation moved to managed code (and then no longer need the X509 mutation functions), if this is what we have to do based on the calendar, it's OK for a test probe.

wfurt · 2020-08-13T04:18:53Z

#39633 claimed:

Anything provided via the CipherSuitePolicy, otherwise
Anything provided via config (OPENSSL_CONF or default, OpenSSL 1.1+ only)
An opinionated default

There is problem with OpenSSL 1.0 as CipherSuitePolicy throws PNSP and config is not available.
So user would be stuck unconditionally with "opinionated default".
That seems like support pain as older systems (more likely needing legacy behavior) would be stuck on TLS12 only.
It seems like we should not enforce CipherSuitePolicy if there is no override.

bartonjs · 2020-08-13T04:28:14Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c

@@ -42,7 +43,7 @@ static int32_t g_config_specified_ciphersuites = 0;

 static void DetectCiphersuiteConfiguration()
 {
-    // This routine will always produce g_config_specified_ciphersuites = 0 on OpenSSL 1.0.x,
+    // This routine will always produce g_config_specified_ciphersuites = 1 on OpenSSL 1.0.x,


If a non-portable build is built against OpenSSL 1.0.x it'll still return 0 with the current code.

bartonjs · 2020-08-13T04:29:17Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c

+        if (API_EXISTS(SSL_state))
+        {
+            // OpenSSL 1.0 does not support CipherSuites so there is no way for caller to override default
+            g_config_specified_ciphersuites = 1;


If the intent is really "OpenSSL 1.0.x should use the OpenSSL default list" (assuming @blowdart said OK), this check could be much earlier and shortcut a lot of work.

What is the percentage of platforms stuck on OpenSSL 1.0?

I don't have any numbers. AFAIK That covers Ubuntu 16 and some Cernots and RH builds.

cc: @tmds @omajid

RHEL.7 (EOL Aug 2021), CentOS.7 (mainstream support ended, extended ends 2024), Ubuntu 16.04 (EOL April 2021).

Probably anything that hit RTM before ~Mar 2018; which might be just those three.

Oh, and the Fedora/RHEL builds will already hit a special case where they have improved the built-in defaults. So we might only really be talking Ubuntu 16.04. Debian 9 (Stretch) has already received its last update, and is now only in extended support.

RHEL.7 (EOL Aug 2021), CentOS.7 (mainstream support ended, extended ends 2024), Ubuntu 16.04 (EOL April 2021).

CentOS 7 is based on the RHEL 7 lifecycle. RHEL 7's EOL would be Aug 2024 (or later).

bartonjs · 2020-08-14T16:15:26Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.c

+        if (x509->cert_info->validity->notBefore)
+        {
+            ASN1_TIME_free(x509->cert_info->validity->notBefore);
+        }
+
+        x509->cert_info->validity->notBefore = ASN1_STRING_dup(time);


This should be wrapped in if (x509->cert_info->validity->notBefore != time), both for side effect observability reasons, and to avoid X509_set1_notBefore(cert, X509_get0_notBefore(cert)) from segfaulting.

bartonjs · 2020-08-14T16:15:56Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.c

+        if (x509->cert_info->validity->notAfter)
+        {
+            ASN1_TIME_free(x509->cert_info->validity->notAfter);
+        }
+
+        x509->cert_info->validity->notAfter = ASN1_STRING_dup(time);


Similarly should be guarded with "if time isn't the current value"

bartonjs · 2020-08-14T16:18:37Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.c

@@ -19,7 +19,7 @@

 #define CRYPTO_LOCK_X509 3
 #define CRYPTO_LOCK_EVP_PKEY 10
-
+#define CRYPTO_LOCK_BIO 21


Suggested change

#define CRYPTO_LOCK_BIO 21

#define CRYPTO_LOCK_BIO 21

Please leave the separation between the crypto lock IDs and the SSL_CTRL IDs.

bartonjs · 2020-08-14T16:25:22Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c

+        {
+            // OpenSSL 1.0 does not support CipherSuites so there is no way for caller to override default
+            g_config_specified_ciphersuites = 1;
+        }
    }
    else
    {


I can't put a comment far enough down, but if the intent is to just use the builtin default on OpenSSL 1.0, replace the complex #elif !defined(FEATURE_DISTRO_AGNOSTIC_SSL) && defined(SSL_SYSTEM_DEFAULT_CIPHER_LIST) with just #else

bartonjs · 2020-08-14T16:25:57Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/opensslshim.h

+#define X509_new X509_new_ptr
 #define X509_NAME_get_index_by_NID X509_NAME_get_index_by_NID_ptr


Suggested change

#define X509_new X509_new_ptr

#define X509_NAME_get_index_by_NID X509_NAME_get_index_by_NID_ptr

#define X509_NAME_get_index_by_NID X509_NAME_get_index_by_NID_ptr

#define X509_new X509_new_ptr

bartonjs · 2020-08-14T16:26:16Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/opensslshim.h

 #define X509_subject_name_hash X509_subject_name_hash_ptr
+#define X509_set_pubkey X509_set_pubkey_ptr


Suggested change

#define X509_subject_name_hash X509_subject_name_hash_ptr

#define X509_set_pubkey X509_set_pubkey_ptr

#define X509_set_pubkey X509_set_pubkey_ptr

#define X509_subject_name_hash X509_subject_name_hash_ptr

danmoseley · 2020-08-14T16:31:33Z

I see this is tagged breaking change -- we don't have much preview time left for feedback to get an idea of the level of impact. Should we have a way for customers to force the old behavior, an AppContext?

bartonjs · 2020-08-14T16:34:53Z

src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs

+            if (!IsOpenSslSupported)
+            {
+                // on Windows and macOS TLS1.0/1.1 are supported.
+                return false;


The comment doesn't make sense with the return. Perhaps this if should just be Debug.Assert(IsOpenSslSupported);?

bartonjs · 2020-08-14T16:36:49Z

Should we have a way for customers to force the old behavior, an AppContext?

They can set it in OpenSSL's config. AppContext won't work because the place that needs to read it is native code.

bartonjs · 2020-08-14T16:39:00Z

Some nitpicks, but LGTM once all 1.0-based executions agree on which default list to use (and Barry has OK'd/asserted the choice)

wfurt · 2020-08-14T19:18:54Z

thanks @bartonjs. I'm still looking into doing the cert in managed code. If I don't get that working today, I'll proceed with the change (assuming @blowdart is OK). I know it is large C chunk but if I find better way it can still go to 5 later as safe test-only change or it can stay in master.

blowdart · 2020-08-14T20:55:51Z

I would say this needs to be accompanied by very clear documentation on the docs site on how to reenable them if you need to, but otherwise, have fun :)

wfurt · 2020-08-14T21:01:34Z

I don't understand your comment @blowdart. The problem is that with old (1.0) OpenSSL you cannot override the default to the proposal and current change is to level it as it was before on old OSes. With 1.1, we will strip pout TLS1.0/1.1 ciphers and proceeded with opinionated default. And yes, we should document and verify that override actually works.

blowdart · 2020-08-14T21:03:53Z

I mean document how, if for some ungodly reason, someone needs TLS1.1, document how they can turn it add it back. You're not stripping entirely just setting the default selection because linux & openssl can't be trusted to be sensible :),

danmoseley · 2020-09-24T18:42:25Z

it seems this broke powershell ? (mentioned in PowerShell/PowerShell#13643) which reinforces that we should have a breaking change note published.

bartonjs · 2020-09-24T18:47:14Z

@wfurt I wonder if both of us were silently thinking the other was going to write the documentation for this. If you haven't already started, I'll find some time today or tomorrow to do it.

wfurt · 2020-09-24T18:58:15Z

that would be great @bartonjs. It was in my TODO list but I got distracted with some other activities ;(

omajid · 2020-10-05T14:45:23Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/opensslshim.h

@@ -1048,13 +1074,16 @@ FOR_ALL_OPENSSL_FUNCTIONS
 #define SSL_is_init_finished local_SSL_is_init_finished
 #define X509_CRL_get0_nextUpdate local_X509_CRL_get0_nextUpdate
 #define X509_NAME_get0_der local_X509_NAME_get0_der
+#define X509_new local_X509_new


This is a leftover and should be removed, right?

I created a PR for this: #43039

retire TLS1.0/1.1 ciphers from default list

10dd81c

wfurt added area-System.Net.Security os-linux Linux OS (any supported distro) labels Aug 13, 2020

wfurt requested review from bartonjs and a team August 13, 2020 01:55

wfurt self-assigned this Aug 13, 2020

geoffkizer reviewed Aug 13, 2020

View reviewed changes

src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.c Outdated Show resolved Hide resolved

geoffkizer reviewed Aug 13, 2020

View reviewed changes

src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.c Outdated Show resolved Hide resolved

geoffkizer reviewed Aug 13, 2020

View reviewed changes

src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c Outdated Show resolved Hide resolved

geoffkizer approved these changes Aug 13, 2020

View reviewed changes

wfurt added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Aug 13, 2020

wfurt added 4 commits August 12, 2020 19:58

fix build with 1.0

199a951

feedback from review

288e7bb

add compat bio_st

e451cdb

don't enforce cipher default on openssl 1.0

88df62e

bartonjs reviewed Aug 13, 2020

View reviewed changes

fix ServerAsyncAuthenticate_MismatchProtocols_Fails

b26bae5

bartonjs reviewed Aug 14, 2020

View reviewed changes

danmoseley added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Aug 14, 2020

wfurt added 2 commits August 17, 2020 03:27

call ERR_clear_error after protocol det:w!ection

9e81bdd

feedback from review

714b2e0

bartonjs approved these changes Aug 17, 2020

View reviewed changes

add missing Using

327409a

wfurt merged commit 161f53c into dotnet:master Aug 17, 2020

karelz added this to the 5.0.0 milestone Aug 18, 2020

wfurt mentioned this pull request Aug 19, 2020

set TLS versions explicitly to prevent SNI test failure on Deb10 #41014

Merged

daxian-dbw mentioned this pull request Sep 18, 2020

Update PowerShell to build against dotnet 5.0-RC.1 PowerShell/PowerShell#13643

Merged

14 tasks

omajid reviewed Oct 5, 2020

View reviewed changes

bartonjs removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 6, 2020

Tratcher mentioned this pull request Oct 23, 2020

TlsHandshakeRejectsTlsLessThan12 dotnet/aspnetcore#26154

Closed

dotnet locked as resolved and limited conversation to collaborators Dec 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

retire TLS1.0/1.1 ciphers from default list #40746

retire TLS1.0/1.1 ciphers from default list #40746

wfurt commented Aug 13, 2020

ghost commented Aug 13, 2020

geoffkizer left a comment

wfurt commented Aug 13, 2020

geoffkizer commented Aug 13, 2020

bartonjs Aug 13, 2020

wfurt Aug 13, 2020

bartonjs Aug 14, 2020

wfurt commented Aug 13, 2020

bartonjs Aug 13, 2020

bartonjs Aug 13, 2020

blowdart Aug 13, 2020

wfurt Aug 13, 2020

bartonjs Aug 13, 2020

bartonjs Aug 14, 2020

omajid Aug 17, 2020

bartonjs Aug 14, 2020

bartonjs Aug 14, 2020

bartonjs Aug 14, 2020

bartonjs Aug 14, 2020

bartonjs Aug 14, 2020

bartonjs Aug 14, 2020

danmoseley commented Aug 14, 2020

bartonjs Aug 14, 2020

bartonjs commented Aug 14, 2020

bartonjs commented Aug 14, 2020

wfurt commented Aug 14, 2020

blowdart commented Aug 14, 2020

wfurt commented Aug 14, 2020

blowdart commented Aug 14, 2020

danmoseley commented Sep 24, 2020

bartonjs commented Sep 24, 2020

wfurt commented Sep 24, 2020

omajid Oct 5, 2020

omajid Oct 5, 2020

	int32_t CryptoNative_OpenSslGetProtocolSupport(SslProtocols protocol)
	int32_t CryptoNative_OpenSslGetProtocolSupport(SslProtocols protocol, X509* cert, EVP_PKEY* key)

		#define X509_new X509_new_ptr
		#define X509_NAME_get_index_by_NID X509_NAME_get_index_by_NID_ptr

		#define X509_subject_name_hash X509_subject_name_hash_ptr
		#define X509_set_pubkey X509_set_pubkey_ptr

retire TLS1.0/1.1 ciphers from default list #40746

retire TLS1.0/1.1 ciphers from default list #40746

Conversation

wfurt commented Aug 13, 2020

ghost commented Aug 13, 2020

geoffkizer left a comment

Choose a reason for hiding this comment

wfurt commented Aug 13, 2020

geoffkizer commented Aug 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfurt commented Aug 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Aug 14, 2020

Choose a reason for hiding this comment

bartonjs commented Aug 14, 2020

bartonjs commented Aug 14, 2020

wfurt commented Aug 14, 2020

blowdart commented Aug 14, 2020

wfurt commented Aug 14, 2020

blowdart commented Aug 14, 2020

danmoseley commented Sep 24, 2020

bartonjs commented Sep 24, 2020

wfurt commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment