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.

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

SSL hostname verification support #262

Closed
wants to merge 1 commit into from

Conversation

@tarcieri
Copy link

tarcieri commented Jan 23, 2016

The current Net::LDAP::Connection::wrap_with_ssl verifies the certification paths of SSL certificates correctly, but it does not verify hostnames. This means any service with a certificate issued by the same CA as the LDAP server can impersonate the LDAP server. (Note: this is probably CVE worthy. Let me know if you'd like help getting a CVE assigned)

This PR is more of a strawman implementation than anything to get the ball rolling towards a solution. I think it could definitely use some tests to ensure hostname verification works! But to do that we might want to talk about how to issue all of the certificates needed in the tests.

No tests yet, just an initial implementation to discuss.
@tarcieri

This comment has been minimized.

Copy link
Author

tarcieri commented Jan 23, 2016

I tried to write this so it's possible to pass tls_options[:verify_host] = false although it is not the default. I am guessing there are people who do not have the correct subjectAltNames for the SSL certificates on their LDAP servers, and this patch if done correctly will make that break.

I made the code warn every time a connection is made without verifying hostnames, so hopefully this provides a nice tradeoff between being able to turn the security off while still being sufficiently annoying if you do.

@@ -88,6 +88,13 @@ def self.wrap_with_ssl(io, tls_options = {})
conn = OpenSSL::SSL::SSLSocket.new(io, ctx)
conn.connect

if tls_options[:verify_host]
# This raises OpenSSL::SSL::SSLError if hostname verification fails
conn.post_connection_check(tls_options[:verify_host])

This comment has been minimized.

Copy link
@tarcieri

tarcieri Jan 23, 2016

Author

A previously undocumented (until I documented it) method it is very important to call if you want your SSL connections to be secure!

@JPvRiel

This comment has been minimized.

Copy link

JPvRiel commented Jan 26, 2016

So this is a second pull request to address this same issue #258. My initial hack was pull #259.

This pull request looks neater and more funcitonal than #259 in terms of parsing the hostname info through the function calls and doing the validation according to the :verify_host option. However, arguably, we should aim for secure by default (which #259 did). May I suggest the following logic:

  • when using LDAPS, verify_host (and validate the cert chain) by default (and in newer versions of ruby VERIFY_PEER is at least also a default)
  • have a :allow_host_missmatch negative option instead of :verify_host for people who want or need to disable it, or tls_options[:verify_host] = true can be set as a default if omitted?
  • also, don't bother to try verify (match) the hostname if users have forced :verify_mode => OpenSSL::SSL::VERIFY_NONE and the issue that warning. This will also imply it's backward compatible and insecure by default when run with older versions of ruby that defaulted to VERIFY_NONE.

Users could force old ruby behaviour with, but arguably they should just use plain LDAP / 389 instead then.

:tls_options => { :verify_mode => OpenSSL::SSL::VERIFY_NONE }
@@ -36,7 +36,7 @@ def prepare_socket(server)
encryption = server[:encryption]

@conn = socket
setup_encryption encryption if encryption
setup_encryption({ verify_host: server[:connected_host] }.merge(encryption)) if encryption

This comment has been minimized.

Copy link
@tarcieri

tarcieri Jan 26, 2016

Author

Note that this does verification by default unless explicitly disabled

This comment has been minimized.

Copy link
@JPvRiel

JPvRiel Jan 26, 2016

@tarcieri, sorry I'm not really much of a ruby dev, so I may misunderstand a bit. I assume any non nil value in a conditional returns false... so users must explicitly enable verification in config with :encryption = { :method => :simple_tls, :tls_options => { :verify_host => true } }?

If I understand correctly, :encryption => { :method => :simple_tls, :tls_options => { :verify_mode => OpenSSL::SSL::VERIFY_PEER, :tls_options[:verify_host] => false} } should imply checking that the cert is at least signed by a trusted CA, but allowing for a mismatch?

What happens if the user has simply put { :method => :simple_tls, :tls_options => { :verify_mode => OpenSSL::SSL::VERIFY_PEER }?

    if tls_options[:verify_host]
       # This raises OpenSSL::SSL::SSLError if hostname verification fails
       conn.post_connection_check(tls_options[:verify_host])

The match function will not run if tls_options[:verify_host] is nil? Hence I thought that your pull doesn't do hostname validation unless the user explicitly configures it.

This comment has been minimized.

Copy link
@tarcieri

tarcieri Jan 26, 2016

Author

No, this is passing the hostname through as verify_host, unless the user has set verify_host to something else.

It's a bit convoluted, but it's on by default, and enables the user to opt-out.

I would agree having a separate user targeted flag would make sense, but you don't seem to be understanding how this code is working.

This comment has been minimized.

Copy link
@JPvRiel

JPvRiel Jan 27, 2016

@tarcieri, thanks, got it now. I cloned your fork and tested :verify_host => false (with valid variables set for my env). It bumps into an error.

Can you provide some config examples?

  conn = Net::LDAP.new :host => hostname,
                       :port => 636,
                       :base => base,
                       :encryption => { :method => :simple_tls,
                                        :tls_options => { :verify_mode => OpenSSL::SSL::VERIFY_PEER,
                                                          :verify_host => false,
                                                          :ca_file => ca_file } },
                       :auth => { :username => login,
                                  :password => pass,
                                  :method => :simple }
  conn.bind 
/usr/lib/ruby/2.1.0/openssl/ssl.rb:87:in `block in set_params': undefined method `verify_host=' for #<OpenSSL::SSL::SSLContext:0x000000016f4130> (NoMethodError)
    from /usr/lib/ruby/2.1.0/openssl/ssl.rb:87:in `each'
    from /usr/lib/ruby/2.1.0/openssl/ssl.rb:87:in `set_params'
    from /home/<username>/Ruby/LDAPS/ruby-net-ldap-fix2/lib/net/ldap/connection.rb:86:in `wrap_with_ssl'

Also, testing my own pull, I noticed that the CI build will fail if it actually checks the hostname by default because the CI test suite has a cert mismatched resulting in

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

So that implies your pull doesn't match the hostname by default because the CI build succeeded. I had the same confusing issue with my attempted fix.

@jch

This comment has been minimized.

Copy link
Member

jch commented Jan 26, 2016

@tarcieri @JPvRiel thanks for starting these. I've been busy with work, so I haven't had time to review either of these in depth. @satoryu any initial thoughts on these?

@tarcieri tarcieri closed this Jan 28, 2016
@tarcieri

This comment has been minimized.

Copy link
Author

tarcieri commented Jan 28, 2016

@JPvRiel your PR is fine. I'd say let's go with that

@tarcieri tarcieri deleted the tarcieri:hostname-verification branch Jan 28, 2016
@JPvRiel

This comment has been minimized.

Copy link

JPvRiel commented Jan 28, 2016

@tarcieri I thought your approach was technically neater by passing the host address info through the function calls to wrap_with_ssl which is where one would logically expect to perform SSL checks. My fix was just hacked into the most convenient place.

Regardless, I also saw notes that the connection handling stuff might be refactored...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.