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

Allow specific SSL/TLS versions to be disabled #208

Merged
merged 3 commits into from Nov 27, 2016
Merged

Conversation

@rtau
Copy link
Contributor

rtau commented Dec 9, 2015

Here is a patch to disable a specific SSL/TLS version to be used on server. The AcceptOnlyTls setting had also been changed to use the same mechanism to disable SSL versions 2 and 3.

@dnobori
Copy link
Member

dnobori commented May 26, 2016

Your great patch is much appreciated. We are considering to apply your patch into the SoftEther VPN main tree.

SoftEther VPN Patch Acceptance Policy:
http://www.softether.org/5-download/src/9.patch

You have two options which are described on the above policy.
Could you please choose either option 1 or 2, and specify it clearly on the reply?

@rtau
Copy link
Contributor Author

rtau commented May 28, 2016

I would opt for option 2. Thanks.

@blakesteel
Copy link

blakesteel commented Jul 30, 2016

I recommend instead of the #208 patch which adds a lot of unnecessary code which needs to be maintained and complicates the design, to instead simply disable all older protocols by default. More code means more potential for exploit. And, these older protocols should never be used by anyone, they are no longer safe.

Also, the SSL_OP_NO_SSLv3, SSL_OP_NO_TLSv1, SSL_OP_NO_TLSv1_1, SSL_OP_NO_TLSv1_2, SSL_OP_NO_DTLSv1, SSL_OP_NO_DTLSv1_2 options have been deprecated since OpenSSL 1.1.0 and should be replaced by SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version instead. Reference: https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_set_options.html

My proposed code change:

Do not modify the Cedar.c, Cedar.h or Network.h files. And, in Cedar/Connection.c and Cedar/Server.c, remove any code involving AcceptOnlyTls, it is no longer needed.

In the StartSSLEx method in src/Mayaqua/Network.c, remove the code for the AcceptOnlyTls setting completely and add the following. Note: 0 in the max proto setting means up to the highest version supported:

SSL_CTX_set_min_proto_version(ssl_ctx, TLS1_2_VERSION);
SSL_CTX_set_max_proto_version(ssl_ctx, 0);

if (sock->ServerMode)
{
    Unlock(openssl_lock);
    AddChainSslCertOnDirectory(ssl_ctx);
    Lock(openssl_lock);
}

If you would like, you could add a define somewhere in the header for providing the default minimum and maximum TLS versions and/or retrieve it via a user provided setting. But, I leave that up to you.

My code can be provided to both the commercial and GPL versions under Option 1. I hope this decision will help provide the needed security, privacy and safety to everyone equally.

Thanks.

Edit: I changed max to 0 so that we take advantage of the next protocol that is released after TLS 1.2 automatically.

@w1bsb w1bsb mentioned this pull request Nov 16, 2016
@dnobori dnobori merged commit 311ab9e into SoftEtherVPN:master Nov 27, 2016
@dnobori
Copy link
Member

dnobori commented Nov 27, 2016

Thank you so much for your contribution to enrich the SoftEther VPN source code.

Your patch has been merged on the main source-tree of SoftEther VPN.

As a token of our gratitude, your GitHub username has been added on the AUTHORS.TXT file and on the header of the related source file.
Please see: https://github.com/SoftEtherVPN/SoftEtherVPN/blob/master/AUTHORS.TXT

Thanks again for your contribution.

@paulmenzel
Copy link
Contributor

paulmenzel commented Jul 27, 2018

For the record, this change was reverted in commit 4df2eb4, but the reasons were not documented.

@rtau
Copy link
Contributor Author

rtau commented Jul 30, 2018

I remember there was another pull request doing something similar around that time but cannot dig out that from GitHub now..

For the commit mentioned, there was code referring to a structure called SslAcceptSettings which have several flags toggled by Tls_Disable1_[0-2], which could serve similar purpose.

Hope this help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.