Add human-readable descriptions to CheckCode returns in modules#21359
Add human-readable descriptions to CheckCode returns in modules#21359adfoster-r7 wants to merge 1 commit intorapid7:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request standardizes Metasploit module check results by adding human-readable description strings to Exploit::CheckCode/CheckCode returns so that status/details bubble up to users more consistently.
Changes:
- Adds descriptive messages to
Safe/Detected/Appears/Vulnerable/Unknowncheck results across many exploit modules. - Makes several checks report version-based context in the returned
CheckCodemessage. - Aligns modules with the ongoing effort from #21304 to improve check output quality.
Reviewed changes
Copilot reviewed 112 out of 112 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/exploits/unix/webapp/zpanel_username_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/zoneminder_snapshots.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/zoneminder_packagecontrol_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/zimbra_lfi.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/zeroshell_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/xymon_useradm_cmd_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/xoda_file_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_total_cache_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/wp_property_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_plainview_activity_monitor_rce.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_pixabay_images_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_phpmailer_host_header.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/wp_optimizepress_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_infusionsoft_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_google_document_embedder_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_foxypress_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_asset_manager_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_advanced_custom_fields_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/wp_admin_shell_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/webtester_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/webmin_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/webmin_show_cgi_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/vicidial_user_authorization_unauth_cmd_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/vicidial_manager_send_cmd_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/vicidial_agent_authenticated_rce.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/vbulletin_vote_sqli_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/twiki_search.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/twiki_maketext.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/twiki_history.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/tuleap_unserialize_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/tuleap_rest_unserialize_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/trixbox_langchoice.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/trixbox_ce_endpoint_devicemap_rce.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/tikiwiki_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/tikiwiki_jhot_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/tikiwiki_graph_formula_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/squash_yaml_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/sphpblog_file_upload.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/skybluecanvas_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/sixapart_movabletype_storable_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/simple_e_document_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/seportal_sqli_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/rconfig_install_cmd_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/projectsend_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/projectpier_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/phpmyadmin_config.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/phpcollab_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/php_xmlrpc_eval.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/php_vbulletin_template.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/php_include.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/php_eval.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/php_charts_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/oracle_vm_agent_utl.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/openx_banner_edit.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/opensis_modname_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/opensis_chain_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/opennetadmin_ping_cmd_injection.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/openmediavault_rpc_rce.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/openemr_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/openemr_sqli_privesc_upload.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/open_flash_chart_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/nextcloud_workflows_rce.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/narcissus_backend_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/nagios_graph_explorer.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/nagios3_history_cgi.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/mybb_backdoor.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/moinmoin_twikidraw.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/maarch_letterbox_file_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/libretto_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/kimai_sqli.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/jquery_file_upload.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/joomla_tinybrowser.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/joomla_media_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/joomla_contenthistory_sqli_rce.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/joomla_comjce_imgmanager.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/joomla_comfields_sqli_rce.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/joomla_akeeba_unserialize.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/invision_pboard_unserialize_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/instantcms_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/hybridauth_install_php_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/horde_unserialize_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/havalite_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/hastymail_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/graphite_pickle_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/google_proxystylesheet_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/get_simple_cms_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/fusionpbx_operator_panel_exec_cmd_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/fusionpbx_exec_cmd_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/freepbx_config_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/foswiki_maketext.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/flashchat_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/elfinder_php_connector_exiftran_cmd_injection.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/egallery_upload_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/drupal_restws_unserialize.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/drupal_restws_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/drupal_drupalgeddon2.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/drupal_coder_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/dogfood_spell_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/datalife_preview_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/coppermine_piceditor.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/clipbucket_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/citrix_access_gateway_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/carberp_backdoor_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/byob_unauth_rce.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/basilic_diff_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/barracuda_img_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/awstatstotals_multisort.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/awstats_migrate_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/awstats_configdir_exec.rb | Adds descriptive strings to check return codes. |
| modules/exploits/unix/webapp/arkeia_upload_exec.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/ajenti_auth_username_cmd_injection.rb | Adds descriptive strings to check return codes (version-context messaging). |
| modules/exploits/unix/webapp/actualanalyzer_ant_cookie_exec.rb | Adds descriptive strings to check return codes. |
| flow_id = create_workflow('sleep 1') | ||
|
|
||
| Exploit::CheckCode::Safe('Target is not vulnerable') if flow_id.nil? | ||
|
|
||
| delete_workflow(flow_id) | ||
| Exploit::CheckCode::Vulnerable | ||
| Exploit::CheckCode::Vulnerable('The target is vulnerable') | ||
| end |
There was a problem hiding this comment.
Exploit::CheckCode::Safe(...) if flow_id.nil? doesn't return, so the method continues, calls delete_workflow(nil), and then returns Vulnerable even when flow_id wasn't created. This should be return Exploit::CheckCode::Safe(...) if flow_id.nil? (or an Unknown/Detected result if workflow creation failed for another reason).
| ) | ||
|
|
||
| if res and res.code == 500 or res.body =~ /PHP_Incomplete_Class/ | ||
| return Exploit::CheckCode::Vulnerable | ||
| return Exploit::CheckCode::Vulnerable('The target is vulnerable') | ||
| elsif res and res.code == 200 | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe('The target is not vulnerable') |
There was a problem hiding this comment.
The condition if res and res.code == 500 or res.body =~ /PHP_Incomplete_Class/ can evaluate res.body even when res is nil (due to and/or precedence), which would raise a NoMethodError during check. Consider rewriting as if res && (res.code == 500 || res.body =~ /PHP_Incomplete_Class/) (and guarding res.body).
| def check | ||
| response, version = find_setup_path(mode: :check) | ||
| return Exploit::CheckCode::Safe unless response&.code == 200 | ||
| return Exploit::CheckCode::Safe("Version #{version} is not vulnerable") unless response&.code == 200 | ||
|
|
There was a problem hiding this comment.
If find_setup_path can't find a setup page (or can't extract a version), version may be nil but this returns Safe("Version #{version} is not vulnerable"), producing a confusing message. Consider returning a message that reflects the real condition (e.g., "phpMyAdmin setup page not found" / "Could not determine version") and avoid interpolating version unless present.
| @@ -76,12 +76,12 @@ def check | |||
| version = res.body.scan(/>HybridAuth (2\.[012]\.[\d\.]+(-dev)?) Installer</).first.first | |||
| vprint_status "Found version: #{version}" | |||
| if version =~ /^2\.(0\.(9|10|11)|1\.[\d]+|2\.[012])/ | |||
| return Exploit::CheckCode::Vulnerable | |||
| return Exploit::CheckCode::Vulnerable("Detected vulnerable version #{version}") | |||
| else | |||
| vprint_error "HybridAuth version #{version} is not vulnerable" | |||
| end | |||
| end | |||
| Exploit::CheckCode::Safe | |||
| Exploit::CheckCode::Safe("Version #{version} is not vulnerable") | |||
| end | |||
There was a problem hiding this comment.
version is only set when the installer page matches the version regex, but the method always falls through to Safe("Version #{version} is not vulnerable"). In most non-vulnerable cases version will be nil here (404 / not writable / unrecognized response), resulting in "Version is not vulnerable". Consider returning more specific Safe/Detected/Unknown results for each branch and only include the version in the message when it's actually extracted.
| res = send_request(rev_url + Rex::Text.uri_encode(rev)) | ||
| if (not res) or (res.code != 200) | ||
| vprint_warning("Error with exploit request (HTTP #{res.code}, should be 200)") unless res.code == 200 | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe('The target is not vulnerable') |
There was a problem hiding this comment.
In the (not res) || (res.code != 200) branch, res can be nil but the warning still calls res.code, which will raise a NoMethodError during check. Consider handling the nil-response case separately before referencing res.code.
| elsif res.body =~ /Kimai/ and res.body =~ /(0\.9\.[\d\.]+)<\/strong>/ | ||
| version = "#{$1}" | ||
| print_good("Found version: #{version}") | ||
| if version >= "0.9.2" and version <= "0.9.2.1306" | ||
| return Exploit::CheckCode::Appears | ||
| return Exploit::CheckCode::Appears("Version #{version} appears to be vulnerable") | ||
| end | ||
| end | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe("Version #{version} is not vulnerable") | ||
| end |
There was a problem hiding this comment.
version is only set when the response matches the Kimai version regex; otherwise it's nil but the method still returns Safe("Version #{version} is not vulnerable"), resulting in an empty version in the output. Consider returning a generic Safe('The target is not vulnerable') when the version can't be determined, or explicitly set a fallback value/message.
| if (res and res.body =~ /Simple PHP Blog (\d)\.(\d)\.(\d)/) | ||
|
|
||
| ver = [ $1.to_i, $2.to_i, $3.to_i ] | ||
| vprint_status("Simple PHP Blog #{ver.join('.')}") | ||
|
|
||
| if (ver[0] == 0 and ver[1] < 5) | ||
| if (ver[1] == 4 and ver[2] > 0) | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe("Version #{ver} is not vulnerable") | ||
| end | ||
|
|
||
| return Exploit::CheckCode::Appears | ||
| return Exploit::CheckCode::Appears("Version #{ver} appears to be vulnerable") | ||
| end | ||
| end | ||
|
|
||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe("Version #{ver} is not vulnerable") | ||
| end |
There was a problem hiding this comment.
ver is an array (e.g., [0, 4, 0]) and may be nil when the version regex doesn't match, but it's interpolated directly in both Safe and Appears messages. This produces outputs like "Version [0, 4, 0] ..." or "Version is not vulnerable". Consider using ver.join('.') for messages and falling back to a generic Safe message when the version can't be parsed.
| version = get_target(res) | ||
| if version.nil? | ||
| # We don't print out an error message here as returning this will | ||
| # automatically cause Metasploit to print out an appropriate error message. | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe("Version #{version} is not vulnerable") | ||
| end |
There was a problem hiding this comment.
When get_target returns nil, this returns Safe("Version #{version} is not vulnerable") where version is nil, resulting in a misleading message. Consider returning a generic Safe('The target is not vulnerable') (or Detected with a "version unsupported"/"unknown" message) in this branch.
| if (not res) or (res.code != 200) | ||
| vprint_warning("Error with exploit request (HTTP #{res.code}, should be 200)") unless res.code == 200 | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe('The target is not vulnerable') |
There was a problem hiding this comment.
In the (not res) || (res.code != 200) branch, res can be nil but the warning still calls res.code, which will raise a NoMethodError during check. Consider guarding the log message (only reference res.code when res is present) or restructuring to handle the nil-response case separately.
| if res.body and (res.body =~ /Performance optimized by W3 Total Cache/ or res.body =~ /Cached page generated by WP-Super-Cache/) | ||
| return Exploit::CheckCode::Detected | ||
| return Exploit::CheckCode::Detected("Detected version #{version}") | ||
| end | ||
|
|
||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe("Version #{version} is not vulnerable") | ||
| end |
There was a problem hiding this comment.
version is only assigned inside the X-Powered-By header branch, but it's referenced in the later Detected/Safe returns. When the header isn't present, this returns messages like "Detected version " / "Version is not vulnerable". Consider avoiding version in those messages unless it was actually extracted, or set a separate message when W3 Total Cache is only detected via body markers.
adfoster-r7
force-pushed
the
improve-checkcode-messages-12
branch
from
0a7f161 to
f23d3bf
Compare
adfoster-r7
force-pushed
the
improve-checkcode-messages-12
branch
2 times, most recently
from
082ef65 to
50ce997
Compare
| rescue ::Rex::ConnectionRefused, ::Rex::HostUnreachable, ::Rex::ConnectionTimeoutp | ||
| vprint_error("Connection failed") | ||
| return Exploit::CheckCode::Unknown | ||
| return Exploit::CheckCode::Unknown('Could not determine the target status') |
There was a problem hiding this comment.
Critical: Problem: rescue references ::Rex::ConnectionTimeoutp, which does not exist and will prevent timeout errors from being caught. Impact: check may raise unexpectedly instead of returning Unknown, breaking module behavior on network timeouts. Fix: change it to ::Rex::ConnectionTimeout (matching the rest of the module).
| end | ||
|
|
||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe("Version #{version} is not vulnerable") |
There was a problem hiding this comment.
Important: Problem: when version is nil, return Exploit::CheckCode::Safe("Version #{version} is not vulnerable") produces an unhelpful message like "Version is not vulnerable". Impact: users get confusing check output when the setup page is found but the version cannot be determined. Fix: only include the version in the message when it is present, otherwise return a generic Safe/Detected message that does not interpolate nil.
adfoster-r7
force-pushed
the
improve-checkcode-messages-12
branch
from
50ce997 to
0bf595c
Compare
| res = login(user, pass) | ||
| unless res | ||
| print_error("No response was received from #{peer} whilst in check(), check it is online and the target port is open!") | ||
| return CheckCode::Detected | ||
| return CheckCode::Detected('The target service was detected') | ||
| end |
There was a problem hiding this comment.
Important: Problem: when login returns no response, check prints an error but returns CheckCode::Detected('The target service was detected') (line 147), which contradicts the condition and misreports status. Impact: users may see a false-positive detection even though connectivity is unknown. Fix: return CheckCode::Unknown (or similar) with a message reflecting the connection failure instead of Detected.
| res = execute_command(cmd) | ||
| t2 = Time.now.to_i | ||
| unless res | ||
| print_error("#{peer} - Connection failed whilst trying to perform the code injection.") | ||
| return CheckCode::Detected | ||
| return CheckCode::Detected("Detected version #{version}") | ||
| end |
There was a problem hiding this comment.
Important: Problem: on a nil response from execute_command (connection failed during the injection verification), check returns CheckCode::Detected("Detected version #{version}") even though vulnerability could not be verified. Impact: callers may treat this as a positive detection rather than an indeterminate check due to connectivity issues. Fix: return CheckCode::Unknown(...) (optionally including the detected version in the message) when the verification request fails.
Improves multiple module check code messages and statuses
This metadata is currently missing in modules, which means the bubbling up of results to users is often missing
Continuation of #21304
Verification