-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
Remove SSH scanner using known_hosts #10456
Conversation
Fix #10266 This disables writing to the `known_hosts` file when performing auxiliary ssh scans.
As noted, this fixes just |
Ah, I had completely forgot to look for other scanners this might be in. I can look and see for others that use SSH. Should this be disabled for every scanner module using SSH (and potentially most exploits?). |
Didn't get a chance to test this, but does it address a key collision? I know it may prevent it in the scenario where you ONLY us msf, but what about this likely scenario:
|
Assuming the entry is a valid key after the change, yes this works. If it is not a valid key (change a random letter), you'll get an error about the key not being on the curve.
|
Removed host key checking on the |
Sounds good, would be easy enough to get this in all of them. I’ll add that then and let you know when those are in. |
@@ -48,14 +48,15 @@ def run_host(ip) | |||
factory = ssh_socket_factory | |||
|
|||
ssh_opts = { | |||
port: rport, | |||
:port => rport, |
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.
Is there a reason for changing the syntax here? I prefer the newer hash syntax if possible. All these keys are symbols, too.
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.
That said, hashrockets are more consistent with varying key types. I won't complain too much - just asking why it's necessary to change syntax.
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.
Consistency is the only reason. While I also prefer the new syntax, rocket formatted ones are what I’ve been asked for before when building exploit information hashes and was the style of the LoginScanner. As such, while I prefer the newer one, I think consistency is more important than minimal changes. I’m open though to other opinions if we want to use them, but stylistically I think the framework would benefit from a more defined code style guideline around hash structures
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.
Agreed. I ask only because you're modifying my module. :-)
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 hashrockets will be the way to go if we publish our own style guide (and not just cargo-cult the Ruby style guide). We have such wildly varying hashes that it'd make sense to stick to one style.
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.
Python is a little more predictable. :-)
Added all of the exploits that were within a |
auth_methods: ['password', 'keyboard-interactive'], | ||
password: %q{<<< %s(un='%s') = %u}, | ||
proxy: factory, | ||
:non_interactive => true |
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.
this was one hash that had a mix of the styles
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.
Yeah, yuck. And they're still all symbol keys, so they COULD have used the newer syntax. Oh well. Hashrockets it is!
Looks like everything passed, so take a look when you have time (no rush) and let me know if there are any changes needed or things missing scope wise. |
Awesome, will take a look! |
I ended up consolidating on the newer hash syntax for my modules, since I was using it already in the same code - just want to be consistent. I've also cleaned up whitespace around the hashrockets. Tested and verified working. No more crap in my This works great. Thanks! |
Release NotesThis disables host key verification for SSH modules, preventing Metasploit from writing to the user's |
Course and that’s fair. Potentially it’d make sense to have an issue to standardize certain coding conventions like that. And the white space makes sense, I wasn’t sure which way to go on it. I ended up going with nvim’s first tab location after the word rather than aligning with the natural rocket location of the longest entry. Appreciate the time reviewing this :) |
Fix #10266
This disables writing to the
known_hosts
file when performing auxiliary ssh scans.Tell us what this change does. If you're fixing a bug, please mention
the github issue number.
Verification
List the steps needed to make sure this thing works. This is described assuming a hackthebox.eu VIP membership. Replication can be made without this and a different RHOSTS value.
~/.ssh/known_hosts
msfconsole
use auxiliary/scanner/ssh/ssh_login
set USERNAME test
set PASSWORD test
set RHOSTS 10.10.10.6
exploit
~/.ssh/known_hosts
file was not created.Tested via HackTheBox retired machine Popcorn using Kali distributed MSF.
ssh.rc:
Before:
After: