Thanks for reporting this. If you have a proposal for documentation or code changes, I'd be happy to review them. I'll take a deeper look when I have some free time.

@jch I've worked with @JPvRiel a little bit on this today. It seems that OpenSSL offers the opportunity to do this through the #post_connection_check method (source)

I'd say something similar to this would work:

      if @ssl_context.verify_mode != OpenSSL::SSL::VERIFY_NONE
        s.post_connection_check(@address)
      end

In connection.rb straight after opening the socket.

Of course adjusting the context and socket name (s for conn). The code is from Ruby's own SMTP implementation at net/smtp.rb. By default it won't break as VERIFY_NONE is the default that someone might use and the call would be avoided, but once you set a verify mode other than that, the check will kick in.

HTH

@etdsoft, thanks, yep post_connection_check is better as it calls verify_certificate_identity without needing to pass the cert.

Tried hack in a patch (my first go at ruby monkey patching). But self.wrap_with_ssl is way down the stack after several preceding function calls that setup sockets, etc, so it's unclear how to self.wrap_with_ssl can compare the hostname given the following server structure used to drive things which includes multiple LDAP hosts?

  # :server
  #   :hosts   Array of tuples specifying host, port
  #   :host    host
  #   :port    port
  #   :socket  prepared socket

@server[:host] would in theory have the host (should be fqdn if using LDAPS) to compare, e.g:

# Monkey patch to test with
module Net
  class LDAP
    class Connection

      def prepare_socket(server)
        socket = server[:socket]
        encryption = server[:encryption]

        @conn = socket
        if encryption
          setup_encryption encryption
          @conn.post_connection_check(server[:host])
        end

      end

    end
  end
end

With the above, it sort of works as I get the following if the hostname doesn't match

irb(main):148:0* conn.bind
Net::LDAP::Error: hostname "<mismatched hostname>" does not match the server certificate

However, in def open_connection(server), we could be opening multiple connections! :-/ So my hack above probably doesn't cut it for the case when multiple hosts are defined, given it's not clear which socket belongs to which hostname by the time the self.wrap_with_ssl or the prepare_socket(server) funciton is called... I don't know of a simple way to get the hostname for a socket object...

def open_connection(server)
    hosts = server[:hosts]
    encryption = server[:encryption]

    socket_opts = {
      connect_timeout: server[:connect_timeout] || DefaultConnectTimeout,
    }

    errors = []
    hosts.each do |host, port|
      begin
        prepare_socket(server.merge(socket: @socket_class.new(host, port, socket_opts)))
        return
      rescue Net::LDAP::Error, SocketError, SystemCallError,
             OpenSSL::SSL::SSLError => e
        # Ensure the connection is closed in the event a setup failure.
        close
        errors << [e, host, port]
      end
    end

    raise Net::LDAP::ConnectionError.new(errors)
  end

Tried hosts, and it breaks with the monkey patch in the previous comment. Here's the result in irb:

> multiconn = Net::LDAP.new :hosts      => [['<fdqn 1>', 636], ['<fqdn 2>', 636]],
                     :base       => '<base DN>',
                     :encryption => { :method => :simple_tls, :tls_options => { :verify_mode => OpenSSL::SSL::VERIFY_PEER, :ca_file => 'ca_bundle.pem' } },
                     :auth => { :username => '<bind user>',
                                :password => '<bind password>]',
                                :method => :simple }
> multconn.bind
Net::LDAP::ConnectionError: Unable to connect to any given server: 
  OpenSSL::SSL::SSLError: hostname "127.0.0.1" does not match the server certificate (<fdqn 1>:636)
  OpenSSL::SSL::SSLError: hostname "127.0.0.1" does not match the server certificate (<fdqn 2>:636)

Not sure why 127.0.0.1 is used (seems to be falling back to a default).

Not sure why 127.0.0.1 is used (seems to be falling back to a default).

That appears to be what's used when :host isn't provided.
https://github.com/ruby-ldap/ruby-net-ldap/blob/master/lib/net/ldap.rb

class Net::LDAP
  DefaultHost = "127.0.0.1"
  def initialize(args = {})
    @host = args[:host] || DefaultHost

@meastman, thanks, yeah that confirms my concern that adding @conn.post_connection_check(server[:host]) in prepare_socket(server) wouldn't work for the multiple 'hosts' use case. So at this point, I'm clueless as to where better the code should do the post_connection_check...

One option might be to update this line:
https://github.com/ruby-ldap/ruby-net-ldap/blob/master/lib/net/ldap/connection.rb#L53

prepare_socket(server.merge(socket: @socket_class.new(host, port, socket_opts)))

to also include the hostname as :host in the new server hash.

@meastman, thanks for the hint. I simply did the check after prepare_socket(server.merge(socket: @socket_class.new(host, port, socket_opts))). Please check if my pull request makes sense.

It seemed to work for both single host and hosts use cases. But I lack the time to dig into your unit testing add proper negative test cases that double check this.

Being unfamilair with ruby, and not a regualar programmer, I don't have the time/knowledge to wrap my head around using ruby test suites test_ssl_ber.rb and test_ldap_connection.rb to help add a proper test-suite. Nevertheless, :encryption config could benefit from negative and positive test cases for SSL/TLS. test-ldaps.rb is a command line util can help with testing against servers in one's own envrionement (assums standard ports 389 and 636 are in use - sorry, I hardcoded those...).

Sorry to bump this issue. It's 2 months since it was raised, and there is a pull request to fix it. Anything else from the community needed to get this fixed?

Sorry to bump this issue. It's 2 months since it was raised, and there is a pull request to fix it. Anything else from the community needed to get this fixed?

@JPvRiel sorry about that - I thought that #262 superseded #259 but wasn't ready yet. I missed that #262 got closed in favor of #259. I'm no longer involved with this project, but hopefully @jch can take a look at some point.

@JPvRiel thanks for the reminder, I'll bump the issue internally as well.

@JPvRiel quick update here. post_connection_check used in #259 was removed in ruby >= 2.0.0, which is the minimum requirement for the net-ldap gem (1.9.x support was dropped). I'm investigating other certificate verification that may need to be done as well, and how to approach that for ruby 2.x.

I spoke too soon about post_connection_check. It is available in 2.x, and looks to be the right approach. I'll leave my feedback on the PR.

This issue has been assigned CVE-2017-17718.

Cf. http://openwall.com/lists/oss-security/2017/12/17/10