-
Notifications
You must be signed in to change notification settings - Fork 82
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
Ensure CommunicationError is raised correctly, not EncryptionError #263
Ensure CommunicationError is raised correctly, not EncryptionError #263
Conversation
55b6a43
to
2716df6
Compare
@@ -578,7 +578,7 @@ def recv_packet(encrypt: false) | |||
raw_response = dispatcher.recv_packet | |||
rescue RubySMB::Error::CommunicationError => e | |||
if encrypt | |||
raise RubySMB::Error::EncryptionError, "Communication error with the "\ | |||
raise e, "Communication error with the "\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the context that you plan to use this in; if the session is killed - will the message be shown to the user as an encryption error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Should I add logic above this to check if the response is nil
and raise a specific message for that scenario, or do you have something in mind?
raise e, "Communication error with the "\ | |
raise RubySMB::Error::CommunicationError, "An error occurred reading from the Socket #{e.message}" if raw_response.nil? | |
if encrypt | |
raise e, "Communication error with the "\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, actually doing my suggestion leave us in almost the same scenario we were for my original implementation. So probably not what we want 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think long-term we should probably handle this better, potentially special casing this error if it's got a connection to a windows 8 target
For now, maybe we could just re-word the error message to be less clear-cut that the communication error was caused by encryption issues
if encrypt
raise e, "Communication error with the "\
"remote host: #{e.message}. The server supports encryption and this error may have been caused by encryption issues."
Or maybe we could have special case for the amount of encrypted messages sent already, and if it's more than 1 - we just don't need this message any more
2716df6
to
aa9ed6e
Compare
Note
This PR is in conjunction with a PR in Metasploit-Framework . This PR will need to be landed before the framework PR.
This PR is to add a more precise error being raised when a session is dead. Currently in Metasploit-Framework if a session dies we would have had a
RubySMB::Error::EncryptionError
error being raised which wasn't accurate. This change will now raise anRubySMB::Error::CommunicationError
when a session dies, which feels a lot closer in terms of accuracy.