7
\$\begingroup\$

I use Gem in a Box, a Ruby project that allows to create personal self-hosted gem repositories. Gem in a Box uses httpclient to connect to the gem repository. After reviewing my server's TLS setup in the aftermath of the OpenSSL heartbleed bug, I decided only to allow protocol versions of TLSv1 and higher on my server. A decision quite a few fellow administrators made too, because of security vulnerabilities in SSLv3 and below.

Now after I did this change, Gem in a Box refused to work with the following message:

OpenSSL::SSL::SSLError: SSL_connect returned=1 errno=0 state=SSLv3 read server hello A: sslv3 alert handshake failure

I traced this problem back until I found the source in httpclient. Even though OpenSSL can find the best protocol version to use by itself, you set it only to use SSLv3 by default. I think this is a very bad idea and created a patch which lets OpenSSL do its own decision again by default.

As I am not so familiar with OpenSSL itself and the documentation of the Ruby binding is severely lacking I want to prevent introducing more problems than there were originally. Please review my changes and verify that it is doing what I intended.

It is quite hard to post the actual changes here because they are only meaningful if one knows the context. So now I will try to describe what I did:

httpclient has a class called SSLConfig which seems to produce helper objects to configure OpenSSL sockets. It sets some default configurations in its #initialize method. Originally it had the @ssl_version instance variable set to "SSLv3", When using the library an instance of SSLConfig is created and its #set_context method is called which applies the aforementioned defaults onto an instance of OpenSSL::SSL::SSLContext, which afterwards seems to be used to create OpenSSL sockets.

I now changed to initial value of @ssl_version to the symbol :auto and modified the #set_context method to only set the actual value of @ssl_version to the SSLContext object, if its value is not :auto.

Manual testing so far resulted in the exact effect I wanted to achieve.

Here are some relevant snippets from lib/httpclient/ssl_config.rb:

class SSLConfig

  ...

  # Which TLS protocol version (also called method) will be used. Defaults
  # to :auto which means that OpenSSL decides (In my tests this resulted 
  # with always the highest available protocol being used).
  # 
  # See the content of OpenSSL::SSL::SSLContext::METHODS for a list of
  # available versions in your specific Ruby environment.
  attr_reader :ssl_version

  ...

  # Creates a SSLConfig.
  def initialize(client)
    return unless SSLEnabled
    @client = client
    @cert_store = X509::Store.new
    @client_cert = @client_key = @client_ca = nil
    @verify_mode = SSL::VERIFY_PEER | SSL::VERIFY_FAIL_IF_NO_PEER_CERT
    @verify_depth = nil
    @verify_callback = nil
    @dest = nil
    @timeout = nil
    @ssl_version = :auto
    @options = defined?(SSL::OP_ALL) ? SSL::OP_ALL | SSL::OP_NO_SSLv2 : nil
    # OpenSSL 0.9.8 default: "ALL:!ADH:!LOW:!EXP:!MD5:+SSLv2:@STRENGTH"
    @ciphers = "ALL:!aNULL:!eNULL:!SSLv2" # OpenSSL >1.0.0 default
    @cacerts_loaded = false
  end

  ...

  # interfaces for SSLSocketWrap.
  def set_context(ctx) # :nodoc:
    load_trust_ca unless @cacerts_loaded
    @cacerts_loaded = true
    # Verification: Use Store#verify_callback instead of SSLContext#verify*?
    ctx.cert_store = @cert_store
    ctx.verify_mode = @verify_mode
    ctx.verify_depth = @verify_depth if @verify_depth
    ctx.verify_callback = @verify_callback || method(:default_verify_callback)
    # SSL config
    ctx.cert = @client_cert
    ctx.key = @client_key
    ctx.client_ca = @client_ca
    ctx.timeout = @timeout
    ctx.options = @options
    ctx.ciphers = @ciphers
    ctx.ssl_version = @ssl_version unless @ssl_version == :auto
  end

  ...

end

Here's what is using the #set_context method in lib/httpclient/session.rb:

...

def create_openssl_socket(socket)
  ssl_socket = nil
  if OpenSSL::SSL.const_defined?("SSLContext")
    ctx = OpenSSL::SSL::SSLContext.new
    @context.set_context(ctx)
    ssl_socket = OpenSSL::SSL::SSLSocket.new(socket, ctx)
  else
    ssl_socket = OpenSSL::SSL::SSLSocket.new(socket)
    @context.set_context(ssl_socket)
  end
  ssl_socket
end

...
\$\endgroup\$
2
\$\begingroup\$

I've got a few things to say about the code, but having looked at the diff:

-      @ssl_version = "SSLv3"
+      @ssl_version = :auto

and:

-      ctx.ssl_version = @ssl_version
+      ctx.ssl_version = @ssl_version unless @ssl_version == :auto

your code changes seem minimal, and to the point doing exactly what they are supposed to do, and nothing more.

What I think you should add are the tests to show your changes, and make sure no-one will break your code in the future.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.