Feedback from code review

lib/msf/base/sessions/ldap.rb

@@ -15,15 +15,20 @@ class Msf::Sessions::LDAP
   attr_accessor :client
 
   attr_accessor :keep_alive_thread
+
+  # @return [Integer] Seconds between keepalive requests
+  attr_accessor :keepalive_seconds
 
   attr_accessor :platform, :arch
   attr_reader :framework
 
   # @param[Rex::IO::Stream] rstream
   # @param [Hash] opts
   # @option opts [Rex::Proto::LDAP::Client] :client
+  # @option opts [Integer] :keepalive
   def initialize(rstream, opts = {})
     @client = opts.fetch(:client)
+    @keepalive_seconds = opts.fetch(:keepalive_seconds)
     self.console = Rex::Post::LDAP::Ui::Console.new(self)
     super(rstream, opts)
   end
@@ -152,20 +157,19 @@ def on_registered
 
   # Start a background thread for regularly sending a no-op command to keep the connection alive
   def start_keep_alive_loop
-    self.keep_alive_thread = framework.threads.spawn('LDAP-shell-keepalive', false) do
-      keep_alive_timeout = 10 * 60 # 10 minutes
+    self.keep_alive_thread = framework.threads.spawn("LDAP-shell-keepalive-#{sid}", false) do
       loop do
         if client.last_interaction.nil?
-          remaining_sleep = keep_alive_timeout
+          remaining_sleep = @keepalive_seconds
         else
-          remaining_sleep = keep_alive_timeout - (Time.now - client.last_interaction)
+          remaining_sleep = @keepalive_seconds - (Process.clock_gettime(Process::CLOCK_MONOTONIC) - client.last_interaction)
         end
         sleep(remaining_sleep)
-        if (Time.now - client.last_interaction) > keep_alive_timeout
+        if (Process.clock_gettime(Process::CLOCK_MONOTONIC) - client.last_interaction) > @keepalive_seconds
           client.search_root_dse
         end
         # This should have moved last_interaction forwards
-        fail if (Time.now - client.last_interaction) > keep_alive_timeout
+        fail if (Process.clock_gettime(Process::CLOCK_MONOTONIC) - client.last_interaction) > @keepalive_seconds
       end
     end
   end

lib/msf/core/exploit/remote/ldap.rb

@@ -168,7 +168,6 @@ def resolve_connect_opts(connect_opts)
     #   the target LDAP server.
     def ldap_new(opts = {})
       ldap = Rex::Proto::LDAP::Client.new(resolve_connect_opts(get_connect_opts.merge(opts)))
-      mutex = Mutex.new
 
       # NASTY, but required
       # monkey patch ldap object in order to ignore bind errors
@@ -185,9 +184,7 @@ def ldap_new(opts = {})
       # @param args [Hash] A hash containing options for the ldap connection
       def ldap.use_connection(args)
         if @open_connection
-          mutex.synchronize do
-            yield @open_connection
-          end
+          yield @open_connection
           register_interaction
         else
           begin

lib/rex/proto/ldap/client.rb

@@ -25,7 +25,7 @@ def initialize(args)
         end
 
         def register_interaction
-          @last_interaction = Time.now
+          @last_interaction = Process.clock_gettime(Process::CLOCK_MONOTONIC)
         end
 
         # @return [Array<String>] LDAP servers naming contexts

modules/auxiliary/scanner/ldap/ldap_login.rb

@@ -36,7 +36,8 @@ def initialize(info = {})
         OptBool.new(
           'APPEND_DOMAIN', [true, 'Appends `@<DOMAIN> to the username for authentication`', false],
           conditions: ['LDAP::Auth', 'in', [Msf::Exploit::Remote::AuthOption::AUTO, Msf::Exploit::Remote::AuthOption::PLAINTEXT]]
-        )
+        ),
+        OptInt.new('SessionKeepalive', [true, 'Time (in seconds) for sending protocol-level keepalive messages', 10 * 60])
       ]
     )
 
@@ -48,6 +49,7 @@ def initialize(info = {})
     else
       # Don't give the option to create a session unless ldap sessions are enabled
       options_to_deregister << 'CreateSession'
+      options_to_deregister << 'SessionKeepalive'
     end
 
     deregister_options(*options_to_deregister)
@@ -175,7 +177,7 @@ def session_setup(result)
     return unless result.connection && result.proof
 
     # Create a new session
-    my_session = Msf::Sessions::LDAP.new(result.connection, { client: result.proof })
+    my_session = Msf::Sessions::LDAP.new(result.connection, { client: result.proof, keepalive_seconds: datastore['SessionKeepalive'] })
 
     merge_me = {
       'USERPASS_FILE' => nil,