Addresses the LDAPS vulnerability to MITM attacks by properly authenticating the hostname

[JPvRiel]

This attempts to address LDAPS vulnerable to MITM - failure to validate hostname against CN or SAN in X509 Cert.

It has been briefly tested and when connecting or binding via LDAP against a , the following exception occurs

Net::LDAP::Error: hostname "<missmatched hostname>" does not match the server certificate

Users who want TLS/SSL just for encryption and don't care about hostname validation should be able to use :tls_options => { :verify_mode => OpenSSL::SSL::VERIFY_NONE } to allow for insecure connections.

[etdsoft]

One thing, in case it makes it to the official docs.

Users who want TLS/SSL just for encryption and don't care about hostname validation should be able to use :tls_options => { :verify_mode => OpenSSL::SSL::VERIFY_NONE } to allow for insecure connections.

I believe that users who want TLS/SSL just for encryption don't have to do anything as in Ruby, the default OpenSSL params include VERIFY_NONE (which is part of the problem on the first place).

[JPvRiel]

I think I failed to check/regression test if the patch breaks non-SSL connections, so I'll need to re-submit. ```
NoMethodError: undefined method[]' for nil:NilClass /home/travis/build/ruby-ldap/ruby-net-ldap/lib/net/ldap/connection.rb:54:in block in open_connection'

[JPvRiel]

@etdsoft

I believe that users who want TLS/SSL just for encryption don't have to do anything as in Ruby, the >default OpenSSL params include VERIFY_NONE (which is part of the problem on the first place).

ruby 1.9 & 2.x has insecure SSL/TLS client defaults and Changed default settings of ext/openssl suggests newer ruby versions are more secure by default, so the pull request/patch could cause net-ldap users to bump into issues if they previously didn't bother or want to validate the sever cert properly, but that's a ruby-wide SSL default issue and the potential 'backward compatibiltiy' bug reliant on insecure defaults will impact the whole ruby community, not just net-ldap. So allowing users to explicitly set VERIFY_NONE provides a fallback option.

Example below shows new versions have VERIFY_PEER by default.

irb(main):025:0* "#{RUBY_VERSION}-p#{RUBY_PATCHLEVEL}"
=> "2.1.5-p273"
irb(main):026:0> OpenSSL::SSL::SSLContext::DEFAULT_PARAMS
=> {:ssl_version=>"SSLv23", :verify_mode=>1, :ciphers=>"ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-DSS-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:DHE-DSS-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES256-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA:DHE-DSS-AES128-SHA256:DHE-DSS-AES256-SHA256:DHE-DSS-AES128-SHA:DHE-DSS-AES256-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:ECDHE-ECDSA-RC4-SHA:ECDHE-RSA-RC4-SHA:RC4-SHA", :options=>-2097019905}
irb(main):004:0> OpenSSL::SSL::VERIFY_PEER
=> 1

[JPvRiel]

#262 pull request that aims to address the same issue.

[JPvRiel]

Okay, found some time to test this more extensively.

I noticed the following is causing the CI build tests to fail for this pull due to:

Error: test_bind_tls_with_cafile(TestBindIntegration): Net::LDAP::Error: hostname "localhost" does not match the server certificate

which happens because rubyldap != localhost:

$ openssl x509 -in "ruby-net-ldap-fix1/test/fixtures/cacert.pem" -subject -noout
subject= /CN=rubyldap

Wrote test-ldaps.rb to run a number of positive and negative test cases for our org's use case (load balanced LDAPS with Active Directory).

Positive Testing

When correct hostname is used

test minimal config secure ldaps (single host)
  connection and bind succeeded

test minimal config secure ldaps (multiple hosts)
  connection and bind succeeded

test explicit config secure ldaps (single host)
  connection and bind succeeded

test explicit config secure ldaps (multiple host)
  connection and bind succeeded

test disable certifcate validation for secure ldaps (single host)
not verifying SSL hostname of LDAPS server
  connection and bind succeeded

test disable certifcate validation for secure ldaps (multiple hosts)
not verifying SSL hostname of LDAPS server
  connection and bind succeeded

NegativeTesting - hostname missmatch

When intentionally incorrect hostname is used

test minimal config secure ldaps (single host)
  connection or bind failed due to exception
  Exception: hostname "ldap.example.com" does not match the server certificate

test minimal config secure ldaps (multiple hosts)
  connection or bind failed due to exception
  Exception: hostname "ldap.example.com" does not match the server certificate

test explicit config secure ldaps (single host)
  connection or bind failed due to exception
  Exception: hostname "ldap.example.com" does not match the server certificate

test explicit config secure ldaps (multiple host)
  connection or bind failed due to exception
  Exception: hostname "ldap.example.com" does not match the server certificate

test disable certifcate validation for secure ldaps (single host)
not verifying SSL hostname of LDAPS server
  connection and bind succeeded

test disable certifcate validation for secure ldaps (multiple hosts)
not verifying SSL hostname of LDAPS server
  connection and bind succeeded

NegativeTesting - cert not valid

When intentionally incorrect .pem file is used

test minimal config secure ldaps (single host)
  connection and bind succeeded

test minimal config secure ldaps (multiple hosts)
  connection and bind succeeded

test explicit config secure ldaps (single host)
  connection or bind failed due to exception
  Exception: SSL_connect returned=1 errno=0 state=error: certificate verify failed

test explicit config secure ldaps (multiple host)
  connection or bind failed due to exception
  Exception: SSL_connect returned=1 errno=0 state=error: certificate verify failed

test disable certifcate validation for secure ldaps (single host)
not verifying SSL hostname of LDAPS server
  connection and bind succeeded

test disable certifcate validation for secure ldaps (multiple hosts)
not verifying SSL hostname of LDAPS server
  connection and bind succeeded

Regression test LDAP (non SSL/TLS) connection and bind

Make sure code changes don't break how the connection for plain LDAP was handled

test plain insecure ldap (single host)
  connection and bind succeeded

test plain insecure ldap (multiple hosts)
  connection and bind succeeded

[jch]

@JPvRiel what was your reasoning for putting this check in #open_connection rather than .wrap_with_ssl? It looks like that code path is always used when setting up SSL connections.

[etdsoft]

I think it's just a matter of not knowing much about the internals of the lib, we just found the issue while doing some SSL verification. Feel free to move it where ever is the right place for this code (you know the code better than us), we're just trying to help out on the SSL-verification case. Makes sense?

[JPvRiel]

@jch, I also wanted to put the code into .wrap_with_ssl initially. However, the current .wrap_with_ssl function doesn't have access to any variable with the FQDN/hostname used to make the connection in order to pass that to post_connection_check. There was another PR that attempted address this passing around the extra host info by adding an extra host parameter for the .wrap_with_ssl function.

See: #262 (comment). However, that PR had a few other unresolved questions about how it handled default validation...

One of the reasons I didn't go that way is I noticed the connection.rb code is flagged for some major refactoring anyhow, and yeah, as above, you'd be in a better place to make a call where best to inject post_connection_check.

[jch]

Good point. I forgot that it's a class method.

[jch]

@JPvRiel thanks for opening this PR and also including a test manifest. Could you look into converting your test script into an integration test? .travis.yml runs an openldap install and sets the INTEGRATION environment variable for tests that live under test/integration. The install currently does not configure tls, so would need to be updated to support that.

[JPvRiel]

@jch to add that into your test code, I'd have to learn the ropes with travis and test/integration. I suppose it's a nice task to learn, but that's going to take me much longer than someone who's already familiar with the projects test harness. Hence I've made my test example available https://github.com/JPvRiel/test-ruby-ldaps hoping it'll be easy to port by a dev already familiar with the ruby-net-ldap test suite...

[jch]

@JPvRiel sorry for the slow feedback cycle. I still haven't had time to work on this, and don't see that changing soon. I'm going to leave this PR open so people can find it easily and apply the patch to their local install as needed.

If anyone is interested in working on this, please let me know and I can help walk you through it.

[tmaher]

Resolved in #279 - @JPvRiel, give a yell if I missed anything.