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

nss: Fall back to latest supported SSL version #3337

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@pghmcfc

pghmcfc commented Dec 3, 2018

NSS may be built without support for the latest SSL/TLS versions, leading to "SSL version range is not valid" errors when the library code supports a recent version (e.g. TLS v1.3) but it has explicitly been disabled.

This change adjusts the maximum SSL version requested by libcurl to be the maximum supported version at runtime, as long as that version is at least as high as the minimum version required by libcurl.

Fixes #3261

Note: The SSL_VersionRangeGetSupported API was added in NSS version 3.14 (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.14_release_notes), same as the SSL_VersionRangeGetDefault API, which is already used in curl, so there should be no change in NSS version requirements.

@bagder

This comment has been minimized.

Member

bagder commented Dec 3, 2018

travis reports a little nit:

./vtls/nss.c:1839:5: warning: if with space (SPACEBEFOREPAREN)
   if (sslver_supported.max < sslver.max &&
     ^
Paul Howarth
nss: Fall back to latest supported SSL version
NSS may be built without support for the latest SSL/TLS versions,
leading to "SSL version range is not valid" errors when the library
code supports a recent version (e.g. TLS v1.3) but it has explicitly
been disabled.

This change adjusts the maximum SSL version requested by libcurl to
be the maximum supported version at runtime, as long as that version
is at least as high as the minimum version required by libcurl.

Fixes #3261

@pghmcfc pghmcfc force-pushed the pghmcfc:nss-ssl-version-fallback branch from 109abb8 to 0751db5 Dec 4, 2018

@bagder bagder added the SSL/TLS label Dec 4, 2018

@bagder

bagder approved these changes Dec 4, 2018

We could possibly consider outputting the numerical TLS version numbers as hex, as they are defined as hex in the NSS header file.

@pghmcfc

This comment has been minimized.

pghmcfc commented Dec 4, 2018

I think it would be even better to output them as text strings like "TLSv1.2" where the values are known, and maybe hex otherwise. Perhaps have a function nss_sslver_to_string that's structured like nss_sslver_from_curl?

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Dec 4, 2018

@pghmcfc Sounds good!

@pghmcfc

This comment has been minimized.

pghmcfc commented Dec 4, 2018

How's this look?

$ curl -v "https://github.com/curl/curl/issues/new" --tlsv1.2
*   Trying 192.30.253.112...
* TCP_NODELAY set
* Connected to github.com (192.30.253.112) port 443 (#0)
* Initializing NSS with certpath: sql:/etc/pki/nssdb
* Falling back from TLSv1.3 to max supported SSL version (TLSv1.2)
*   CAfile: none
  CApath: none
* loaded libnssckbi.so
* ALPN, server accepted to use http/1.1
* SSL connection using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
* Server certificate:
* 	subject: CN=github.com,O="GitHub, Inc.",L=San Francisco,ST=California,C=US,serialNumber=5157550,incorporationState=Delaware,incorporationCountry=US,businessCategory=Private Organization
* 	start date: May 08 00:00:00 2018 GMT
* 	expire date: Jun 03 12:00:00 2020 GMT
* 	common name: github.com
* 	issuer: CN=DigiCert SHA2 Extended Validation Server CA,OU=www.digicert.com,O=DigiCert Inc,C=US
> GET /curl/curl/issues/new HTTP/1.1
> Host: github.com
> User-Agent: curl/7.62.0
> Accept: */*
> 
< HTTP/1.1 302 Found
< Server: GitHub.com
< Date: Tue, 04 Dec 2018 12:26:39 GMT
< Content-Type: text/html; charset=utf-8
< Transfer-Encoding: chunked
< Status: 302 Found
< Cache-Control: no-cache
< Vary: X-PJAX
< Location: https://github.com/login?return_to=https%3A%2F%2Fgithub.com%2Fcurl%2Fcurl%2Fissues%2Fnew
< Set-Cookie: has_recent_activity=1; path=/; expires=Tue, 04 Dec 2018 13:26:39 -0000
< Set-Cookie: _gh_sess=OTlnV3M2a1JLcTliT2VYWWRIVWdTc1hRcWdNaVpjMTNqYjZ4ZExYb2hHcmRWYUZHRzdiSGd3NmYrMnpQL3F6R2xJN0hSeklWWjQyZ1g2cmZlQVpPN1NzZFA2MkVVODY0QVc4L3poM1JOZ1l5WlRndktMR2RIRVZvWTVXMERxUS9lZC9yekxXSi9HNU9lUHdCQVRFSWdiaUtGV0t4R1FBVisvdk1ieUVjZDBMMVZraU9HS2FCbW1MbVlpdHhvcTlBLS1zUjZyd1ZmenJISUxWM21TaWg4K1RRPT0%3D--ade31fe886abc73903b35681535b3076b0fd36bf; path=/; secure; HttpOnly
< X-Request-Id: f9ecf2a6-3d94-4cd0-81d7-6377cfd9d426
< Strict-Transport-Security: max-age=31536000; includeSubdomains; preload
< X-Frame-Options: deny
< X-Content-Type-Options: nosniff
< X-XSS-Protection: 1; mode=block
< Expect-CT: max-age=2592000, report-uri="https://api.github.com/_private/browser/errors"
< Content-Security-Policy: default-src 'none'; base-uri 'self'; block-all-mixed-content; connect-src 'self' uploads.github.com status.github.com collector.githubapp.com api.github.com www.google-analytics.com github-cloud.s3.amazonaws.com github-production-repository-file-5c1aeb.s3.amazonaws.com github-production-upload-manifest-file-7fdce7.s3.amazonaws.com github-production-user-asset-6210df.s3.amazonaws.com wss://live.github.com; font-src assets-cdn.github.com; form-action 'self' github.com gist.github.com; frame-ancestors 'none'; frame-src render.githubusercontent.com; img-src 'self' data: assets-cdn.github.com identicons.github.com collector.githubapp.com github-cloud.s3.amazonaws.com *.githubusercontent.com; manifest-src 'self'; media-src 'none'; script-src assets-cdn.github.com; style-src 'unsafe-inline' assets-cdn.github.com
< X-GitHub-Request-Id: A564:5DE5:21CADF0:41C15E2:5C06727F
< 
* Connection #0 to host github.com left intact
<html><body>You are being <a href="https://github.com/login?return_to=https%3A%2F%2Fgithub.com%2Fcurl%2Fcurl%2Fissues%2Fnew">redirected</a>.</body></html>
@kdudka

kdudka approved these changes Dec 4, 2018

Besides the style issues, it looks fine to me.

Show resolved Hide resolved lib/vtls/nss.c Outdated
Show resolved Hide resolved lib/vtls/nss.c Outdated
Paul Howarth
nss: Improve info message when falling back SSL protocol
Use descriptive text strings rather than decimal numbers.

@pghmcfc pghmcfc force-pushed the pghmcfc:nss-ssl-version-fallback branch from a4ed6fe to 01d4b54 Dec 4, 2018

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Dec 4, 2018

Thanks for the fix-ups!

While reviewing the #ifdefs I spotted that the code already uses SSL_LIBRARY_VERSION_TLS_1_2 without checking its availability first, which makes some of the #ifdefs redundant. But this could be cleaned up later.

@pghmcfc

This comment has been minimized.

pghmcfc commented Dec 4, 2018

Maybe the other uses of SSL_LIBRARY_VERSION_TLS_1_2 should be protected. The configure script looks for the function SSL_VersionRangeSet to verify that NSS is usable, and that was introduced in NSS version 3.14 (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.14_release_notes), which supported up to TLSv1.1. TLSv1.2 support (and hence the SSL_LIBRARY_VERSION_TLS_1_2 define) was added in NSS version 3.15.1 (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.15.1_release_notes).

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Dec 4, 2018

Actually, it should be easy to make it consistent (and compatible with old NSS releases) again:

--- a/lib/vtls/nss.c
+++ b/lib/vtls/nss.c
@@ -1807,10 +1807,14 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
   SSLVersionRange sslver = {
     SSL_LIBRARY_VERSION_TLS_1_0,  /* min */
 #ifdef SSL_LIBRARY_VERSION_TLS_1_3
     SSL_LIBRARY_VERSION_TLS_1_3   /* max */
-#else
+#elif defined SSL_LIBRARY_VERSION_TLS_1_2
     SSL_LIBRARY_VERSION_TLS_1_2
+#elif defined SSL_LIBRARY_VERSION_TLS_1_1
+    SSL_LIBRARY_VERSION_TLS_1_1
+#else
+    SSL_LIBRARY_VERSION_TLS_1_0
 #endif
   };

   BACKEND->data = data;
@pghmcfc

This comment has been minimized.

pghmcfc commented Dec 4, 2018

Yes, that's fixed the build on Fedoras 16 and 17.

@pghmcfc pghmcfc force-pushed the pghmcfc:nss-ssl-version-fallback branch from 54a6372 to 98e9843 Dec 4, 2018

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Dec 5, 2018

Perfect. Thank you for working on this!

@bagder

This comment has been minimized.

Member

bagder commented Dec 5, 2018

I'm puzzed by the DoH torture test failures on travis (test 2100) which indeed is not happening due to this change.

@bagder

This comment has been minimized.

Member

bagder commented Dec 5, 2018

(doh leak now worked on in #3342)

@bagder

This comment has been minimized.

Member

bagder commented Dec 5, 2018

Thanks a lot both of you!

@bagder

This comment has been minimized.

Member

bagder commented Dec 5, 2018

Landed in 6848ea5

@bagder bagder closed this Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment