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: fix max-tls to be 1.3/1.2 #3262

Merged
merged 2 commits into from Nov 14, 2018

Conversation

Projects
None yet
2 participants
@bagder
Member

bagder commented Nov 12, 2018

Uh, the default value that is!

Fixes #3261

@bagder bagder added the SSL/TLS label Nov 12, 2018

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Nov 12, 2018

Did you test it? If I read the code correctly, the proposed change is a no-op. libcurl on top of NSS defaults to NSS' default min/max versions of TLS.

There seems to be one place that needs to be updated for the --tlsv1 option to work with TLS 1.3 but it will have no effect on --tlsv1.2:

https://github.com/curl/curl/blob/42fd2350/lib/vtls/nss.c#L1642

@bagder

This comment has been minimized.

Member

bagder commented Nov 12, 2018

Did you test it?

Yeps. Before my patch: curl fails as #3261 describes. After patch: that command line works...

the proposed change is a no-op

I beg to differ. The non-patch version sets 1.0 to be the max version by default here:

SSL_LIBRARY_VERSION_TLS_1_0 /* max */

... so when we raise the minimum to 1.2 with the option, the call to SSL_VersionRangeSet will fail since it asks for minimum 1.2, maximum 1.0:

if(SSL_VersionRangeSet(model, &sslver) != SECSuccess)

If you have a better/more appropriate fix, I'll happily accept that! =)

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Nov 13, 2018

Sorry, I overlooked commit 2e5651a which had actually triggered this bug (allowing to set max < min). Your patch indeed fixes it!

The code I was talking about is actually dead code now and it can be safely removed:

--- a/lib/vtls/nss.c
+++ b/lib/vtls/nss.c
@@ -1638,17 +1638,6 @@ static CURLcode nss_load_ca_certificates(struct connectdata *conn,
 static CURLcode nss_sslver_from_curl(PRUint16 *nssver, long version)
 {
   switch(version) {
-  case CURL_SSLVERSION_TLSv1:
-    /* TODO: set sslver->max to SSL_LIBRARY_VERSION_TLS_1_3 once stable */
-#ifdef SSL_LIBRARY_VERSION_TLS_1_2
-    *nssver = SSL_LIBRARY_VERSION_TLS_1_2;
-#elif defined SSL_LIBRARY_VERSION_TLS_1_1
-    *nssver = SSL_LIBRARY_VERSION_TLS_1_1;
-#else
-    *nssver = SSL_LIBRARY_VERSION_TLS_1_0;
-#endif
-    return CURLE_OK;
-
   case CURL_SSLVERSION_SSLv2:
     *nssver = SSL_LIBRARY_VERSION_2;
     return CURLE_OK;
@@ -1709,10 +1698,8 @@ static CURLcode nss_init_sslver(SSLVersionRange *sslver,
   }

   switch(min) {
-  case CURL_SSLVERSION_DEFAULT:
-    break;
   case CURL_SSLVERSION_TLSv1:
-    sslver->min = SSL_LIBRARY_VERSION_TLS_1_0;
+  case CURL_SSLVERSION_DEFAULT:
     break;
   default:
     result = nss_sslver_from_curl(&sslver->min, min);

bagder and others added some commits Nov 12, 2018

@bagder bagder force-pushed the bagder/nss-max-tls branch from a629c8a to 3d988c5 Nov 13, 2018

@bagder

This comment has been minimized.

Member

bagder commented Nov 13, 2018

Thanks @kdudka, rebased and committed your patch in the branch.

@kdudka

kdudka approved these changes Nov 14, 2018

Looks good!

@bagder bagder merged commit 3d988c5 into master Nov 14, 2018

7 of 8 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
LGTM analysis: C/C++ No alert changes
Details
LGTM analysis: Python No code changes detected
Details
buildbot/curl-unthreaded-solaris10-i386 Build done.
Details
buildbot/curl-unthreaded-solaris10-sparc Build done.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 74.861%
Details

@bagder bagder deleted the bagder/nss-max-tls branch Nov 14, 2018

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