Skip to content

Improve vuln and vuln attempt tracking#21307

Merged
adfoster-r7 merged 2 commits intorapid7:masterfrom
adfoster-r7:improve-vuln-and-vuln-attempt-tracking
Apr 24, 2026
Merged

Improve vuln and vuln attempt tracking#21307
adfoster-r7 merged 2 commits intorapid7:masterfrom
adfoster-r7:improve-vuln-and-vuln-attempt-tracking

Conversation

@adfoster-r7
Copy link
Copy Markdown
Contributor

@adfoster-r7 adfoster-r7 commented Apr 15, 2026

Improves vulnerability and vulnerability attempt tracking in Metasploit. Now additional details are registered - giving operators richer context when reviewing discovered vulnerabilities.

Example with check code and check detail now appearing after the check method has been run on a module:

msf > vulns -v

Vulnerabilities
===============
  0. Vuln ID: 10
     Timestamp: 2026-04-15 00:05:25 UTC
     Host: 10.140.113.233
     Name: ElasticSearch Snapshot API Directory Traversal
     References: CVE-2015-5531,PACKETSTORM-132721
     Information: Vulnerability confirmed by check of auxiliary/scanner/http/elasticsearch_traversal.
     Resource: {}
     Service:
     Vuln attempts:
     0. ID: 28
        Vuln ID: 10
        Timestamp: 2026-04-15 00:05:25 UTC
        Exploit: false
        Fail reason: none
        Username: foo
        Module: auxiliary/scanner/http/elasticsearch_traversal
        Session ID: nil
        Loot ID: nil
        Fail Detail: nil
        Check Code: appears
        Check Detail: Successfully created snapshot repositories, suggesting the Snapshot API is vulnerable to CVE-2015-5531.

Verification

  • Ensure CI passes
  • Run through workflows that register vulns / vuln attempts, and verify the outputs with vulns -v

@adfoster-r7 adfoster-r7 force-pushed the improve-vuln-and-vuln-attempt-tracking branch 2 times, most recently from 7ab8824 to bd8238e Compare April 15, 2026 13:30
@adfoster-r7 adfoster-r7 requested a review from Copilot April 15, 2026 13:30
@adfoster-r7 adfoster-r7 force-pushed the improve-vuln-and-vuln-attempt-tracking branch from bd8238e to f0a658f Compare April 15, 2026 13:39
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances Metasploit’s vulnerability and vulnerability-attempt tracking by persisting and displaying check outcomes (code + detail) alongside recorded vuln attempts, giving operators more context when reviewing results (e.g., via vulns -v).

Changes:

  • Persist check_code and check_detail on Mdm::VulnAttempt, and display them in vulns -v.
  • Enrich vuln/vuln-attempt registration paths (AutoCheck, report_failure, report_vuln, DB manager) to carry check result metadata and avoid duplicate attempts.
  • Expand test coverage with new/updated specs and update a few modules to provide richer check details / use AutoCheck.

Reviewed changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
spec/lib/msf/ui/console/command_dispatcher/db_spec.rb Updates expected vulns -v output to include check fields.
spec/lib/msf/core/vuln_attempt_registration_spec.rb Adds integration specs covering vuln + vuln_attempt creation across check/run/autocheck scenarios.
spec/lib/msf/core/module/failure_spec.rb Adds tests for check-code-to-failure mapping and CheckCode equality w/ metadata.
spec/lib/msf/core/exploit/remote/auto_check_spec.rb Updates expectations so AutoCheck passes check_code into vuln reporting.
modules/exploits/multi/http/phpmyadmin_preg_replace.rb Enables AutoCheck for the exploit module.
modules/auxiliary/scanner/smb/smb_ms17_010.rb Passes check_code into report_vuln for richer attempt context.
modules/auxiliary/scanner/http/elasticsearch_traversal.rb Adds detailed reason strings to CheckCode results.
lib/msf/ui/console/module_command_dispatcher.rb Treats Appears as a “good” check outcome and reports it as a vuln.
lib/msf/ui/console/command_dispatcher/db.rb Prints Check Code / Check Detail for vuln attempts in vulns -v.
lib/msf/core/module/failure.rb Adds check metadata to failure reporting, maps fail_reason from check results, and ensures a vuln exists for check outcomes.
lib/msf/core/exploit/remote/auto_check.rb Includes check_code in report_vuln opts for AutoCheck-discovered vulns.
lib/msf/core/exploit.rb Adds vuln_attempt_recorded accessor to support de-duplication.
lib/msf/core/db_manager/exploit_attempt.rb Persists check metadata on vuln attempts, improves vuln lookup fallback, and avoids duplicate vuln_attempt creation.
lib/msf/core/auxiliary/report.rb Populates check metadata on vuln_attempts created via report_vuln and sets a de-dupe flag.
lib/msf/core/auxiliary.rb Adds check_code + vuln_attempt_recorded accessors for auxiliary modules.
lib/msf/base/simple/auxiliary.rb Stores CheckCode results from check execution onto the module instance.
db/schema.rb Adds check_code and check_detail columns to vuln_attempts.
Gemfile.lock Switches metasploit_data_models to a git source and reflects dependency resolution updates.
Gemfile Pins metasploit_data_models to a specific git fork/branch.

Comment thread lib/msf/base/simple/auxiliary.rb
Comment on lines +113 to +117
# Skip creating a duplicate vuln attempt if one was already recorded
# during this run (e.g. by Auxiliary::Report#report_vuln).
if self.respond_to?(:vuln_attempt_recorded) && self.vuln_attempt_recorded
info[:skip_vuln_attempt] = true
end
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

vuln_attempt_recorded is used to suppress duplicate vuln attempts, but it is never reset. Because module instances can be run multiple times, a previous run that set this flag will cause later runs to set skip_vuln_attempt even when no attempt was recorded for the current run, leading to missing vuln_attempt records. Reset this flag at the end of report_failure (or at the start of each run) so it is scoped to a single execution.

Copilot uses AI. Check for mistakes.
Comment thread lib/msf/core/module/failure.rb
Comment thread lib/msf/core/auxiliary/report.rb Outdated
Comment on lines +336 to +340
# Signal that a vuln attempt was already recorded for this run so that
# report_failure (called later by the job wrapper) can skip creating a
# duplicate attempt in do_report_failure_or_success.
self.vuln_attempt_recorded = true if self.respond_to?(:vuln_attempt_recorded=)

Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

report_vuln sets vuln_attempt_recorded = true to avoid duplicates, but there is no corresponding reset. This state can persist across subsequent runs of the same module instance and incorrectly suppress vuln attempt creation. Ensure the flag is cleared after attempt reporting completes (e.g., in report_failure ensure, or at the beginning of each run).

Copilot uses AI. Check for mistakes.
Comment thread Gemfile Outdated
# Add default group gems to `metasploit-framework.gemspec`:
# spec.add_runtime_dependency '<name>', [<version requirements>]
gemspec name: 'metasploit-framework'
gem 'metasploit_data_models', git: 'https://github.com/adfoster-r7/metasploit_data_models', branch: 'add-checkcode-details-to-vuln-attempt'
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

Pinning metasploit_data_models to a personal GitHub fork/branch makes builds non-reproducible and introduces a supply-chain risk (the branch can be force-pushed or removed). Prefer depending on an official released gem version (or at least a specific immutable git revision) and remove the fork/branch override before merging.

Suggested change
gem 'metasploit_data_models', git: 'https://github.com/adfoster-r7/metasploit_data_models', branch: 'add-checkcode-details-to-vuln-attempt'
gem 'metasploit_data_models'

Copilot uses AI. Check for mistakes.
Comment thread lib/msf/core/module/failure.rb Outdated
Comment on lines +104 to +109
framework.db.report_vuln(
workspace: info[:workspace],
host: info[:host],
name: self.name,
refs: self.references,
info: "Vulnerability confirmed by check of #{self.fullname}."
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

The report_vuln info string is always "Vulnerability confirmed by check..." even when check_code is Appears. Appears is explicitly a non-confirmed/heuristic result, so this message is inaccurate and will mislead operators reviewing vuln records. Consider using different phrasing for Appears (e.g., "appears vulnerable") or incorporate check_code.code into the message.

Suggested change
framework.db.report_vuln(
workspace: info[:workspace],
host: info[:host],
name: self.name,
refs: self.references,
info: "Vulnerability confirmed by check of #{self.fullname}."
vuln_info = if self.check_code == Msf::Exploit::CheckCode::Appears
"Target appears vulnerable based on check of #{self.fullname}."
else
"Vulnerability confirmed by check of #{self.fullname}."
end
framework.db.report_vuln(
workspace: info[:workspace],
host: info[:host],
name: self.name,
refs: self.references,
info: vuln_info

Copilot uses AI. Check for mistakes.
@smcintyre-r7 smcintyre-r7 moved this from Todo to Ready in Metasploit Kanban Apr 17, 2026
@adfoster-r7 adfoster-r7 force-pushed the improve-vuln-and-vuln-attempt-tracking branch 2 times, most recently from fd84e38 to a4fb46e Compare April 23, 2026 17:05
@adfoster-r7 adfoster-r7 requested a review from Copilot April 23, 2026 17:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

Comment on lines +162 to +173
if res[7, 2] == "\x3e\x00"
# send ChannelRequestTwo - prevent BSoD
sock.put(channel_request << [user2, chan2].pack("nn"))

report_goods
return Exploit::CheckCode::Vulnerable
return Exploit::CheckCode::Vulnerable('Response confirmed vulnerability presence')
else
return Exploit::CheckCode::Safe
return Exploit::CheckCode::Safe('Not vulerable')
end

# Can't determine, but at least I know the service is running
return Exploit::CheckCode::Detected
return Exploit::CheckCode::Detected('Service detected, but unable to confirm vulnerability')
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

check_rdp_vuln always returns from the if/else at lines 162-170, so the Detected return afterwards is unreachable. This makes the intent unclear and the Detected path can never occur; consider restructuring to only return Safe/Vulnerable for confirmed cases and fall through to Detected when the response doesn't allow a determination (or remove the dead code).

Copilot uses AI. Check for mistakes.
return Exploit::CheckCode::Vulnerable('Response confirmed vulnerability presence')
else
return Exploit::CheckCode::Safe
return Exploit::CheckCode::Safe('Not vulerable')
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

Spelling typo in the Safe check message: "Not vulerable" should be "Not vulnerable" (user-facing output).

Suggested change
return Exploit::CheckCode::Safe('Not vulerable')
return Exploit::CheckCode::Safe('Not vulnerable')

Copilot uses AI. Check for mistakes.
Comment thread lib/msf/core/module/failure.rb
Comment on lines 178 to +192
opts = {
workspace: instance.workspace,
host: instance.respond_to?(:target_host) && instance.target_host ? instance.target_host : instance.datastore['RHOST'],
proto: instance.datastore['PROTO'] || 'tcp',
name: instance.name,
info: "This was flagged as vulnerable by the explicit check of #{instance.fullname}.",
refs: instance.references
}

# Include port so that checks against different ports on the same host
# create distinct vuln records instead of collapsing into one.
if instance.datastore['RPORT']
rport = instance.respond_to?(:rport) ? instance.rport : instance.datastore['RPORT']
opts[:port] = rport.to_i if rport.to_i > 0
end
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

report_vuln defaults proto to 'tcp' (and datastore['PROTO'] doesn't appear to be a standard option elsewhere). With the new port inclusion, this can cause UDP checks to create/associate service-scoped vulns as TCP. Consider deriving proto from the module's transport mixins (e.g., Msf::Exploit::Remote::Udp vs Tcp) or the service context instead of defaulting to TCP.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.

Comment on lines +82 to +86
# Skip vuln lookup entirely when the check result indicates the target
# is not vulnerable. A Safe/Unknown check on port 80 should not
# attach a VulnAttempt to an unrelated vuln on port 9200 just because
# the module refs happen to match.
safe_fail_reasons = [Msf::Module::Failure::NotVulnerable, Msf::Module::Failure::Unknown]
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

safe_fail_reasons includes Msf::Module::Failure::Unknown, which is also used by many modules for genuine exploit/runtime failures (via fail_with(Failure::Unknown, ...)). Skipping vuln lookup for all Unknown failures can drop the vuln association for real exploit attempts. Consider only applying this skip when an explicit non-vulnerable check result is present (e.g. based on opts[:check_code]), rather than using fail_reason alone.

Copilot uses AI. Check for mistakes.
Comment on lines +176 to +180
# Skip vuln lookup when the result indicates the target is not
# vulnerable — avoids attaching a VulnAttempt to an unrelated vuln
# on a different port that happens to share the same refs.
safe_fail_reasons = [Msf::Module::Failure::NotVulnerable, Msf::Module::Failure::Unknown]
unless safe_fail_reasons.include?(freason)
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

Here safe_fail_reasons again treats Msf::Module::Failure::Unknown as “safe” for vuln lookup. This can prevent linking attempts to existing vulns for legitimate exploit failures that use Failure::Unknown. Prefer keying this behavior off an explicit check result field (e.g. opts[:check_code]) instead of the generic freason.

Suggested change
# Skip vuln lookup when the result indicates the target is not
# vulnerable — avoids attaching a VulnAttempt to an unrelated vuln
# on a different port that happens to share the same refs.
safe_fail_reasons = [Msf::Module::Failure::NotVulnerable, Msf::Module::Failure::Unknown]
unless safe_fail_reasons.include?(freason)
# Skip vuln lookup only when an explicit check result indicates the
# target is not vulnerable. Do not infer "safe" from a generic
# exploit failure reason such as Failure::Unknown.
check_code = opts[:check_code]
target_not_vulnerable = false
if check_code.respond_to?(:code)
target_not_vulnerable = check_code.code.to_s.casecmp('safe').zero?
elsif check_code.is_a?(String) || check_code.is_a?(Symbol)
target_not_vulnerable = check_code.to_s.casecmp('safe').zero?
elsif defined?(Msf::Exploit::CheckCode::Safe)
target_not_vulnerable = (check_code == Msf::Exploit::CheckCode::Safe)
end
unless target_not_vulnerable

Copilot uses AI. Check for mistakes.
Comment on lines 57 to 60
def run
@scanner_run_completed = true
@show_progress = datastore['ShowProgress']
@show_percent = datastore['ShowProgressPercent'].to_i
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

run sets @scanner_run_completed = true immediately. Since Auxiliary::Scanner#report_failure returns early when this flag is set, any exception raised during run before per-host threads execute will cause the job wrapper’s ensure block to skip reporting the failure entirely (no attempt recorded). Consider setting the flag only after the scan completes normally, or only skipping when fail_reason == Msf::Module::Failure::None.

Copilot uses AI. Check for mistakes.
end

CheckCode::Safe
CheckCode::Safe('Could not determine phpMyAdmin version.')
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

The final branch returns CheckCode::Safe('Could not determine phpMyAdmin version.'), but “could not determine” is not a Safe result (Safe means the target is not exploitable). This message indicates an indeterminate state; consider returning CheckCode::Unknown (or Detected if you only confirmed the service) so operators don’t get a false sense of safety.

Suggested change
CheckCode::Safe('Could not determine phpMyAdmin version.')
CheckCode::Unknown('Could not determine phpMyAdmin version.')

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.

Comment on lines 162 to 174
if res[7, 2] == "\x3e\x00"
# send ChannelRequestTwo - prevent BSoD
sock.put(channel_request << [user2, chan2].pack("nn"))

report_goods
return Exploit::CheckCode::Vulnerable
return Exploit::CheckCode::Vulnerable('Response confirmed vulnerability presence')
else
return Exploit::CheckCode::Safe
return Exploit::CheckCode::Safe('Not vulnerable')
end

# Can't determine, but at least I know the service is running
return Exploit::CheckCode::Detected
return Exploit::CheckCode::Detected('Service detected, but unable to confirm vulnerability')
end
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

The final return Exploit::CheckCode::Detected(...) is unreachable because both branches of the preceding if res[7, 2] == ... already return. If the intent is to fall back to Detected when the response is inconclusive, restructure the control flow; otherwise remove the dead code to avoid confusion.

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +99
# Skip vuln lookup entirely when the check result indicates the target
# is not vulnerable. A Safe/Unknown check on port 80 should not
# attach a VulnAttempt to an unrelated vuln on port 9200 just because
# the module refs happen to match.
safe_fail_reasons = [Msf::Module::Failure::NotVulnerable, Msf::Module::Failure::Unknown]
unless safe_fail_reasons.include?(opts[:fail_reason])
# Try to find an existing vulnerability with the same service & references
# or, if svc is nil, with the same host & references
vuln = find_vuln_by_refs(rids, host, svc, false)

# Fall back to a host-only lookup when the service-scoped query found
# nothing. Vulns created by `check` (via module_command_dispatcher's
# report_vuln) often have no associated service, so a service-scoped
# query will miss them.
if vuln.nil? && svc
vuln = find_vuln_by_refs(rids, host, nil, false)
end
end
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

safe_fail_reasons includes Msf::Module::Failure::Unknown, which is also used for many real exploit/run failures (timeouts, unexpected replies, etc.), not just check results. This change will prevent those failures from being associated with an existing vuln (no VulnAttempt row) even when the exploit is targeting the same service/refs. Consider skipping vuln lookup only when the caller is reporting a check outcome (e.g., gate on opts[:check_code] being present and indicating Safe/Unknown/Detected), rather than treating all fail_reason == Unknown as “safe” for vuln association.

Copilot uses AI. Check for mistakes.
Comment on lines +176 to +184
# Skip vuln lookup when the result indicates the target is not
# vulnerable — avoids attaching a VulnAttempt to an unrelated vuln
# on a different port that happens to share the same refs.
safe_fail_reasons = [Msf::Module::Failure::NotVulnerable, Msf::Module::Failure::Unknown]
unless safe_fail_reasons.include?(freason)
# Try find a matching vulnerability
vuln = find_vuln_by_refs(ref_objs, host, svc, false)
vuln ||= find_vuln_by_refs(ref_objs, host, nil, false) if svc && vuln.nil?
end
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

Same issue as above: treating fail_reason == Unknown as a reason to skip vuln lookup can drop VulnAttempt associations for genuine exploit/run failures that are categorized as Unknown. Tighten the condition (e.g., only skip when opts[:check_code] is present and represents a non-vulnerable check result) so exploit failures still attach to the correct vuln when one exists.

Copilot uses AI. Check for mistakes.
Comment on lines 177 to 189
def report_vuln(instance, checkcode = nil)
opts = {
workspace: instance.workspace,
host: instance.respond_to?(:target_host) && instance.target_host ? instance.target_host : instance.datastore['RHOST'],
proto: instance.datastore['PROTO'] || 'tcp',
proto: if instance.class.ancestors.include?(Msf::Exploit::Remote::Udp)
'udp'
else
'tcp'
end,
name: instance.name,
info: "This was flagged as vulnerable by the explicit check of #{instance.fullname}.",
refs: instance.references
}
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

check_simple now calls report_vuln for CheckCode::Appears, but report_vuln always sets info to "flagged as vulnerable by the explicit check". For Appears this is misleading (it’s a weaker assertion than confirmed vulnerability). Consider tailoring the info string based on the check code (e.g., “appears vulnerable…” vs “confirmed vulnerable…”).

Copilot uses AI. Check for mistakes.
@adfoster-r7 adfoster-r7 force-pushed the improve-vuln-and-vuln-attempt-tracking branch from d37646f to 6902e57 Compare April 24, 2026 00:30
@adfoster-r7 adfoster-r7 requested a review from Copilot April 24, 2026 00:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.

Comment thread lib/msf/core/auxiliary/scanner.rb
Comment on lines +889 to +892
let(:current_mod) do
selective_scanner_class.vulnerable_hosts = Set['192.0.2.1']
selective_scanner_class.injected_check_code = appears_check_code
build_module(selective_scanner_class)
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

This spec uses Set[...] but does not require Ruby’s set standard library anywhere in the file/spec helpers. This can raise NameError: uninitialized constant Set depending on load order. Add require 'set' (or avoid Set) so the spec is self-contained and deterministic.

Copilot uses AI. Check for mistakes.
Comment thread lib/msf/core/auxiliary/report.rb
@adfoster-r7 adfoster-r7 force-pushed the improve-vuln-and-vuln-attempt-tracking branch from 6902e57 to 5d0d0ab Compare April 24, 2026 09:23
@adfoster-r7 adfoster-r7 requested a review from Copilot April 24, 2026 09:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.

Comment thread lib/msf/core/auxiliary/scanner.rb
Comment on lines +82 to +83
# Skip vuln lookup when an explicit check result indicates the target
# is not vulnerable. Only key off check_code — fail_reason alone is
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

The safe_check_codes list includes CheckCode::Unknown, but the surrounding comment says this branch is for check results that indicate the target is not vulnerable. Unknown usually means the check couldn't determine vulnerability (e.g. connectivity/timeout), so this is at least misleading documentation; if the intent is specifically to avoid associating attempts when the check failed to prove vulnerability, please update the comment accordingly (or reconsider whether Unknown should be treated the same as Safe here).

Suggested change
# Skip vuln lookup when an explicit check result indicates the target
# is not vulnerable. Only key off check_code — fail_reason alone is
# Skip vuln lookup when an explicit check result says the target is
# safe or otherwise does not establish vulnerability (for example,
# CheckCode::Unknown). Only key off check_code — fail_reason alone is

Copilot uses AI. Check for mistakes.
@adfoster-r7 adfoster-r7 force-pushed the improve-vuln-and-vuln-attempt-tracking branch from 5d0d0ab to ccfc94f Compare April 24, 2026 10:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.

Comment on lines +176 to +185
# Skip vuln lookup when an explicit check result says the target is
# safe or the check could not establish vulnerability (Unknown).
# Only key off check_code — fail_reason alone is too broad
# (e.g. Failure::Unknown covers real exploit failures too).
safe_check_codes = [Msf::Exploit::CheckCode::Safe.code, Msf::Exploit::CheckCode::Unknown.code]
unless safe_check_codes.include?(opts[:check_code])
# Try find a matching vulnerability
vuln = find_vuln_by_refs(ref_objs, host, svc, false)
vuln ||= find_vuln_by_refs(ref_objs, host, nil, false) if svc && vuln.nil?
end
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

Same issue as above: safe_check_codes excludes Msf::Exploit::CheckCode::Detected, so an AutoCheck Detected result can still cause this method to look up and attach to an existing vuln by refs. Consider adding detected to the skip list (or switching to an allow-list of positive check codes) to avoid misattributing attempts to vulns.

Copilot uses AI. Check for mistakes.
Comment on lines +83 to +90
if $1.to_i > 5
return CheckCode::Safe
return CheckCode::Safe("PHP version #{php_version} is not vulnerable. Only PHP <= 5.4.6 supports preg_replace /e modifier.")
else
if $1.to_i == 5 and $2.to_i > 4
return CheckCode::Safe
return CheckCode::Safe("PHP version #{php_version} is not vulnerable. Only PHP <= 5.4.6 supports preg_replace /e modifier.")
else
if $1.to_i == 5 and $2.to_i == 4 and $3.to_i > 6
return CheckCode::Safe
return CheckCode::Safe("PHP version #{php_version} is not vulnerable. Only PHP <= 5.4.6 supports preg_replace /e modifier.")
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

The new Safe reason text claims only PHP <= 5.4.6 supports the preg_replace /e modifier. The /e modifier was deprecated in PHP 5.5 but remained supported until PHP 7.0, so this message is inaccurate and may mislead operators (and suggests the Safe gating logic might be too strict). Please adjust the wording (and, if needed, the version check) to reflect the actual /e support window.

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +87
# Skip vuln lookup when an explicit check result says the target is
# safe or the check could not establish vulnerability (Unknown).
# Only key off check_code — fail_reason alone is too broad
# (e.g. Failure::Unknown covers real exploit failures too).
safe_check_codes = [Msf::Exploit::CheckCode::Safe.code, Msf::Exploit::CheckCode::Unknown.code]
unless safe_check_codes.include?(opts[:check_code])
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

safe_check_codes excludes Msf::Exploit::CheckCode::Detected. With AutoCheck, Detected explicitly proceeds to exploitation without reporting a vuln; if the exploit later fails and there is an existing vuln with matching refs, this code can still associate a VulnAttempt to that vuln even though the check did not indicate vulnerability. Consider treating detected like safe/unknown here (skip vuln lookup), or alternatively only perform vuln lookup when check_code is appears/vulnerable.

Suggested change
# Skip vuln lookup when an explicit check result says the target is
# safe or the check could not establish vulnerability (Unknown).
# Only key off check_code — fail_reason alone is too broad
# (e.g. Failure::Unknown covers real exploit failures too).
safe_check_codes = [Msf::Exploit::CheckCode::Safe.code, Msf::Exploit::CheckCode::Unknown.code]
unless safe_check_codes.include?(opts[:check_code])
# Only perform vuln lookup when the explicit check result indicates
# vulnerability. Results like Safe, Unknown, and Detected should not
# associate this exploit attempt with an existing vulnerability.
# Only key off check_code — fail_reason alone is too broad
# (e.g. Failure::Unknown covers real exploit failures too).
allowed_check_codes = [Msf::Exploit::CheckCode::Appears.code, Msf::Exploit::CheckCode::Vulnerable.code]
if allowed_check_codes.include?(opts[:check_code])

Copilot uses AI. Check for mistakes.
@adfoster-r7 adfoster-r7 force-pushed the improve-vuln-and-vuln-attempt-tracking branch from ccfc94f to feb5cf3 Compare April 24, 2026 13:05
@adfoster-r7 adfoster-r7 requested a review from Copilot April 24, 2026 14:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.

Comment thread lib/msf/core/module/failure.rb Outdated
Comment on lines +175 to +179
# Narrow by port when available so we don't match an attempt against
# a different service on the same host (e.g. port 80 vs 9200).
if info[:port]
scope = scope.joins(vuln: :service)
.where(services: { port: info[:port] })
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

When re-querying to enrich an existing VulnAttempt, the scope is narrowed only by services.port. On hosts that have both TCP and UDP services on the same port (or other same-port scenarios), this can match and update the wrong attempt. Since info[:proto] is already available, it should be included in the service filter (and/or otherwise disambiguated) when present.

Suggested change
# Narrow by port when available so we don't match an attempt against
# a different service on the same host (e.g. port 80 vs 9200).
if info[:port]
scope = scope.joins(vuln: :service)
.where(services: { port: info[:port] })
# Narrow by service attributes when available so we don't match an
# attempt against a different service on the same host (e.g. port 80
# vs 9200, or TCP vs UDP on the same port).
if info[:port]
service_conditions = { port: info[:port] }
service_conditions[:proto] = info[:proto].to_s.downcase if info[:proto]
scope = scope.joins(vuln: :service)
.where(services: service_conditions)

Copilot uses AI. Check for mistakes.
if opts[:check_code].nil? || vuln_check_codes.include?(opts[:check_code])
# Try find a matching vulnerability
vuln = find_vuln_by_refs(ref_objs, host, svc, false)
vuln ||= find_vuln_by_refs(ref_objs, host, nil, false) if svc && vuln.nil?
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

Same issue as above in do_report_failure_or_success: falling back to find_vuln_by_refs(ref_objs, host, nil, ...) when a service-scoped lookup fails can match an unrelated vuln on another service for the same host. Consider restricting this fallback to vulns with service_id NULL (or otherwise explicitly matching only the "no service" case) so attempts don’t get attached to the wrong vuln.

Suggested change
vuln ||= find_vuln_by_refs(ref_objs, host, nil, false) if svc && vuln.nil?
if svc && vuln.nil?
fallback_vuln = find_vuln_by_refs(ref_objs, host, nil, false)
vuln = fallback_vuln if fallback_vuln && fallback_vuln.service_id.nil?
end

Copilot uses AI. Check for mistakes.
@adfoster-r7 adfoster-r7 force-pushed the improve-vuln-and-vuln-attempt-tracking branch from feb5cf3 to 4420285 Compare April 24, 2026 15:18
@adfoster-r7 adfoster-r7 force-pushed the improve-vuln-and-vuln-attempt-tracking branch from 4420285 to e00515c Compare April 24, 2026 15:27
@adfoster-r7 adfoster-r7 requested a review from Copilot April 24, 2026 15:27
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.

when Msf::Exploit::CheckCode::Safe.code
NotVulnerable
when Msf::Exploit::CheckCode::Detected.code, Msf::Exploit::CheckCode::Unknown.code
Unknown
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

fail_reason_from_check_code doesn’t handle Msf::Exploit::CheckCode::Unsupported. When a module’s check returns Unsupported (a common outcome), report_failure will keep the existing fail_reason (often none) instead of reflecting a configuration/unsupported condition. Consider mapping Unsupported to an appropriate failure constant (e.g. BadConfig) and adding a spec case for it.

Suggested change
Unknown
Unknown
when Msf::Exploit::CheckCode::Unsupported.code
BadConfig

Copilot uses AI. Check for mistakes.
@adfoster-r7 adfoster-r7 marked this pull request as ready for review April 24, 2026 15:41
@github-project-automation github-project-automation Bot moved this from Ready to In Progress in Metasploit Kanban Apr 24, 2026
@adfoster-r7 adfoster-r7 merged commit d121ff6 into rapid7:master Apr 24, 2026
65 checks passed
@adfoster-r7 adfoster-r7 deleted the improve-vuln-and-vuln-attempt-tracking branch April 24, 2026 17:36
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Metasploit Kanban Apr 24, 2026
@adfoster-r7 adfoster-r7 restored the improve-vuln-and-vuln-attempt-tracking branch April 25, 2026 09:42
@adfoster-r7
Copy link
Copy Markdown
Contributor Author

Release Notes

Improves vulnerability and vulnerability attempt tracking in Metasploit. Now additional details are registered - giving operators richer context when reviewing discovered vulnerabilities.

@adfoster-r7 adfoster-r7 added the rn-enhancement release notes enhancement label Apr 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rn-enhancement release notes enhancement

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants