LDAPS vulnerable to MITM - failure to validate hostname against CN or SAN in X509 Cert

[JPvRiel]

There are noted TLS/SSL limitations in the documented parts of the code and the info about encryption. Technically, TLS/SSL also provides the ability to authenticate servers by matching the hostname to a CN (common name) or a (SAN) Subject Alternative Name and from what I've seen, while net-ldap notes the certificate and trust chain limitations as insecure default, but it doesn't note the extra step needed to get to proper LDAP server authentication via LDAPS.

In order to verify certificates and enable other TLS options, the
  #   :tls_options hash can be passed alongside :simple_tls or :start_tls.
  #   This hash contains any options that can be passed to
  #   OpenSSL::SSL::SSLContext#set_params(). The most common options passed
  #   should be OpenSSL::SSL::SSLContext::DEFAULT_PARAMS, or the :ca_file option,
  #   which contains a path to a Certificate Authority file (PEM-encoded).

The above advice is not sufficient. It only assures that a server has a valid cert which was signed by a trusted CA, but it does not provide proof that it's the correct server E.g. evil.example.net with a cert issued as CN=evil.example.net could still impersonate ldap.example.net to steal credentials.

In addition to the above suggtestion setting a more secure and sane TLS/SSL context that validates certs , the net-ldap lib should also, probably by default, add and make use of verify_certificate_identity. I've browsed connection.rb and don't see anything that checks the hostname (FQDN) is authenticated against the certficate.

  def self.wrap_with_ssl(io, tls_options = {})
    raise Net::LDAP::NoOpenSSLError, "OpenSSL is unavailable" unless Net::LDAP::HasOpenSSL

    ctx = OpenSSL::SSL::SSLContext.new

    # By default, we do not verify certificates. For a 1.0 release, this should probably be changed at some point.
    # See discussion in https://github.com/ruby-ldap/ruby-net-ldap/pull/161
    ctx.set_params(tls_options) unless tls_options.empty?

    conn = OpenSSL::SSL::SSLSocket.new(io, ctx)
    conn.connect

    # Doesn't work:
    # conn.sync_close = true

    conn.extend(GetbyteForSSLSocket) unless conn.respond_to?(:getbyte)
    conn.extend(FixSSLSocketSyncClose)

    conn
  end 

Given ruby essentially 'wraps' OpenSSL, this Hostname_validation reference also highlights the issue, with a choice extract

One common mistake made by users of OpenSSL is to assume that OpenSSL will validate the hostname in the server's certificate. Versions prior to 1.0.2 did not perform hostname validation. Version 1.0.2 and up contain support for hostname validation, but they still require the user to call a few functions to set it up.

[jch]

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.

[etdsoft]

@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

[JPvRiel]

@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

[JPvRiel]

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).

[meastman]

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

[JPvRiel]

@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...

[meastman]

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.

[JPvRiel]

@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.

[JPvRiel]

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...).

[JPvRiel]

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?

[meastman]

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.

[jch]

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

[jch]

@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.

[jch]

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.

[carnil]

This issue has been assigned CVE-2017-17718.

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