Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc3f27d
Addresses the LDAPS vulnerability to MITM attacks by checking hostnam…
JPvRiel-SB Jan 14, 2016
51f12c5
fix a regression in a prior commit and check if :verify_mode key exis…
JPvRiel Jan 20, 2016
da59fd5
second fix to conditonal logic after more extensive positive and nega…
JPvRiel Jan 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/net/ldap/connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,15 @@ def open_connection(server)
hosts.each do |host, port|
begin
prepare_socket(server.merge(socket: @socket_class.new(host, port, socket_opts)))
if encryption
if encryption[:tls_options] &&
encryption[:tls_options][:verify_mode] &&
encryption[:tls_options][:verify_mode] == OpenSSL::SSL::VERIFY_NONE
warn "not verifying SSL hostname of LDAPS server"
else
@conn.post_connection_check(host)
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return
rescue Net::LDAP::Error, SocketError, SystemCallError,
OpenSSL::SSL::SSLError => e
Expand Down