Add human-readable descriptions to CheckCode returns in modules#21352
Add human-readable descriptions to CheckCode returns in modules#21352adfoster-r7 wants to merge 1 commit intorapid7:masterfrom
Conversation
|
Thanks for your pull request! As part of our landing process, we manually verify that all modules work as expected. We've added the |
There was a problem hiding this comment.
Pull request overview
This PR adds human-readable description strings to CheckCode return values across a broad set of exploit modules so module check results bubble up to users with clearer context.
Changes:
- Add descriptive messages to
CheckCode::{Safe,Detected,Appears,Vulnerable,Unknown}returns across many modules. - Make “inconclusive” paths (e.g., missing version info / no reply) more explicit via messages, improving operator feedback.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/exploits/linux/upnp/miniupnpd_soap_bof.rb | Adds check result messages for connection failure, fingerprint detection, and unknown state. |
| modules/exploits/linux/upnp/dlink_upnp_msearch_exec.rb | Adds a descriptive Detected message when product info is unavailable. |
| modules/exploits/linux/upnp/belkin_wemo_upnp_exec.rb | Adds Unknown/Detected/Appears/Safe check descriptions based on device/version parsing. |
| modules/exploits/linux/telnet/netgear_telnetenable.rb | Adds descriptions to Detected/Unknown check outcomes. |
| modules/exploits/linux/ssh/vyos_restricted_shell_privesc.rb | Adds descriptive Safe messages for SSH connection/auth failure cases. |
| modules/exploits/linux/ssh/ssh_erlangotp_rce.rb | Adds a descriptive Vulnerable message. |
| modules/exploits/linux/snmp/awind_snmp_exec.rb | Adds descriptive Vulnerable/Safe/Unknown check outcomes based on SNMP sysDescr parsing. |
| modules/exploits/linux/smtp/exim_gethostbyname_bof.rb | Adds descriptions for Unknown/Vulnerable/Safe check paths. |
| modules/exploits/linux/smtp/apache_james_exec.rb | Adds descriptions to Safe and Appears version comparisons. |
| modules/exploits/linux/samba/setinfopolicy_heap.rb | Adds descriptions to Appears/Safe/Unknown based on Samba version parsing. |
| modules/exploits/linux/samba/lsa_transnames_heap.rb | Adds descriptive Detected/Safe results and a rescue-path description. |
| modules/exploits/linux/samba/is_known_pipename.rb | Adds descriptive Safe messages for non-Samba and patched/out-of-range versions. |
| modules/exploits/linux/redis/redis_replication_cmd_exec.rb | Adds descriptions for Safe results when CONFIG/version checks indicate non-vulnerable. |
| modules/exploits/linux/pptp/poptop_negative_read.rb | Adds descriptive Detected/Safe results for PoPToP detection. |
| modules/exploits/linux/postgres/postgres_payload.rb | Adds description to the authenticated Appears result. |
| modules/exploits/linux/misc/zyxel_multiple_devices_zhttp_lan_rce.rb | Adds descriptive Vulnerable/Safe messages for traversal check results. |
| modules/exploits/linux/misc/zyxel_ike_decoder_rce_cve_2023_28771.rb | Adds a descriptive Unknown message when IKE isn’t detected. |
| modules/exploits/linux/misc/zabbix_server_exec.rb | Adds descriptive Vulnerable/Safe messages for check command execution paths. |
| modules/exploits/linux/misc/unidata_udadmin_password_stack_overflow.rb | Adds a descriptive Detected message including version. |
| modules/exploits/linux/misc/unidata_udadmin_auth_bypass.rb | Adds a descriptive Detected message including version. |
| modules/exploits/linux/misc/ueb9_bpserverd.rb | Adds descriptive Detected/Safe messages for port validation-based detection. |
| modules/exploits/linux/misc/tplink_archer_a7_c7_lan_rce.rb | Adds descriptive Vulnerable/Safe/Unknown messages for manifest checks. |
| modules/exploits/linux/misc/sercomm_exec.rb | Adds descriptive Appears/Safe messages for endianness/backdoor detection. |
| modules/exploits/linux/misc/qnap_transcode_server.rb | Adds descriptive Safe/Detected/Unknown messages for response/connection cases. |
| modules/exploits/linux/misc/novell_edirectory_ncp_bof.rb | Adds descriptive Safe/Detected messages for NCP response validation. |
| modules/exploits/linux/misc/netcore_udp_53413_backdoor.rb | Adds descriptive Detected/Vulnerable/Safe messages for backdoor/echo behavior. |
| modules/exploits/linux/misc/nagios_nrpe_arguments.rb | Adds descriptive messages for Vulnerable/Safe/Unknown outcomes around argument handling. |
| modules/exploits/linux/misc/jenkins_ldap_deserialize.rb | Adds descriptive Safe/Vulnerable/Unknown messages for vulnerability probing. |
| modules/exploits/linux/misc/jenkins_java_deserialize.rb | Adds descriptive Safe/Vulnerable/Unknown messages for vulnerability probing. |
| modules/exploits/linux/misc/igel_command_injection.rb | Adds descriptive messages for Unknown/Appears/Safe firmware parsing/version range checks. |
| modules/exploits/linux/misc/hp_vsa_login_bof.rb | Adds descriptive Appears/Detected/Safe messages for response pattern checks. |
| modules/exploits/linux/misc/hp_nnmi_pmd_bof.rb | Adds descriptive Unknown/Detected messages for PMD probing outcomes. |
| modules/exploits/linux/misc/hid_discoveryd_command_blink_on_unauth_rce.rb | Adds descriptive messages for Unknown/Safe/Detected/Appears paths in UDP discovery/version logic. |
| modules/exploits/linux/misc/fortimanager_rce_cve_2024_47575.rb | Adds a descriptive message to the non-detected (Unknown) check outcome. |
| modules/exploits/linux/misc/cve_2021_38647_omigod.rb | Adds descriptive Unknown/Safe messages for response and parsing failures. |
| modules/exploits/linux/misc/cve_2020_13160_anydesk.rb | Adds descriptive Safe message when the service isn’t discovered. |
| modules/exploits/linux/misc/cisco_rv340_sslvpn.rb | Adds descriptive Detected/Unknown messages for login page detection. |
| modules/exploits/linux/misc/cisco_ios_xe_rce.rb | Adds a descriptive Safe message when version retrieval fails. |
| modules/exploits/linux/misc/aerospike_database_udf_cmd_exec.rb | Improves Safe wording and adds a descriptive Appears message. |
| modules/exploits/linux/imap/imap_uw_lsub.rb | Adds a descriptive Safe message when the target banner doesn’t match. |
| modules/exploits/linux/ids/alienvault_centerd_soap_exec.rb | Adds descriptive Safe message when version extraction fails. |
| modules/exploits/linux/ftp/proftp_telnet_iac.rb | Adds descriptive Safe/Appears messages for banner/version-based checks. |
| modules/exploits/linux/ftp/proftp_sreplace.rb | Adds descriptive Safe/Appears messages for banner/version-based checks. |
| modules/exploits/linux/antivirus/escan_password_exec.rb | Adds descriptive Detected/Unknown messages for UI detection. |
| rescue ::Rex::ConnectionError | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe('Connection failed') | ||
| end |
There was a problem hiding this comment.
rescue ::Rex::ConnectionError returning Exploit::CheckCode::Safe('Connection failed') is misleading: a connection failure means the check couldn't be performed, not that the target is safe. Consider returning CheckCode::Unknown (or at least adjust the message to reflect inability to check).
| rescue Rex::ConnectionError | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe('Connection to SSH service failed') | ||
| rescue Net::SSH::Disconnect, ::EOFError | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe('SSH connection was disconnected') | ||
| rescue Timeout::Error | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe('SSH connection timed out') | ||
| rescue Net::SSH::AuthenticationFailed | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe('SSH authentication failed') | ||
| rescue Net::SSH::Exception | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe('SSH connection encountered an error') | ||
| end |
There was a problem hiding this comment.
All of these exception cases return CheckCode::Safe(...), but they represent inability to connect/authenticate rather than evidence the target is not vulnerable. Returning CheckCode::Unknown (with the same messages) would better reflect the check outcome and avoid false negatives.
| if res.blank? | ||
| vprint_status 'No reply from server' | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe('No reply from transcode server') |
There was a problem hiding this comment.
res.blank? (no reply) currently returns CheckCode::Safe('No reply from transcode server'). A missing response generally means the check was inconclusive (network drop/service down), so this should be CheckCode::Unknown to avoid reporting Safe on an unreachable target.
| return CheckCode::Safe('No reply from transcode server') | |
| return CheckCode::Unknown('No reply from transcode server') |
| end | ||
| else # No response | ||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe('No response from Zabbix server') |
There was a problem hiding this comment.
In the no-response case, returning Exploit::CheckCode::Safe('No response from Zabbix server') can produce false negatives (no response != not vulnerable). Prefer CheckCode::Unknown for the res.nil? path so unreachable/filtered targets are reported as inconclusive.
| return Exploit::CheckCode::Safe('No response from Zabbix server') | |
| return Exploit::CheckCode::Unknown('No response from Zabbix server') |
| end | ||
|
|
||
| CheckCode::Unknown | ||
| CheckCode::Unknown('Target does not appear to be running an IKE service') |
There was a problem hiding this comment.
This returns CheckCode::Unknown(...), but the message states definitively that the target is not running IKE. Since a lack of a matching reply can also be caused by filtering/timeouts, consider rewording to something like "No IKE response received" (or similar) so the message matches the uncertainty of Unknown.
| CheckCode::Unknown('Target does not appear to be running an IKE service') | |
| CheckCode::Unknown('No matching IKE response received; target service could not be confirmed.') |
| return CheckCode::Safe("Crestron Electronics model #{sys_description} is not a known vulnerable model") | ||
| end |
There was a problem hiding this comment.
The Safe-path message uses sys_description (full sysDescr) but labels it as a "model" ("Crestron Electronics model #{sys_description}..."). This is confusing and can produce very long/unstructured output; consider either using the extracted model value or rewording to reference sys_description explicitly.
| CheckCode::Safe('Target does not appear to be running Samba') | ||
| rescue StandardError | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe('Could not connect to target') | ||
| end |
There was a problem hiding this comment.
rescue StandardError returning CheckCode::Safe('Could not connect to target') conflates connectivity failures with a non-vulnerable determination. This should be CheckCode::Unknown (or similar) so a transient connection issue doesn't get reported as Safe.
| end | ||
|
|
||
| return CheckCode::Safe if version.blank? | ||
| return CheckCode::Safe('Could not determine AlienVault version') if version.blank? |
There was a problem hiding this comment.
When the version cannot be extracted (version.blank?), returning CheckCode::Safe('Could not determine AlienVault version') is contradictory and can hide vulnerable targets. This should return CheckCode::Unknown (or reword + keep Safe only if you're intentionally assuming not vulnerable).
| return CheckCode::Safe('Could not determine AlienVault version') if version.blank? | |
| return CheckCode::Unknown('Could not determine AlienVault version') if version.blank? |
| return CheckCode::Detected('Detected Fortinet FortiManager') if organization == 'Fortinet' && common_name&.start_with?('FMG') | ||
|
|
||
| CheckCode::Unknown | ||
| CheckCode::Unknown('Target does not appear to be a Fortinet FortiManager') |
There was a problem hiding this comment.
This path returns CheckCode::Unknown, but the message asserts the target is not FortiManager. If you want to keep Unknown (e.g., cert parsing heuristics may be inconclusive), reword to reflect uncertainty ("Could not confirm target is FortiManager"); otherwise consider returning CheckCode::Safe if the intent is a negative identification.
| CheckCode::Unknown('Target does not appear to be a Fortinet FortiManager') | |
| CheckCode::Unknown('Could not confirm target is a Fortinet FortiManager') |
| # from now on, an exception means vulnerable | ||
| check_code = Exploit::CheckCode::Vulnerable | ||
| check_code = Exploit::CheckCode::Vulnerable('Exim gethostbyname buffer overflow triggered') | ||
| # raise an exception if no valid SMTP reply | ||
| reply = smtp_recv(ANY_CODE) | ||
| # can't determine vulnerable state if smtp_verify_helo() isn't called | ||
| return Exploit::CheckCode::Unknown if reply[:code] !~ /#{HELO_CODES}/ | ||
| return Exploit::CheckCode::Unknown('Could not determine vulnerability status') if reply[:code] !~ /#{HELO_CODES}/ |
There was a problem hiding this comment.
check_code is set to CheckCode::Vulnerable('Exim gethostbyname buffer overflow triggered') before validating the subsequent SMTP replies, and a later generic rescue will return that value for any exception (including non-vulnerability-related network errors). Consider narrowing the rescue/exception handling or using a less definitive message (or Unknown) unless you can confirm the crash/trigger condition.
adfoster-r7
force-pushed
the
improve-checkcode-messages-5
branch
from
b6c0b8b to
f5f1d9b
Compare
| # TODO: insert description check for other vulnerable models (that I don't have) | ||
| # In the meantime, we return 'safe'. | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe("Crestron Electronics model #{model} is not a known vulnerable model") | ||
| end |
There was a problem hiding this comment.
model can be nil here when the sysDescr doesn’t match AM-100/AM-101, which results in a user-facing message like "Crestron Electronics model is not a known vulnerable model". Consider either handling the nil case explicitly (e.g., "non-AM-100/AM-101 Crestron device" / "unknown model") or returning CheckCode::Unknown when the model can’t be identified from sys_description.
| # check if it's a valid port number (1-65534) | ||
| if bpd_port && bpd_port >= 1 && bpd_port <= 65535 | ||
| Exploit::CheckCode::Detected | ||
| Exploit::CheckCode::Detected('UEB bpserverd service detected') | ||
| else | ||
| Exploit::CheckCode::Safe | ||
| Exploit::CheckCode::Safe('Target does not appear to be running UEB bpserverd') |
There was a problem hiding this comment.
check establishes a TCP connection (connect(global = false)) but never calls disconnect on the returned socket. This can leak sockets/file descriptors when check is run repeatedly. Consider wrapping the socket usage in an ensure block and calling disconnect(s1) (or s1.close) before returning the CheckCode.
| rescue StandardError | ||
| return CheckCode::Safe | ||
| return CheckCode::Unknown('Could not connect to target') | ||
| end |
There was a problem hiding this comment.
The rescue StandardError now always returns CheckCode::Unknown('Could not connect to target'), but exceptions from smb_login can also be caused by authentication/SMB negotiation issues or unexpected replies (not strictly connectivity). Consider rescuing more specific connection-related exceptions, or including the exception class/message in the returned Unknown to avoid a misleading reason string.
adfoster-r7
force-pushed
the
improve-checkcode-messages-5
branch
2 times, most recently
from
4d9db38 to
92660b6
Compare
|
|
||
| def check | ||
| return Exploit::CheckCode::Unknown, 'Login failed, please check credentials' unless login(datastore['EMAIL'], datastore['PASSWORD']) | ||
| return Exploit::CheckCode::Unknown('Could not determine the target status'), 'Login failed, please check credentials' unless login(datastore['EMAIL'], datastore['PASSWORD']) |
There was a problem hiding this comment.
This check return mixes the two supported patterns: it returns multiple values and embeds a message inside the CheckCode::Unknown(...) object. This can lead to inconsistent user-facing output (which message gets displayed depends on the caller). Consider returning a single CheckCode::Unknown('Login failed, please check credentials'), or returning Exploit::CheckCode::Unknown, 'Login failed, please check credentials' (but not both).
| else | ||
| return Exploit::CheckCode::Detected | ||
| return Exploit::CheckCode::Detected("Target detected: version #{version}") | ||
| end |
There was a problem hiding this comment.
When the version can't be extracted, version is still nil here, so the message becomes "Target detected: version ". Consider changing the description to indicate that Pandora was detected but the version could not be determined (and avoid interpolating version when it's nil/blank).
| else | ||
| print_error('Router is not a NETGEAR router') | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe("Version #{model} is not vulnerable") |
There was a problem hiding this comment.
In this branch model is not set (it will be nil), so this message can become "Version is not vulnerable" when the target isn't a NETGEAR router. Consider returning a message that doesn't reference model here (e.g., indicating the service wasn't detected) or set model before this branch.
| return CheckCode::Safe("Version #{model} is not vulnerable") | |
| return CheckCode::Safe('NETGEAR router not detected') |
| return CheckCode::Safe(version[2].chop.to_s) | ||
| end | ||
| CheckCode::Safe | ||
| CheckCode::Safe('Unable to parse target version from response') |
There was a problem hiding this comment.
This branch indicates the module couldn't parse the target version, but it still returns CheckCode::Safe, which implies the target is not vulnerable (a false-negative classification). Since the status is indeterminate here, consider returning CheckCode::Unknown(...) instead of Safe when version parsing fails.
| CheckCode::Safe('Unable to parse target version from response') | |
| CheckCode::Unknown('Unable to parse target version from response') |
| end | ||
|
|
||
| CheckCode::Safe | ||
| CheckCode::Safe("Version #{version} is not vulnerable") |
There was a problem hiding this comment.
version is initialized to nil, so this can end up returning "Version is not vulnerable" when the version string wasn't found in the response. Consider either returning CheckCode::Unknown('Could not determine the target version') when version is nil/blank, or using a message that doesn't interpolate an unset version.
|
|
||
| Exploit::CheckCode::Safe | ||
| Exploit::CheckCode::Safe("Version #{version} is not vulnerable") | ||
| end |
There was a problem hiding this comment.
This can return "Version is not vulnerable" when version was never discovered. Consider returning CheckCode::Unknown('Could not determine the target version') when version is nil/blank, or adjust the message to avoid referencing an unknown version.
| print_good("Router may be vulnerable (NETGEAR #{model})") | ||
| return CheckCode::Detected | ||
| return CheckCode::Detected("Target detected: version #{model}") | ||
| else |
There was a problem hiding this comment.
model contains the router model (e.g., R7000/R6400), not a version, so the description is misleading. Consider changing the message to reference the detected model (or vendor/product) rather than calling it a version.
| if version.to_s.eql? '' | ||
| vprint_error "Could not determine QNAP Q'Center appliance version" | ||
| return CheckCode::Detected | ||
| return CheckCode::Detected("Target detected: version #{version}") | ||
| end |
There was a problem hiding this comment.
When version can't be parsed, version is an empty string here, so the description becomes "Target detected: version ". Consider changing the description to indicate that Q'Center was detected but the version could not be determined (and avoid interpolating version when blank).
| end | ||
| end | ||
| CheckCode::Safe | ||
| CheckCode::Safe('Target version is not vulnerable') |
There was a problem hiding this comment.
If the module cannot parse a version from the response, returning CheckCode::Safe('Target version is not vulnerable') is misleading because the status is actually unknown. Consider returning CheckCode::Unknown('Unable to parse target version from response') (or similar) in this fallback path instead of Safe.
| CheckCode::Safe('Target version is not vulnerable') | |
| CheckCode::Unknown('Unable to parse target version from response') |
adfoster-r7
force-pushed
the
improve-checkcode-messages-5
branch
from
92660b6 to
28a0f9f
Compare
| end | ||
|
|
||
| CheckCode::Safe | ||
| CheckCode::Safe("Version #{version} is not vulnerable") |
There was a problem hiding this comment.
If the version marker regex doesn't match, version remains nil and this returns Safe("Version is not vulnerable"), which is misleading and may incorrectly report non-vulnerable when version detection failed. Consider returning Unknown (or Detected) when the version can't be extracted, and only include version in the message when present.
| CheckCode::Safe("Version #{version} is not vulnerable") | |
| CheckCode::Unknown('Oracle EBS detected, but the version could not be determined') |
| else | ||
| print_error('Router is not a NETGEAR router') | ||
| return CheckCode::Safe | ||
| return CheckCode::Safe("Version #{model} is not vulnerable") |
There was a problem hiding this comment.
In the non-NETGEAR-router branch, model is nil (it is only set inside the WWW-Authenticate header path), so this returns Safe("Version is not vulnerable") even though the router model wasn't detected. Consider returning a message like NETGEAR router not detected (or Unknown) instead of interpolating an unset model value.
| return CheckCode::Safe("Version #{model} is not vulnerable") | |
| return CheckCode::Unknown('NETGEAR router not detected') |
| if version and version >= "5" and version <= "5.0-1.07" | ||
| return Exploit::CheckCode::Appears | ||
| return Exploit::CheckCode::Appears("Version #{version} appears to be vulnerable") | ||
| end | ||
|
|
||
| return Exploit::CheckCode::Safe | ||
| return Exploit::CheckCode::Safe("Version #{version} is not vulnerable") | ||
| end |
There was a problem hiding this comment.
If the response doesn't contain the currentMutinyVersion marker, version stays nil and the method returns Safe("Version is not vulnerable"), which is misleading because version detection failed. Consider returning Unknown (or a Safe message that doesn't claim a specific version assessment) when version can't be extracted.
| begin | ||
| res = send_request_raw({ | ||
| 'method' => "GET", | ||
| 'uri' => "/zport/acl_users/cookieAuthHelper/login_form" | ||
| }) | ||
| return Exploit::CheckCode::Appears if res.body =~ /<p>Copyright © 2005-20[\d]{2} Zenoss, Inc\. \| Version\s+<span>3\./ | ||
| return Exploit::CheckCode::Detected if res.body =~ /<link rel="shortcut icon" type="image\/x\-icon" href="\/zport\/dmd\/favicon\.ico" \/>/ | ||
| return Exploit::CheckCode::Appears('The target appears to be vulnerable') if res.body =~ /<p>Copyright © 2005-20[\d]{2} Zenoss, Inc\. \| Version\s+<span>3\./ | ||
| return Exploit::CheckCode::Detected('The target service was detected') if res.body =~ /<link rel="shortcut icon" type="image\/x\-icon" href="\/zport\/dmd\/favicon\.ico" \/>/ | ||
|
|
||
| return Exploit::CheckCode::Safe | ||
| rescue ::Rex::ConnectionRefused, ::Rex::HostUnreachable, ::Rex::ConnectionTimeoutp | ||
| return Exploit::CheckCode::Safe('The target is not vulnerable') |
There was a problem hiding this comment.
send_request_raw can return nil on timeout/connection errors (it doesn't always raise). In that case, res.body will raise a NoMethodError here and skip the intended rescue path. Add a nil check for res before accessing res.body and return an Unknown/Detected CheckCode with a helpful message when no response is received.
| if (version = html.at('//input[@name = "version"]/@value')) | ||
| vprint_status("Nagios XI version: #{version}") | ||
| if Rex::Version.new(version) <= target[:version] | ||
| return CheckCode::Appears | ||
| return CheckCode::Appears("Version #{version} appears to be vulnerable") | ||
| end | ||
| end | ||
|
|
||
| CheckCode::Safe | ||
| CheckCode::Safe("Version #{version} is not vulnerable") | ||
| end |
There was a problem hiding this comment.
version is a Nokogiri attribute node (from html.at('//input.../@value')), so interpolating it in the Safe message can produce output like value="5.2.7" (or be nil if the input is missing). Consider extracting version = version&.value (or .text) and returning Unknown when it can't be determined, rather than reporting Version is not vulnerable.
adfoster-r7
force-pushed
the
improve-checkcode-messages-5
branch
from
28a0f9f to
2757120
Compare
|
|
||
| res = get_configuration | ||
| return CheckCode::Safe if res.nil? || res.code != 200 | ||
| return CheckCode::Safe('The target is not vulnerable') if res.nil? || res.code != 200 |
There was a problem hiding this comment.
check returns CheckCode::Safe('The target is not vulnerable') when get_configuration returns nil (no HTTP response) or a non-200 status. A nil response is an indeterminate connectivity/timeout case, so reporting Safe/not-vulnerable is misleading; consider returning CheckCode::Unknown (with a message like no response/unexpected HTTP code) for res.nil?, and reserve Safe for cases where you can positively determine the target is not vulnerable.
adfoster-r7
force-pushed
the
improve-checkcode-messages-5
branch
from
2757120 to
c9b4ac6
Compare
| print_error("#{datastore['USERNAME']} doesn't have the right to >>Package Update<<") | ||
| print_status("Please try with another user account!") | ||
| CheckCode::Safe | ||
| CheckCode::Safe('The target is not vulnerable') |
There was a problem hiding this comment.
check returns CheckCode::Safe('The target is not vulnerable') when the authenticated user lacks the "Package Updates" privilege. That result can be misleading because the target may still be vulnerable, but this account can't validate/exploit it. Consider returning CheckCode::Detected/CheckCode::Unknown with a reason like "Valid credentials, but user lacks Software Package Updates access" (and reserve Safe for cases where the target/version is confirmed not exploitable).
| elsif res.get_html_document.at('div[@id="footer-copyright"]').text.include? 'rConfig' | ||
| print_status('rConfig detected, but not version 3.9') | ||
| return Exploit::CheckCode::Detected | ||
| return Exploit::CheckCode::Detected('The target service was detected') | ||
| end | ||
| end |
There was a problem hiding this comment.
check can return nil if the response doesn't match either of the rConfig fingerprints (the method ends without a default CheckCode). This can lead to inconsistent framework behavior compared to returning an explicit status. Add an explicit fallback return (e.g., CheckCode::Safe('rConfig not detected') or CheckCode::Unknown('Unable to identify rConfig')) after the fingerprint checks.
| leak_admin_creds ? CheckCode::Vulnerable('The target is vulnerable') : CheckCode::Safe('The target is not vulnerable') | ||
| end |
There was a problem hiding this comment.
Passing reasons like 'The target is vulnerable' / 'The target is not vulnerable' to CheckCode::Vulnerable/Safe produces redundant output because those classes already prepend default messages (e.g., "The target is vulnerable."). Prefer omitting the reason or using a reason that adds new information (e.g., "Successfully leaked admin creds via SSRF" / "Could not leak admin creds").
adfoster-r7
force-pushed
the
improve-checkcode-messages-5
branch
from
c9b4ac6 to
abc004b
Compare
| if res.get_html_document.at('div[@id="footer-copyright"]').text.include? 'rConfig Version 3.9' | ||
| print_good('rConfig version 3.9 detected') | ||
| return Exploit::CheckCode::Appears('The target appears to be vulnerable') | ||
| return Exploit::CheckCode::Appears('rConfig version 3.9 was detected') | ||
| elsif res.get_html_document.at('div[@id="footer-copyright"]').text.include? 'rConfig' | ||
| print_status('rConfig detected, but not version 3.9') | ||
| return Exploit::CheckCode::Detected('The target service was detected') | ||
| return Exploit::CheckCode::Detected('rConfig was detected but not version 3.9') |
There was a problem hiding this comment.
res.get_html_document.at('div[@id="footer-copyright"]').text will raise NoMethodError if the element is missing (e.g., when the target is not rConfig but still returns HTML). To keep check robust, assign the node once and handle nil (returning Safe for not detected or Unknown for unexpected content) instead of calling .text unconditionally.
| end | ||
|
|
||
| CheckCode::Safe("Version #{version} is not vulnerable") | ||
| CheckCode::Safe(version ? "Version #{version} is not vulnerable" : 'Could not determine Nagios XI version') |
There was a problem hiding this comment.
When the version element is missing, this returns CheckCode::Safe('Could not determine Nagios XI version'). If the version can't be determined, Safe is a misleading status (it implies confirmed not vulnerable). Consider returning CheckCode::Unknown('Could not determine Nagios XI version'), or returning Safe only when you can positively identify the target as non-vulnerable/not Nagios XI.
| CheckCode::Safe(version ? "Version #{version} is not vulnerable" : 'Could not determine Nagios XI version') | |
| version ? CheckCode::Safe("Version #{version} is not vulnerable") : CheckCode::Unknown('Could not determine Nagios XI version') |
adfoster-r7
force-pushed
the
improve-checkcode-messages-5
branch
from
abc004b to
1f9124e
Compare
| end | ||
| if res.get_html_document.at('div[@id="footer-copyright"]').text.include? 'rConfig Version 3.9' | ||
|
|
||
| footer = res.get_html_document.at('div[@id="footer-copyright"]') |
There was a problem hiding this comment.
res.get_html_document.at('div[@id="footer-copyright"]') is being treated as a CSS selector by Nokogiri (since it doesn’t start with //), and div[@id="..."] is invalid CSS. This will raise a Nokogiri::CSS::SyntaxError at runtime and prevent check from working. Use a valid CSS selector (e.g., div#footer-copyright) or an explicit XPath query (e.g., at_xpath('//div[@id="footer-copyright"]')).
| footer = res.get_html_document.at('div[@id="footer-copyright"]') | |
| footer = res.get_html_document.at('div#footer-copyright') |
| end | ||
| print_error("#{datastore['USERNAME']} doesn't have the right to >>Package Update<<") | ||
| print_status("Please try with another user account!") | ||
| CheckCode::Safe('The target is not vulnerable') | ||
| CheckCode::Detected("Version #{version} may be vulnerable, but user '#{datastore['USERNAME']}' lacks Package Updates permissions") | ||
| end |
There was a problem hiding this comment.
The final CheckCode::Detected("Version #{version} may be vulnerable...") path can be reached even when version was never extracted (or when version > 910), which results in misleading output (e.g., empty version) and an incorrect status. Consider branching explicitly: return Safe when the parsed version is > 910; return Detected only when version is within the vulnerable range but the user lacks Package Updates permissions; and return Unknown/Detected with a version-unknown message when the version couldn’t be determined from sysinfo.cgi.
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