diff --git a/.rubocop.yml b/.rubocop.yml index 5ce2c60a2b35a..7cdd8ffc4cfce 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -29,6 +29,9 @@ require: - ./lib/rubocop/cop/lint/bare_check_code_in_non_exploit.rb - ./lib/rubocop/cop/lint/check_code_missing_reason.rb - ./lib/rubocop/cop/lint/module_redundant_arch_platform.rb + - ./lib/rubocop/cop/lint/module_missing_autocheck.rb + - ./lib/rubocop/cop/lint/module_default_payload.rb + - ./lib/rubocop/cop/lint/module_http_fingerprint.rb Lint/CheckCodeMissingReason: Enabled: true @@ -707,3 +710,31 @@ Lint/BareCheckCodeInNonExploit: - 'modules/auxiliary/**/*' - 'modules/post/**/*' - 'modules/evasion/**/*' + +Lint/ModuleMissingAutocheck: + Description: >- + Modules with a check method should prepend Msf::Exploit::Remote::AutoCheck + so users can verify vulnerability before exploitation. + Enabled: true + Severity: info + Include: + - 'modules/exploits/**/*' + - 'modules/auxiliary/**/*' + +Lint/ModuleDefaultPayload: + Description: >- + Do not hardcode a default PAYLOAD in DefaultOptions. + Let the framework choose the most appropriate payload automatically. + Enabled: true + Severity: warning + Include: + - 'modules/**/*' + +Lint/ModuleHttpFingerprint: + Description: >- + HttpFingerprint is a legacy passive fingerprinting mechanism. + Use a check method with AutoCheck instead. + Enabled: true + Severity: info + Include: + - 'modules/**/*' diff --git a/lib/rubocop/cop/lint/module_default_payload.rb b/lib/rubocop/cop/lint/module_default_payload.rb new file mode 100644 index 0000000000000..490abff36d977 --- /dev/null +++ b/lib/rubocop/cop/lint/module_default_payload.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Lint + # Detects modules that hardcode a PAYLOAD in DefaultOptions. + # + # The framework can automatically select the most appropriate payload based + # on the target and available session types. Hardcoding a default payload + # limits flexibility and may not work in all environments. + # + # If a module genuinely requires a specific payload (e.g. the framework's + # auto-selection picks an incompatible one), suppress with an inline + # `# rubocop:disable Lint/ModuleDefaultPayload` and add a comment explaining + # why. This makes the workaround searchable so the underlying auto-selection + # issue can be fixed later without blocking the PR. + # + # @example + # # bad - hardcoded default payload without justification + # 'DefaultOptions' => { + # 'PAYLOAD' => 'cmd/unix/reverse_bash' + # } + # + # # good - let the framework choose + # 'DefaultOptions' => { + # 'SSL' => true, + # 'WfsDelay' => 5 + # } + # + # # acceptable - justified workaround (searchable for future fix) + # 'DefaultOptions' => { + # # Auto-selection picks generic/shell_reverse_tcp which lacks job support + # 'PAYLOAD' => 'cmd/unix/reverse_bash' # rubocop:disable Lint/ModuleDefaultPayload + # } + # + class ModuleDefaultPayload < Base + MSG = 'Do not hardcode a default PAYLOAD in DefaultOptions — ' \ + 'let the framework choose automatically. ' \ + 'If a specific payload is genuinely required, add a comment explaining why ' \ + 'and suppress with `# rubocop:disable Lint/ModuleDefaultPayload`. ' \ + 'See Modernizing Existing Modules in CONTRIBUTING.md.' + + def on_pair(node) + return unless payload_in_default_options?(node) + + add_offense(node, message: MSG) + end + + private + + # Check if this pair node is 'PAYLOAD' => ... inside a 'DefaultOptions' hash + def payload_in_default_options?(node) + # Node must be a pair with key 'PAYLOAD' + return false unless node.key.str_type? && node.key.value == 'PAYLOAD' + + # Parent must be a hash + parent_hash = node.parent + return false unless parent_hash&.hash_type? + + # Grandparent must be a pair with key 'DefaultOptions' + grandparent_pair = parent_hash.parent + return false unless grandparent_pair&.pair_type? + return false unless grandparent_pair.key.str_type? && grandparent_pair.key.value == 'DefaultOptions' + + true + end + end + end + end +end diff --git a/lib/rubocop/cop/lint/module_http_fingerprint.rb b/lib/rubocop/cop/lint/module_http_fingerprint.rb new file mode 100644 index 0000000000000..4851bca0d1b32 --- /dev/null +++ b/lib/rubocop/cop/lint/module_http_fingerprint.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Lint + # Detects usage of the legacy `HttpFingerprint` constant assignment. + # + # `HttpFingerprint` was a passive fingerprinting mechanism that predates + # the modern `check` method API. Modules should implement a `check` method + # and use `prepend Msf::Exploit::Remote::AutoCheck` instead. + # + # @example + # # bad - legacy passive fingerprinting + # HttpFingerprint = { :pattern => [/Apache/] } + # + # # good - active check method + # prepend Msf::Exploit::Remote::AutoCheck + # + # def check + # # version detection logic + # CheckCode::Appears('Target appears vulnerable') + # end + # + class ModuleHttpFingerprint < Base + MSG = 'HttpFingerprint is a legacy passive fingerprinting mechanism. ' \ + 'Implement a check method and prepend AutoCheck instead. ' \ + 'See Modernizing Existing Modules in CONTRIBUTING.md.' + + def on_casgn(node) + _scope, name, _value = *node + return unless name == :HttpFingerprint + + add_offense(node, message: MSG) + end + end + end + end +end diff --git a/lib/rubocop/cop/lint/module_missing_autocheck.rb b/lib/rubocop/cop/lint/module_missing_autocheck.rb new file mode 100644 index 0000000000000..91b6ec288f1bc --- /dev/null +++ b/lib/rubocop/cop/lint/module_missing_autocheck.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Lint + # Detects exploit and auxiliary modules that define a `check` method but do not + # `prepend Msf::Exploit::Remote::AutoCheck`. + # + # AutoCheck wraps the `exploit`/`run` method to automatically call `check` before + # exploitation, giving users the ability to verify vulnerability first. + # + # @example + # # bad - check method without AutoCheck prepend + # class MetasploitModule < Msf::Exploit::Remote + # include Msf::Exploit::Remote::HttpClient + # + # def check + # CheckCode::Safe('Not vulnerable') + # end + # + # def exploit + # end + # end + # + # # good - AutoCheck prepended after includes + # class MetasploitModule < Msf::Exploit::Remote + # include Msf::Exploit::Remote::HttpClient + # prepend Msf::Exploit::Remote::AutoCheck + # + # def check + # CheckCode::Safe('Not vulnerable') + # end + # + # def exploit + # end + # end + # + class ModuleMissingAutocheck < Base + MSG = 'Module has a check method but does not prepend Msf::Exploit::Remote::AutoCheck. ' \ + 'Add it after your includes — see Modernizing Existing Modules in CONTRIBUTING.md.' + + def on_def(node) + return unless node.method_name == :check + + class_node = node.each_ancestor(:class).first + return unless class_node + + # Only flag the MetasploitModule class, not nested helper/utility classes + return unless metasploit_module_class?(class_node) + + return if has_autocheck_prepend?(class_node) + + add_offense(node, message: MSG) + end + + private + + # The framework loader requires the primary module class be named MetasploitModule + def metasploit_module_class?(class_node) + class_node.identifier.short_name == :MetasploitModule + end + + # Search the class body for a `prepend` call whose argument ends in ::AutoCheck + def has_autocheck_prepend?(class_node) + class_node.each_descendant(:send).any? do |send_node| + next unless send_node.method_name == :prepend + next if send_node.arguments.empty? + + arg = send_node.first_argument + const_ends_with_autocheck?(arg) + end + end + + # Check if a const node's name chain ends with :AutoCheck + def const_ends_with_autocheck?(node) + return false unless node&.const_type? + + node.short_name == :AutoCheck + end + end + end + end +end diff --git a/spec/rubocop/cop/lint/module_default_payload_spec.rb b/spec/rubocop/cop/lint/module_default_payload_spec.rb new file mode 100644 index 0000000000000..c41f7646dc7df --- /dev/null +++ b/spec/rubocop/cop/lint/module_default_payload_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop/cop/lint/module_default_payload' + +RSpec.describe RuboCop::Cop::Lint::ModuleDefaultPayload do + subject(:cop) { described_class.new(config) } + let(:empty_rubocop_config) { {} } + let(:config) { RuboCop::Config.new(empty_rubocop_config) } + + it 'flags DefaultOptions containing PAYLOAD key' do + expect_offense(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + def initialize(info = {}) + super( + update_info( + info, + 'Name' => 'Test Module', + 'DefaultOptions' => { + 'PAYLOAD' => 'cmd/unix/reverse_bash' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not hardcode a default PAYLOAD in DefaultOptions [...] + } + ) + ) + end + end + RUBY + end + + it 'does not flag DefaultOptions without PAYLOAD key' do + expect_no_offenses(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + def initialize(info = {}) + super( + update_info( + info, + 'Name' => 'Test Module', + 'DefaultOptions' => { + 'SSL' => true, + 'WfsDelay' => 5 + } + ) + ) + end + end + RUBY + end + + it 'does not flag modules without DefaultOptions' do + expect_no_offenses(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + def initialize(info = {}) + super( + update_info( + info, + 'Name' => 'Test Module', + 'Author' => ['Test'] + ) + ) + end + end + RUBY + end + + it 'does not flag PAYLOAD string used outside DefaultOptions context' do + expect_no_offenses(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + def initialize(info = {}) + super( + update_info( + info, + 'Name' => 'Test Module', + 'Notes' => { + 'PAYLOAD' => 'this is not DefaultOptions' + } + ) + ) + end + end + RUBY + end +end diff --git a/spec/rubocop/cop/lint/module_http_fingerprint_spec.rb b/spec/rubocop/cop/lint/module_http_fingerprint_spec.rb new file mode 100644 index 0000000000000..d33147f63081c --- /dev/null +++ b/spec/rubocop/cop/lint/module_http_fingerprint_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop/cop/lint/module_http_fingerprint' + +RSpec.describe RuboCop::Cop::Lint::ModuleHttpFingerprint do + subject(:cop) { described_class.new(config) } + let(:empty_rubocop_config) { {} } + let(:config) { RuboCop::Config.new(empty_rubocop_config) } + + it 'flags HttpFingerprint constant assignment' do + expect_offense(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + HttpFingerprint = { :pattern => [/Apache/] } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ HttpFingerprint is a legacy passive fingerprinting mechanism. [...] + end + RUBY + end + + it 'does not flag other constant assignments' do + expect_no_offenses(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + Rank = ExcellentRanking + end + RUBY + end + + it 'does not flag local variable named http_fingerprint' do + expect_no_offenses(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + def check + http_fingerprint = {} + end + end + RUBY + end + + it 'flags HttpFingerprint with different value types' do + expect_offense(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + HttpFingerprint = { :uri => '/index.html', :pattern => [] } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ HttpFingerprint is a legacy passive fingerprinting mechanism. [...] + end + RUBY + end +end diff --git a/spec/rubocop/cop/lint/module_missing_autocheck_spec.rb b/spec/rubocop/cop/lint/module_missing_autocheck_spec.rb new file mode 100644 index 0000000000000..7cf241265956f --- /dev/null +++ b/spec/rubocop/cop/lint/module_missing_autocheck_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rubocop/cop/lint/module_missing_autocheck' + +RSpec.describe RuboCop::Cop::Lint::ModuleMissingAutocheck do + subject(:cop) { described_class.new(config) } + let(:empty_rubocop_config) { {} } + let(:config) { RuboCop::Config.new(empty_rubocop_config) } + + it 'flags module with def check but no prepend AutoCheck' do + expect_offense(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + include Msf::Exploit::Remote::HttpClient + + def initialize(info = {}) + super(update_info(info, 'Name' => 'Test')) + end + + def check + ^^^^^^^^^ Module has a check method but does not prepend Msf::Exploit::Remote::AutoCheck. [...] + CheckCode::Safe('Not vulnerable') + end + + def exploit + end + end + RUBY + end + + it 'does not flag module with def check AND prepend AutoCheck' do + expect_no_offenses(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + include Msf::Exploit::Remote::HttpClient + prepend Msf::Exploit::Remote::AutoCheck + + def initialize(info = {}) + super(update_info(info, 'Name' => 'Test')) + end + + def check + CheckCode::Safe('Not vulnerable') + end + + def exploit + end + end + RUBY + end + + it 'does not flag module without a check method' do + expect_no_offenses(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + include Msf::Exploit::Remote::HttpClient + + def initialize(info = {}) + super(update_info(info, 'Name' => 'Test')) + end + + def exploit + end + end + RUBY + end + + it 'does not flag when AutoCheck is prepended with full const path' do + expect_no_offenses(<<~RUBY) + class MetasploitModule < Msf::Exploit::Remote + include Msf::Exploit::Remote::HttpClient + prepend Msf::Exploit::Remote::AutoCheck + + def check + CheckCode::Appears('Version looks vulnerable') + end + + def exploit + end + end + RUBY + end +end