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

Jump to bottom

nss: fix max-tls to be 1.3/1.2 #3262

Merged
merged 2 commits into from Nov 14, 2018
Merged

nss: fix max-tls to be 1.3/1.2 #3262

merged 2 commits into from Nov 14, 2018

Conversation

bagder
Copy link
Member

@bagder bagder commented Nov 12, 2018 •
edited

Uh, the default value that is!

Fixes #3261

@bagder bagder added the TLS label Nov 12, 2018
@kdudka
Copy link
Contributor

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

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:

curl/lib/vtls/nss.c

Line 1795 in 42fd235

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:

curl/lib/vtls/nss.c

Line 1844 in 42fd235

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

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

@kdudka
Copy link
Contributor

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

bagder commented Nov 13, 2018

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

kdudka
kdudka approved these changes Nov 14, 2018
View reviewed changes
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.

Looks good!

@bagder bagder merged commit 3d988c5 into master Nov 14, 2018
@bagder bagder deleted the bagder/nss-max-tls branch November 14, 2018 09:36
@lock lock bot locked as resolved and limited conversation to collaborators Feb 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kdudka kdudka kdudka approved these changes

Assignees
No one assigned
Labels
TLS
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bagder @kdudka