Skip to content
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 from

Conversation

pghmcfc
Copy link
Contributor

@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
Copy link
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 &&
     ^

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 curl#3261
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

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

@pghmcfc
Copy link
Contributor Author

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
Copy link
Contributor

kdudka commented Dec 4, 2018

@pghmcfc Sounds good!

@pghmcfc
Copy link
Contributor Author

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>

Copy link
Contributor

@kdudka kdudka left a comment

Choose a reason for hiding this comment

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

Besides the style issues, it looks fine to me.

lib/vtls/nss.c Outdated Show resolved Hide resolved
lib/vtls/nss.c Outdated Show resolved Hide resolved
Use descriptive text strings rather than decimal numbers.
@kdudka
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

pghmcfc commented Dec 4, 2018

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

@kdudka
Copy link
Contributor

kdudka commented Dec 5, 2018

Perfect. Thank you for working on this!

@bagder
Copy link
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
Copy link
Member

bagder commented Dec 5, 2018

(doh leak now worked on in #3342)

@bagder
Copy link
Member

bagder commented Dec 5, 2018

Thanks a lot both of you!

@bagder
Copy link
Member

bagder commented Dec 5, 2018

Landed in 6848ea5

@bagder bagder closed this Dec 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Mar 5, 2019
@pghmcfc pghmcfc deleted the nss-ssl-version-fallback branch March 11, 2020 13:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants