Use ldap_connect in place of ldap_new to trigger the bind early

[dwelch-r7]

The auxiliary/gather/ldap_hashdump added in #13906 was using ldap_new instead of ldap_connect this lead to the logic checking for a valid connection to always claim a connection was established since ldap.get_operation_result.code will always be 0 (indicating no errors) before connecting

Simple fix to just replace ldap_new with ldap_connect

Before

    89:     ldap_new do |ldap|
    90:       require 'pry-byebug'; binding.pry
 => 91:       if ldap.get_operation_result.code == 0
    92:         vprint_status("#{peer} LDAP connection established")
    93:       else
    94:         # Even if we get "Invalid credentials" error, we may proceed with anonymous bind
    95:         print_ldap_error(ldap)
    96:       end


[1] pry(#<Msf::Modules::Auxiliary__Gather__Ldap_hashdump::MetasploitModule>)> ldap.instance_variable_get :@open_connection
=> nil

After

    89:     ldap_connect do |ldap|
    90:       require 'pry-byebug'; binding.pry
 => 91:       if ldap.get_operation_result.code == 0
    92:         vprint_status("#{peer} LDAP connection established")
    93:       else
    94:         # Even if we get "Invalid credentials" error, we may proceed with anonymous bind
    95:         print_ldap_error(ldap)
    96:       end

[1] pry(#<Msf::Modules::Auxiliary__Gather__Ldap_hashdump::MetasploitModule>)> ldap.instance_variable_get :@open_connection
=> #<Net::LDAP::Connection:0x00007f91f79da6d0 @conn=#<Socket:fd 13>, @message_queue={1=>[]}, @msgid=1>

Verification Steps

• Follow the steps listed in the module documentation https://github.com/HynekPetrak/metasploit-framework/blob/aa60b4efc0d6955e537d6650da1fa6b8918e8146/documentation/modules/auxiliary/gather/ldap_hashdump.md#verification-steps

[adfoster-r7]

It looks like this now skips the monkey patching logic that was explicitly put in to catch some bind errors:

metasploit-framework/lib/msf/core/exploit/remote/ldap.rb

Lines 223 to 255 in dfbd14e

	def ldap_new(opts = {})
	ldap = Net::LDAP.new(get_connect_opts.merge(opts))
	# NASTY, but required
	# monkey patch ldap object in order to ignore bind errors
	# Some servers (e.g. OpenLDAP) return result even after a bind
	# has failed, e.g. with LDAP_INAPPROPRIATE_AUTH - anonymous bind disallowed.
	# See: https://www.openldap.org/doc/admin23/security.html#Authentication%20Methods
	# "Note that disabling the anonymous bind mechanism does not prevent anonymous
	# access to the directory."
	# Bug created for Net:LDAP at https://github.com/ruby-ldap/ruby-net-ldap/issues/375
	#
	# @yieldparam conn [Net::LDAP] The LDAP connection handle to use for connecting to
	# the target LDAP server.
	# @param args [Hash] A hash containing options for the ldap connection
	def ldap.use_connection(args)
	if @open_connection
	yield @open_connection
	else
	begin
	conn = new_connection
	conn.bind(args[:auth] || @auth)
	# Commented out vs. original
	# result = conn.bind(args[:auth] || @auth)
	# return result unless result.result_code == Net::LDAP::ResultCodeSuccess
	yield conn
	ensure
	conn.close if conn
	end
	end
	end
	yield ldap
	end

Is that intentional?

[dwelch-r7]

oh, I didn't spot that, not intentional no, I'll take another pass at this, would even just reverting this change and removing the if ldap.get_operation_result.code == 0 section be viable?

[gwillcox-r7]

Happy to review this when ready but seems like this may need some revision. Going to await Dean's updates, then feel free to ping me when you'd like a review.

[dwelch-r7]

Closing this for now, this doesn't seem like the right solution