CipherSuitePolicy implementation by krwq · Pull Request #36775 · dotnet/corefx

krwq · 2019-04-10T18:53:52Z

Fixes: https://github.com/dotnet/corefx/issues/24588
Fixes: https://github.com/dotnet/corefx/issues/33809

Replacing: #36648 because CI doesn't work correctly with draft PRs

cc: @karelz @joshfree

…s enabled

azure-pipelines · 2019-04-10T22:18:57Z

Azure Pipelines successfully started running 3 pipeline(s), but failed to run 1 pipeline(s).

krwq · 2019-04-10T23:39:28Z

/azp run

azure-pipelines · 2019-04-10T23:39:55Z

Azure Pipelines successfully started running 3 pipeline(s), but failed to run 1 pipeline(s).

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

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

src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.h

src/System.Net.Security/src/System/Net/Security/CipherSuitesPolicy.Linux.cs

…ersion is installed

src/System.Net.Security/src/System/Net/Security/CipherSuitesPolicyPal.Linux.cs

bartonjs · 2019-04-12T22:03:49Z

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

+        ret &= SSL_CTX_set_ciphersuites(ctx, cipherSuites);
    }
+#else
+    (void)cipherSuites;


Should this be ret &= (cipherSuites == NULL);? (aka fail if cipherSuites made it this far?)

the code was initially written with working on lower than 1.1.1 in mind (the only issue with that is we would have to have IANA -> OpenSSL string mapping somewhere since SSL_CIPHER_find is not available), assuming we got here and TLS 1.3 is not supported then setting 1.3 cipher suites will have no effect anyway since we already would have failed when setting < 1.3 cipher suites.

GrabYourPitchforks · 2019-04-13T04:45:48Z

src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs

                Ssl.SslCtxSetQuietShutdown(innerContext);

-                if (!Ssl.SetEncryptionPolicy(innerContext, policy))
+                byte[] cipherList =


Should Debug.Assert that these are null terminated.

I already do the checks inside ShouldOptOutOfTls13 and ShouldOptOutOfLowerThanTls13 but can add another one

GrabYourPitchforks · 2019-04-13T04:53:06Z

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

+    else
+    {
+        // iOS, tvOS, watchOS
+        SSLCipherSuite* cipherSuites16 = (SSLCipherSuite*)malloc(sizeof(SSLCipherSuite) * (size_t)numCipherSuites);


Return value of malloc should be checked against nullptr for OOM conditions. (And honestly it's best practice to use calloc instead of malloc.)

agree on OOM, for calloc comment: we fill it out anyway instruction later so not sure why use calloc - we will double initialize

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.

davidfowl · 2019-04-13T15:17:53Z

Do we need to react to this change in Kestrel?

cc @shirhatti @blowdart @halter73 @Tratcher @anurse

Tratcher · 2019-04-13T15:26:31Z

Yes, exposing this API: https://github.com/dotnet/corefx/pull/36775/files#diff-bd768ea3857573965e853e2ce2563434R134

HTTP/2 is supposed to restrict ciphers. I'll file it. dotnet/aspnetcore#9349

Tratcher · 2019-04-13T15:37:59Z

Was there any discussion for implementing a deny list as well? That's what HTTP/2 calls for.
https://tools.ietf.org/html/rfc7540#section-9.2.2
https://tools.ietf.org/html/rfc7540#appendix-A

davidsh · 2019-04-13T15:44:32Z

Was there any discussion for implementing a deny list as well?

Tons of discussions with the Windows team. We spent a long time discussing this API and what is best for all the platforms (Windows, Linux, etc.). This API and PR is the result of that long effort.

Feel free to watch the API review on YouTube. :)

davidfowl · 2019-04-13T16:00:22Z

@Tratcher it would be good if we could expose this entire type (if it makes sense) so we could avoid the property by property copy when new features are added.

krwq · 2019-04-13T18:03:32Z

@Tratcher for blocking "bad" cipher suites you can simply only allow TLS 1.3 which only allows 5 "safe" cipher suites or specify a list explicitly. Current implementation is ready for adding more advanced filtering in the future: https://github.com/dotnet/corefx/blob/master/src/System.Net.Security/src/System/Net/Security/TlsCipherSuiteData.Lookup.tt - technically it should be possible to expose this as an external tool which generates CipherSuitesPolicy.

ViktorHofer · 2019-04-14T15:58:10Z

This broke the official build: https://dev.azure.com/dnceng/internal/_build/results?buildId=154142

/__w/1/s/src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c(374,14): error GAA0F1F0D: implicit declaration of function 'SSL_CIPHER_find' is invalid in C99 [-Werror,-Wimplicit-function-declaration] [/__w/1/s/src/Native/build-native.proj]
/__w/1/s/src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c(374,12): error GD47D7371: incompatible integer to pointer conversion assigning to 'const SSL_CIPHER *' (aka 'const struct ssl_cipher_st *') from 'int' [-Werror,-Wint-conversion] [/__w/1/s/src/Native/build-native.proj]
/__w/1/s/src/Native/build-native.proj(36,5): error MSB3073: The command ""/__w/1/s/src/Native/build-native.sh" x64 Release Linux outconfig netcoreapp-Linux-Release-x64 stripsymbols" exited with code 1.
    0 Warning(s)
    3 Error(s)

cc @safern

karelz · 2019-04-14T17:23:21Z

If we're blocking official builds, we should back out the change ASAP.

krwq · 2019-04-14T18:09:20Z

@karelz I'll try to address this right now since we don't have good CI coverage for this case

Tratcher · 2019-05-23T23:02:07Z

Feel free to watch the API review on YouTube. :)

@karelz @davidsh Anybody have a link for that? Or at least a date? They're not well indexed or linked from any of the issues.

karelz · 2019-05-24T00:08:27Z

@Tratcher check out api-approved date: 4/2: https://github.com/dotnet/corefx/issues/33809#event-2247491106 ... that's the one you're looking for ;)

* CipherSuitePolicy implementation (Linux) * SSL_CIPHER_find * do not call TLS1.3 APIs on platforms which don't support it * Non-TLS1.3 specific tests are skipped when not enough cipher suites is enabled * clean ups * attempt to fix OSX * another attempt to fix OSX * missing define * address some feedback, try to fix test failures * portable build fix * do not call old set ciphers API when only TLS 1.3 is requested * apply feedback * add OSX implementation * fixes to OSX * explicit convert * use explicit SSLCipherSuite instead of uint16_t * random change to trigger CI * s/unsafe/fixed * fixes * random change to trigger CI * client ordering does not have to win * tests: AllowedCipherSuites, new CipherSuitesPolicy(null) * run AllowedCipherSuites tests only when CSP is supported * add summary on CipherSuitesPolicy * address feedback * move OS specific files to CipherSuitesPolicyPal * FALLBACK->LIGHTUP and remove local_ * do not call 1.1.1 function on non-portable build when lower openssl version is installed * get rid of warning that arg is unused * make CipherSuitesPolicyPal public members internal Commit migrated from dotnet/corefx@07f443b

krwq added 12 commits April 5, 2019 23:48

CipherSuitePolicy implementation (Linux)

3102766

SSL_CIPHER_find

4323618

do not call TLS1.3 APIs on platforms which don't support it

5e3b703

Non-TLS1.3 specific tests are skipped when not enough cipher suites i…

3031634

…s enabled

clean ups

af30465

attempt to fix OSX

ef3519d

another attempt to fix OSX

1a6f1f7

missing define

80fc8a1

address some feedback, try to fix test failures

9978be7

portable build fix

d402d7b

do not call old set ciphers API when only TLS 1.3 is requested

6e78270

apply feedback

fab7fd1

krwq added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 10, 2019

krwq added 4 commits April 10, 2019 13:48

add OSX implementation

8c5e2d5

fixes to OSX

8a2a49e

explicit convert

ea000aa

use explicit SSLCipherSuite instead of uint16_t

60e1235

krwq force-pushed the ciphersuitespolicy branch from 8370757 to 60e1235 Compare April 10, 2019 21:34

krwq added 4 commits April 10, 2019 14:41

random change to trigger CI

d106a33

s/unsafe/fixed

0975c53

fixes

b1c3382

random change to trigger CI

7bc25fb

client ordering does not have to win

550ad52

krwq removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 11, 2019

krwq added 3 commits April 10, 2019 18:24

tests: AllowedCipherSuites, new CipherSuitesPolicy(null)

2ad5374

run AllowedCipherSuites tests only when CSP is supported

b6fce4f

add summary on CipherSuitesPolicy

6fdb6b4

bartonjs reviewed Apr 11, 2019

View reviewed changes

src/Native/Unix/System.Security.Cryptography.Native/opensslshim.h Show resolved Hide resolved

bartonjs reviewed Apr 11, 2019

View reviewed changes

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

bartonjs reviewed Apr 11, 2019

View reviewed changes

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

bartonjs reviewed Apr 11, 2019

View reviewed changes

src/System.Net.Security/src/System/Net/Security/CipherSuitesPolicy.Linux.cs Outdated Show resolved Hide resolved

krwq added 5 commits April 11, 2019 15:14

address feedback

3a62889

move OS specific files to CipherSuitesPolicyPal

bb57ff8

FALLBACK->LIGHTUP and remove local_

314d2c9

do not call 1.1.1 function on non-portable build when lower openssl v…

0382355

…ersion is installed

get rid of warning that arg is unused

9b7c610

bartonjs reviewed Apr 12, 2019

View reviewed changes

src/System.Net.Security/src/System/Net/Security/CipherSuitesPolicyPal.Linux.cs Outdated Show resolved Hide resolved

make CipherSuitesPolicyPal public members internal

85c1dd0

bartonjs reviewed Apr 12, 2019

View reviewed changes

bartonjs approved these changes Apr 12, 2019

View reviewed changes

krwq merged commit 07f443b into dotnet:master Apr 13, 2019

GrabYourPitchforks reviewed Apr 13, 2019

View reviewed changes

krwq mentioned this pull request Apr 13, 2019

Address post merge feedback from #36775 #36839

Merged

krwq mentioned this pull request Apr 14, 2019

Fix build after #36775 #36862

Merged

karelz added this to the 3.0 milestone May 4, 2019

Conversation

krwq commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azure-pipelines bot commented Apr 10, 2019

Uh oh!

krwq commented Apr 10, 2019

Uh oh!

azure-pipelines bot commented Apr 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bartonjs Apr 12, 2019

Choose a reason for hiding this comment

Uh oh!

krwq Apr 12, 2019

Choose a reason for hiding this comment

Uh oh!

GrabYourPitchforks Apr 13, 2019

Choose a reason for hiding this comment

Uh oh!

krwq Apr 13, 2019

Choose a reason for hiding this comment

Uh oh!

GrabYourPitchforks Apr 13, 2019

Choose a reason for hiding this comment

Uh oh!

krwq Apr 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GrabYourPitchforks Apr 13, 2019

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Apr 13, 2019

Uh oh!

Tratcher commented Apr 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher commented Apr 13, 2019

Uh oh!

davidsh commented Apr 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidfowl commented Apr 13, 2019

Uh oh!

krwq commented Apr 13, 2019

Uh oh!

ViktorHofer commented Apr 14, 2019

Uh oh!

karelz commented Apr 14, 2019

Uh oh!

krwq commented Apr 14, 2019

Uh oh!

Tratcher commented May 23, 2019

Uh oh!

karelz commented May 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

krwq commented Apr 10, 2019 •

edited

Loading

krwq Apr 13, 2019 •

edited

Loading

Tratcher commented Apr 13, 2019 •

edited

Loading

davidsh commented Apr 13, 2019 •

edited

Loading