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.

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

OpenSslEngine.setEnabledProtocols fails to enable protocols that are currently disabled #4736

Closed
blucas opened this issue Jan 20, 2016 · 6 comments
Closed

OpenSslEngine.setEnabledProtocols fails to enable protocols that are currently disabled #4736

blucas opened this issue Jan 20, 2016 · 6 comments
Assignees
@Scottmitch
Labels
defect
Milestone
4.0.34.Final

Comments

@blucas
Copy link
Contributor

blucas commented Jan 20, 2016

Netty Version: master (I know... 😦 )

The OpenSslEngine.setEnabledProtocols() will not allow the user to enable a protocol that has already been disabled. For example, netty (by default) disables SSLv3. If you wish to enable SSLv3 you should be able to call OpenSslEngine.setEnabledProtocols() including SSLv3 as one of the parameters of the array. Unfortunately this does not work.

The reason has to do with the implementation of the method.

It does the following:

  1. Determine which protocols to disable
  2. Enable all protocols
  3. Disable each protocol from step 1 above.

The problem is step 2 above does not work. I have no idea what SSL.SSL_OP_ALL actually does, but it doesn't do what the comment expects.

Solution:
Expose SSL_clear_options to netty via a new native method clearOptions() then use clearOptions in setEnabledProtocols

Pull Requests to follow. Will need one for netty-tcnative and netty.
@blucas blucas mentioned this issue Jan 20, 2016
Closed
@Scottmitch
Copy link
Member

https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_set_options.html

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
These options turn off the SSLv3, TLSv1, TLSv1.1 or TLSv1.2 protocol versions with TLS or the DTLSv1, DTLSv1.2 versions with DTLS, respectively. As of OpenSSL 1.1.0, these options are deprecated, use SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version instead.

Looks like we should investigate using SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version instead. Mixed up the versions ... these new interfaces are only available in 1.1.0 ... so I guess we will not use this yet.

@Scottmitch Scottmitch self-assigned this Jan 20, 2016
@Scottmitch
Copy link
Member

@blucas - Your approach as described sounds reasonable.

@Scottmitch
Copy link
Member

@normanmaurer - On a related note I wonder if we should be more careful about SSL_OP_ALL. See https://github.com/bagder/curl/blob/master/lib/vtls/openssl.c#L1730 and http://daniel.haxx.se/blog/2012/01/27/sloppily-using-ssl_op_all/.

Scottmitch pushed a commit to netty/netty-tcnative that referenced this issue Jan 20, 2016
@Scottmitch
Add method to clear options set by SSL_set_options
4edc0db 
Motivation:

We currently provide a way to set SSL options via SSL_set_options but we do not provide a way to clear out those options. Related to netty/netty#4736

Modifications:

Add clearOptions function which executes SSL_clear_options

Result:

Allow users to clear out options which were set using SSL_set_options
@Scottmitch Scottmitch added this to the 4.0.34.Final milestone Jan 20, 2016
@blucas blucas mentioned this issue Jan 20, 2016
Closed
@blucas
Copy link
Contributor Author

blucas commented Jan 20, 2016

@Scottmitch - On another related note (but outside the scope of this issue), it might be worth adding a setEnabledProtocols method to SslContextBuilder so that users do not have to call that method on the SslEngine implementation directly.

@Scottmitch
Copy link
Member

it might be worth adding a setEnabledProtocols method to SslContextBuilder

sgtm

@johnou johnou mentioned this issue Jan 22, 2016
Closed
normanmaurer pushed a commit that referenced this issue Jan 22, 2016
@normanmaurer
Clear disabled SSL protocols before enabling provided SSL protocols
7090d13 
Motivation:

Attempts to enable SSL protocols which are currently disabled fail when using the OpenSslEngine. Related to #4736

Modifications:

Clear out all options that have disabled SSL protocols before attempting to enable any SSL protocol.

Result:

setEnabledProtocols works as expected.
normanmaurer pushed a commit that referenced this issue Jan 22, 2016
@normanmaurer
Clear disabled SSL protocols before enabling provided SSL protocols
357720e 
Motivation:

Attempts to enable SSL protocols which are currently disabled fail when using the OpenSslEngine. Related to #4736

Modifications:

Clear out all options that have disabled SSL protocols before attempting to enable any SSL protocol.

Result:

setEnabledProtocols works as expected.
@normanmaurer
Copy link
Member

Fixed by #4737

pulllock pushed a commit to pulllock/netty that referenced this issue Oct 19, 2023
@normanmaurer
Clear disabled SSL protocols before enabling provided SSL protocols
5c29207 
Motivation:

Attempts to enable SSL protocols which are currently disabled fail when using the OpenSslEngine. Related to netty#4736

Modifications:

Clear out all options that have disabled SSL protocols before attempting to enable any SSL protocol.

Result:

setEnabledProtocols works as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Scottmitch Scottmitch

Labels
defect
Projects
None yet
Milestone
4.0.34.Final
Development

No branches or pull requests
3 participants
@normanmaurer @blucas @Scottmitch