Skip to content

Forcing TLS version allows to bypass the OpenSSL OS default #4304

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.

Sign up for GitHub

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
Closed
cnotin wants to merge 4 commits into from
Closed

Forcing TLS version allows to bypass the OpenSSL OS default #4304

cnotin wants to merge 4 commits into from

Conversation

@cnotin
Copy link
Contributor

@cnotin cnotin commented Sep 8, 2019

Fixes #4298

I changed a few stuffs but I tried to group every block in a single commit.
This is my first attempt to contribute to cURL, and I'm not a regular C developer, so I don't mind (and even expect) a thorough review :)

Here are a few tests:

# ./src/curl -s https://www.howsmyssl.com/a/check | jq '.tls_version'
"TLS 1.3"
# ./src/curl -s --tlsv1.0 https://www.howsmyssl.com/a/check | jq '.tls_version'
"TLS 1.3"

# ./src/curl -s --tlsv1.0 --tls-max 1.0 https://www.howsmyssl.com/a/check | jq '.tls_version'
"TLS 1.0"
# ./src/curl -s --tlsv1.0 --tls-max 1.1 https://www.howsmyssl.com/a/check | jq '.tls_version'
"TLS 1.1"
# ./src/curl -s --tlsv1.0 --tls-max 1.2 https://www.howsmyssl.com/a/check | jq '.tls_version'
"TLS 1.2"
# ./src/curl -s --tlsv1.0 --tls-max 1.3 https://www.howsmyssl.com/a/check | jq '.tls_version'
"TLS 1.3"

# ./src/curl -s --tlsv1.0 --tls-max 1.0 https://www.howsmyssl.com/a/check | jq '.tls_version'
"TLS 1.0"
# ./src/curl -s --tlsv1.1 --tls-max 1.1 https://www.howsmyssl.com/a/check | jq '.tls_version'
"TLS 1.1"
# ./src/curl -s --tlsv1.2 --tls-max 1.2 https://www.howsmyssl.com/a/check | jq '.tls_version'
"TLS 1.2"

And the fix of the original issue, with a server that only accepts TLS1.0 while the OpenSSL default on my system is TLS1.2:

  • Before:
# curl --tlsv1 https://tls-v1-0.badssl.com:1010/ 
curl: (35) error:1425F102:SSL routines:ssl_choose_client_version:unsupported protocol
  • After:
# ./src/curl -s --tlsv1 https://tls-v1-0.badssl.com:1010/  | head -n2
<!DOCTYPE html>
<html>

@cnotin
Copy link
Contributor Author

cnotin commented Sep 8, 2019

I also would suggest to replace the implementation of set_ssl_version_min_max_legacy by the one from Ruby's openssl:
https://github.com/ruby/openssl/blob/4b43ffc1292eeb70ff886847836e21ad96ed8796/ext/openssl/ossl_ssl.c#L169-L193
It is clearer and more systematic IMO, what do you think? Should I suggest something based on it too?

@cnotin
openssl: modernize set_ssl_version_min_max
16ec651 
OpenSSL 1.1.0 adds SSL_CTX_set_<min|max>_proto_version() that we now use when available.
Existing code is preserved for older versions of OpenSSL.
@cnotin
Copy link
Contributor Author

cnotin commented Sep 8, 2019 •
edited
Loading

Regarding #4097:

  • Before, it fails to go down to TLSv1.0:
# curl -v --tlsv1.0 --tls-max 1.0 https://www.cloudflare.com/robots.txt 
[...]
curl: (35) error:141E70BF:SSL routines:tls_construct_client_hello:no protocols available
# curl -V
curl 7.65.3 (x86_64-pc-linux-gnu) libcurl/7.65.3 OpenSSL/1.1.1c zlib/1.2.11 libidn2/2.2.0 libpsl/0.20.2 (+libidn2/2.0.5) libssh2/1.8.0 nghttp2/1.39.2 librtmp/2.3
Release-Date: 2019-07-19
[...]
  • After, it works:
# ./src/curl -v --tlsv1.0 --tls-max 1.0 https://www.cloudflare.com/robots.txt 2>&1 | grep -i "SSL connection\|User-Agent"
* SSL connection using TLSv1.0 / ECDHE-RSA-AES128-SHA
> User-Agent: curl/7.66.0-DEV
User-agent: *
# LD_LIBRARY_PATH=/root/tools/openssl/output/lib ./src/curl -V
curl 7.66.0-DEV (x86_64-pc-linux-gnu) libcurl/7.66.0-DEV OpenSSL/1.0.2t zlib/1.2.11 libidn2/2.2.0
Release-Date: [unreleased]
[...]

With TLSv1.2 as the system default:

# tail -n3 /etc/ssl/openssl.cnf 
[system_default_sect]
MinProtocol = TLSv1.2
CipherString = DEFAULT@SECLEVEL=2

@cnotin cnotin marked this pull request as ready for review September 8, 2019 16:54
@cnotin
Copy link
Contributor Author

cnotin commented Sep 8, 2019

And with an older (<1.1.0) version of OpenSSL which doesn't have SSL_CTX_set_<min|max>_proto_version():

# LD_LIBRARY_PATH=/root/tools/openssl/output/lib ./src/curl -V
curl 7.66.0-DEV (x86_64-pc-linux-gnu) libcurl/7.66.0-DEV OpenSSL/1.0.2t zlib/1.2.11 libidn2/2.2.0
[...]
# LD_LIBRARY_PATH=/root/tools/openssl/output/lib ./src/curl  -v --tlsv1.0 --tls-max 1.0 https://www.cloudflare.com/robots.txt 2>&1 | grep -i "SSL connection\|User-Agent"
* SSL connection using TLSv1.0 / ECDHE-RSA-AES128-SHA
> User-Agent: curl/7.66.0-DEV
User-agent: *
# LD_LIBRARY_PATH=/root/tools/openssl/output/lib ./src/curl  -v --tlsv1.0 https://www.cloudflare.com/robots.txt 2>&1 | grep -i "openssl\|SSL connection\|User-Agent"
* SSL connection using TLSv1.2 / ECDHE-ECDSA-AES128-GCM-SHA256
> User-Agent: curl/7.66.0-DEV
User-agent: *

@cnotin
Copy link
Contributor Author

cnotin commented Sep 8, 2019

Well looks like the build is failing... I see why in some cases, and I'll address it, but I'm not sure that this PR is responsible of all errors (some I don't even see where the error is)

@bagder
Copy link
Member

bagder commented Sep 9, 2019

The ngtcp2 build failure is totally not your fault.

The libressl build failure looks like it needs a better preprocessor check for when the API exists.

@cnotin
Copy link
Contributor Author

cnotin commented Sep 9, 2019

Thanks! I'll add a #ifdef around the whole function to prevent the "unused function" warning

@bagder bagder closed this in ffe34b7 Sep 10, 2019
@bagder
Copy link
Member

bagder commented Sep 10, 2019

Thanks a lot! I squashed them into two commits before merge (primarily because I wanted to do some minor edits and couldn't really figure out to which commit!).

@cnotin
Copy link
Contributor Author

cnotin commented Sep 10, 2019

You're welcome, thanks for your quick feedback and your overall work on cURL!

@cnotin cnotin mentioned this pull request Sep 10, 2019
Closed
@cnotin cnotin deleted the pr-ossl branch September 10, 2019 20:58
@cnotin cnotin mentioned this pull request Sep 10, 2019
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Dec 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Forcing TLS version should ensure to bypass the OpenSSL OS default

2 participants

@cnotin @bagder