Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add explicit error handling to base login scanner #19252

Conversation

zgoldman-r7
Copy link
Contributor

@zgoldman-r7 zgoldman-r7 commented Jun 11, 2024

Continuation of #17959

This PR addresses a silent catch-all rescue in our base login scanner. Previously, exceptions not caught by whatever login scanner implementation was being used would silently fail:
Screenshot 2024-06-11 at 10 54 23 AM
This adds explicit error catching for known errors.
Screenshot 2024-06-11 at 10 54 38 AM
This exception list is pulled from the exceptions potentially raised by each of the modules, excluding exceptions specific to one module that are already caught.

Verification

List the steps needed to make sure this thing works

  • Start msfconsole
  • use a module that leverages login_scanner such as mssql_login
  • trigger an error context that the module does not rescue that is in the exception list (shortcut: hardcode a raise in the module)
  • verify the behavior is what we'd want and expect
  • double check the exception list

@@ -251,8 +251,13 @@ def scan!
break if total_error_count >= 10
end
end
rescue => e
elog('Attempt may not yield a result', error: e)
rescue Rex::ConnectionRefused, Rex::SocketError, Rex::HostCommunicationError, Rex::ConnectionError, RuntimeError, NoMethodError, TypeError, Rex::TimeoutError, OpenSSL::Cipher::CipherError, Rex::ConnectionProxyError, Errno::ECONNRESET, Errno::EINTR, Errno::EPIPE, ::SystemCallError, ::EOFError, Errno::ENOTCONN, ::Timeout::Error, Rex::Proto::Kerberos::Model::Error::KerberosEncryptionNotSupported, ::JSON::ParserError, Errno::ECONNRESET, OpenSSL::SSL::SSLError, Rex::StreamClosedError, ::Rex::Proto::Kerberos::Model::Error::KerberosError, Errno::ETIMEDOUT, StandardError => e
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these exceptions are definitely not login related issues, and are more likely exceptions raised by broken modules with Ruby issues - i.e. NoMethodError and RuntimeError and TypeError

Likewise rescue StandardError => e is just the same as rescue => e (details) - I think these code changes might need some more cycles spent on analysing the approach here 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I'll take another pass 🫡

@zgoldman-r7 zgoldman-r7 force-pushed the stop-login-scanner-swallowing-exceptions branch from 2f8be32 to a511729 Compare June 11, 2024 18:01
rescue => e
elog('Attempt may not yield a result', error: e)
rescue Rex::ConnectionRefused, Rex::SocketError, Rex::HostCommunicationError, Rex::ConnectionError, Rex::TimeoutError, OpenSSL::Cipher::CipherError, Rex::ConnectionProxyError, Errno::ECONNRESET, Errno::EINTR, Errno::EPIPE, ::SystemCallError, ::EOFError, Errno::ENOTCONN, ::Timeout::Error, Rex::Proto::Kerberos::Model::Error::KerberosEncryptionNotSupported, ::JSON::ParserError, Errno::ECONNRESET, OpenSSL::SSL::SSLError, Rex::StreamClosedError, ::Rex::Proto::Kerberos::Model::Error::KerberosError, Errno::ETIMEDOUT => e
framework_module.print_error("Error, scan may not produce results: #{e.message}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
framework_module.print_error("Error, scan may not produce results: #{e.message}")
framework_module.print_error("Error, scan may not produce results: #{e.message}") if framework_module

Copy link
Contributor

Choose a reason for hiding this comment

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

As framework_module might be nil

@zgoldman-r7 zgoldman-r7 force-pushed the stop-login-scanner-swallowing-exceptions branch from 275e80a to 7b00ae8 Compare June 12, 2024 15:57
@adfoster-r7 adfoster-r7 self-assigned this Jun 24, 2024
rescue => e
elog('Attempt may not yield a result', error: e)
rescue Rex::ConnectionRefused, Rex::SocketError, Rex::ConnectionError, Rex::ConnectionProxyError, Rex::StreamClosedError, Rex::TimeoutError, ::Timeout::Error, Errno::ETIMEDOUT, Errno::ENOTCONN, Errno::ECONNRESET => e
framework_module.print_warning("Error, scan may not produce results: #{e.message}") if framework_module
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if this logged out the credential details that caused the issue - otherwise it doesn't align with the current output generated by the logging scanner

Copy link
Contributor

Choose a reason for hiding this comment

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

diff --git a/lib/metasploit/framework/login_scanner/base.rb b/lib/metasploit/framework/login_scanner/base.rb
index 6b1ba2f820..6d0e5e9c85 100644
--- a/lib/metasploit/framework/login_scanner/base.rb
+++ b/lib/metasploit/framework/login_scanner/base.rb
@@ -251,8 +251,11 @@ module Metasploit
                   break if total_error_count >= 10
                 end
               end
-            rescue Rex::ConnectionRefused, Rex::SocketError, Rex::ConnectionError, Rex::ConnectionProxyError, Rex::StreamClosedError, Rex::TimeoutError, ::Timeout::Error, Errno::ETIMEDOUT, Errno::ENOTCONN, Errno::ECONNRESET => e
-              framework_module.print_warning("Error, scan may not produce results: #{e.message}") if framework_module
+            rescue => e
+              if framework_module
+                prefix = framework_module.respond_to?(:peer) ? "#{framework_module.peer} - LOGIN FAILED:" : "LOGIN FAILED:"
+                framework_module.print_warning("#{prefix} #{credential.to_h} - Unhandled error - scan may not produce correct results: #{e.message} - #{e.backtrace}")
+              end
               elog("Scan Error: #{e.message}", error: e)
               consecutive_error_count += 1
               total_error_count += 1

It's a bit verbose; but at least we'd be able to track down any issue that gets reported pretty fast

msf6 auxiliary(scanner/redis/redis_login) > run rhost=127.0.0.1 username=admin password=foo rport=6379

[!] 127.0.0.1:6379        - 127.0.0.1:6379        - LOGIN FAILED: {:private_data=>"foo", :private_type=>:password, :username=>nil, :realm_key=>nil, :realm_value=>nil} - Unhandled error - scan may not produce correct results: undefined method `strip' for nil:NilClass - ["/Users/user/Documents/code/metasploit-framework/modules/auxiliary/scanner/redis/redis_login.rb:110:in `block in run_host'", "/Users/user/Documents/code/metasploit-framework/lib/metasploit/framework/login_scanner/base.rb:234:in `block in scan!'", "/Users/user/Documents/code/metasploit-framework/lib/metasploit/framework/login_scanner/base.rb:179:in `block in each_credential'", "/Users/user/Documents/code/metasploit-framework/lib/metasploit/framework/credential_collection.rb:94:in `block in each_filtered'", "/Users/user/Documents/code/metasploit-framework/lib/metasploit/framework/credential_collection.rb:111:in `each_unfiltered'", "/Users/user/Documents/code/metasploit-framework/lib/metasploit/framework/credential_collection.rb:91:in `each_filtered'", "/Users/user/Documents/code/metasploit-framework/lib/metasploit/framework/login_scanner/base.rb:141:in `each_credential'", "/Users/user/Documents/code/metasploit-framework/lib/metasploit/framework/login_scanner/base.rb:205:in `scan!'", "/Users/user/Documents/code/metasploit-framework/modules/auxiliary/scanner/redis/redis_login.rb:83:in `run_host'", "/Users/user/Documents/code/metasploit-framework/lib/msf/core/auxiliary/scanner.rb:128:in `block (2 levels) in run'", "/Users/user/Documents/code/metasploit-framework/lib/msf/core/thread_manager.rb:105:in `block in spawn'"]

@zgoldman-r7 zgoldman-r7 force-pushed the stop-login-scanner-swallowing-exceptions branch from 7b00ae8 to 4316d52 Compare July 3, 2024 14:48
@adfoster-r7 adfoster-r7 merged commit 4909a43 into rapid7:master Jul 3, 2024
39 checks passed
@adfoster-r7
Copy link
Contributor

adfoster-r7 commented Jul 3, 2024

Release Notes

Improves error logging for unhandled exceptions for login scanners

@adfoster-r7 adfoster-r7 added the rn-fix release notes fix label Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn-fix release notes fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants