Pass module_path instead of parent_path to file_changed?

[Fixes #37630057]

Modules were always being detected as having file changes because the
parent_path directory, instead of the actual module_path, was being
passed to module_manager.file_changed?, which caused the modification
times to not match.

To ensure this change fixes the ambiguous module warnings, a full spec
for Msf::Core::Modules::Loader::Base has been written.

spec/msf has moved to spec/lib/msf to match conventional spec layout and
allow for the spec/support directory to not be confused as a lib
subdirectory being tested.

Author: limhoff-r7

lib/msf/core.rb

@@ -19,6 +19,20 @@
 
 module Msf
 	LogSource = "core"
+
+	# Returns Pathname of the root of the Metasploit Framework source
+	#
+	# @return [Pathname] root directory above lib, data, modules, etc.
+	def self.root
+		unless instance_variable_defined? :@root
+			msf_pathname = Pathname.new(__FILE__).dirname
+			lib_pathname = msf_pathname.parent
+
+			@root = lib_pathname.parent
+		end
+
+		@root
+	end
 end
 
 # General

lib/msf/core/module_manager/loading.rb

@@ -26,11 +26,11 @@ module Msf::ModuleManager::Loading
   def file_changed?(path)
     changed = false
 
-    module_details = self.module_info_by_path[path]
+    module_info = self.module_info_by_path[path]
 
     # if uncached then it counts as changed
     # Payloads can't be cached due to stage/stager matching
-    if module_details.nil? or module_details[:mtype] == Msf::MODULE_PAYLOAD
+    if module_info.nil? or module_info[:modification_time] == Msf::MODULE_PAYLOAD
       changed = true
     else
       begin
@@ -39,7 +39,7 @@ def file_changed?(path)
         # if the file does not exist now, that's a change
         changed = true
       else
-        cached_modification_time = module_details[:mtime].to_i
+        cached_modification_time = module_info[:modification_time].to_i
 
         # if the file's modification time's different from the cache, then it's changed
         if current_modification_time != cached_modification_time

lib/msf/core/modules/loader/base.rb

@@ -3,6 +3,7 @@
 #
 require 'msf/core/modules/loader'
 require 'msf/core/modules/namespace'
+require 'msf/core/modules/version_compatibility_error'
 
 # Responsible for loading modules for {Msf::ModuleManager}.
 #
@@ -77,7 +78,6 @@ def loadable?(path)
     raise ::NotImplementedError
   end
 
-
   # Loads a module from the supplied path and module_reference_name.
   #
   # @param [String] parent_path The path under which the module exists.  This is not necessarily the same path as passed
@@ -104,14 +104,18 @@ def load_module(parent_path, type, module_reference_name, options={})
     force = options[:force] || false
     reload = options[:reload] || false
 
-    unless force or module_manager.file_changed?(parent_path)
-      dlog("Cached module from #{parent_path} has not changed.", 'core', LEV_2)
+    module_path = self.module_path(parent_path, type, module_reference_name)
+    file_changed = module_manager.file_changed?(module_path)
+
+    unless force or file_changed
+      dlog("Cached module from #{module_path} has not changed.", 'core', LEV_2)
 
       return false
     end
 
+    reload ||= force || file_changed
+
     metasploit_class = nil
-    module_path = self.module_path(parent_path, type, module_reference_name)
 
     loaded = namespace_module_transaction(type + "/" + module_reference_name, :reload => reload) { |namespace_module|
       # set the parent_path so that the module can be reloaded with #load_module
@@ -121,7 +125,7 @@ def load_module(parent_path, type, module_reference_name, options={})
 
       begin
         namespace_module.module_eval_with_lexical_scope(module_content, module_path)
-          # handle interrupts as pass-throughs unlike other Exceptions
+      # handle interrupts as pass-throughs unlike other Exceptions so users can bail with Ctrl+C
       rescue ::Interrupt
         raise
       rescue ::Exception => error
@@ -191,6 +195,11 @@ def load_module(parent_path, type, module_reference_name, options={})
       # not trigger an ambiguous name warning, which would cause the reloaded module to not be stored in the
       # ModuleManager.
       module_manager.delete(module_reference_name)
+
+	    # Delete the original copy of the module in the type-specific module set stores the reloaded module and doesn't
+	    # trigger an ambiguous name warning
+	    module_set = module_manager.module_set(type)
+	    module_set.delete(module_reference_name)
     end
 
     # Do some processing on the loaded module to get it into the right associations
@@ -290,7 +299,6 @@ def reload_module(original_metasploit_class_or_instance)
 
     dlog("Reloading module #{module_reference_name}...", 'core')
 
-
     if load_module(parent_path, type, module_reference_name, :force => true, :reload => true)
       # Create a new instance of the module
       reloaded_module_instance = module_manager.create(module_reference_name)
@@ -486,20 +494,18 @@ def namespace_module_transaction(uniq_module_reference_name, options={}, &block)
       elog("Reloading namespace_module #{previous_namespace_module} when :reload => false")
     end
 
+    relative_name = namespace_module_names.last
+
     if previous_namespace_module
       parent_module = previous_namespace_module.parent
-      relative_name = namespace_module_names.last
-
-      # remove_const is private, so invoke in instance context
-      parent_module.instance_eval do
-        remove_const relative_name
-      end
-    else
-      parent_module = nil
-      relative_name = namespace_module_names.last
+      # remove_const is private, so use send to bypass
+      parent_module.send(:remove_const, relative_name)
     end
 
     namespace_module = create_namespace_module(namespace_module_names)
+    # Get the parent module from the created module so that restore_namespace_module can remove namespace_module's
+    # constant if needed.
+    parent_module = namespace_module.parent
 
     begin
       loaded = block.call(namespace_module)
@@ -534,17 +540,29 @@ def read_module_content(parent_path, type, module_reference_name)
   #
   # @param [Module] parent_module The .parent of namespace_module before it was removed from the constant tree.
   # @param [String] relative_name The name of the constant under parent_module where namespace_module was attached.
-  # @param [Module] namespace_module The previous namespace module containing the old module content.
+  # @param [Module, nil] namespace_module The previous namespace module containing the old module content.  If `nil`,
+  #   then the relative_name constant is removed from parent_module, but nothing is set as the new constant.
   # @return [void]
   def restore_namespace_module(parent_module, relative_name, namespace_module)
-    if parent_module and namespace_module
-      # the const may have been redefined by {#create_namespace_module}, in which case that new namespace_module needs
-      # to be removed so the original can replace it.
-      if parent_module.const_defined? relative_name
-        remove_const relative_name
+    if parent_module
+      # If there is a current module with relative_name
+      if parent_module.const_defined?(relative_name)
+	      # if the current value isn't the value to be restored.
+	      if parent_module.const_get(relative_name) != namespace_module
+		      # remove_const is private, so use send to bypass
+		      parent_module.send(:remove_const, relative_name)
+
+		      # if there was a previous module, not set it to the name
+		      if namespace_module
+			      parent_module.const_set(relative_name, namespace_module)
+		      end
+	      end
+      else
+	      # if there was a previous module, but there isn't a current module, then restore the previous module
+	      if namespace_module
+		      parent_module.const_set(relative_name, namespace_module)
+	      end
       end
-
-      parent_module.const_set(relative_name, namespace_module)
     end
   end
 

spec/msf/core/module_manager_spec.rb renamed to spec/lib/msf/core/module_manager_spec.rb

@@ -107,4 +107,78 @@
       end
     end
   end
+
+	context '#file_changed?' do
+		let(:module_basename) do
+			[basename_prefix, '.rb']
+		end
+
+		it 'should return true if module info is not cached' do
+			Tempfile.open(module_basename) do |tempfile|
+				module_path = tempfile.path
+
+				subject.send(:module_info_by_path)[module_path].should be_nil
+				subject.file_changed?(module_path).should be_true
+			end
+		end
+
+		it 'should return true if the cached modification time is Msf::MODULE_PAYLOAD' do
+			Tempfile.open(module_basename) do |tempfile|
+				module_path = tempfile.path
+
+				subject.send(:module_info_by_path)[module_path] = {
+						:modification_time => Msf::MODULE_PAYLOAD
+				}
+
+				subject.file_changed?(module_path).should be_true
+			end
+		end
+
+		context 'with cache module info and not a payload module' do
+			it 'should return true if the file does not exist on the file system' do
+				tempfile = Tempfile.new(module_basename)
+				module_path = tempfile.path
+				modification_time = File.mtime(module_path).to_i
+
+				subject.send(:module_info_by_path)[module_path] = {
+						:modification_time => modification_time
+				}
+
+				tempfile.unlink
+
+				File.exist?(module_path).should be_false
+				subject.file_changed?(module_path).should be_true
+			end
+
+			it 'should return true if modification time does not match the cached modification time' do
+				Tempfile.open(module_basename) do |tempfile|
+					module_path = tempfile.path
+					modification_time = File.mtime(module_path).to_i
+					cached_modification_time = (modification_time * rand).to_i
+
+					subject.send(:module_info_by_path)[module_path] = {
+							:modification_time => cached_modification_time
+					}
+
+					cached_modification_time.should_not == modification_time
+					subject.file_changed?(module_path).should be_true
+				end
+			end
+
+			it 'should return false if modification time does match the cached modification time' do
+				Tempfile.open(module_basename) do |tempfile|
+					module_path = tempfile.path
+					modification_time = File.mtime(module_path).to_i
+					cached_modification_time = modification_time
+
+					subject.send(:module_info_by_path)[module_path] = {
+							:modification_time => cached_modification_time
+					}
+
+					cached_modification_time.should == modification_time
+					subject.file_changed?(module_path).should be_false
+				end
+			end
+		end
+	end
 end