Apply escaping args to other command shells #19745
Conversation
|
Not a blocker to this PR; Looks like it'd be worth us expanding our acceptance tests to include ssh modules+sessions in the future 👍 |
| if @platform == 'windows' | ||
| extend(Msf::Sessions::WindowsEscaping) | ||
| else | ||
| extend(Msf::Sessions::UnixEscaping) | ||
| end |
There was a problem hiding this comment.
Are you sure there are no other possibilities besides Windows and Unix wrt. shell escaping?
There was a problem hiding this comment.
From https://github.com/rapid7/metasploit-framework/blob/master/lib/metasploit/framework/ssh/platform.rb#L97-L132, I'm really not all that familiar with what exactly to expect shell-escaping-wise when you get (for example) an "arista" shell. If it's some custom shell completely unrelated to anything POSIX, then any of the behaviour that uses escape_arg (such as uploading files or launching processes) would be useless anyway and would need its entirely new implementation.
In those cases, I'm not sure what the best default would be. The alternative I guess would be better in those unknown cases to just not extend the object, and do no escaping.
There was a problem hiding this comment.
From my very brief analysis, the current state should easily cover all platforms except Cisco ios, Juniper, Mikrotik, Arista and vcenter - I'm not 100% sure how those shells work when it comes to character escaping. So the options are: add else for all above mentioned and just implement escaping for them later on with more thorough analysis or ignore it, hope for best and add at least else block for Unknown platform. We will probably have to create/extend tests for SSH sessions (though I'm not very familiar how they are implemented now, but I don't expect such cases are covered there).
Co-authored-by: Julien Voisin <[email protected]>
| # will in turn be interpreted by CommandLineToArgvW. | ||
| def argv_to_commandline(args) | ||
| escaped_args = args.map do |arg| | ||
| escape_arg(arg) |
There was a problem hiding this comment.
More condensed form
| escape_arg(arg) | |
| escaped_args = args.map { |arg| escape_arg(arg) } |
| # Escape an individual argument per Windows shell rules | ||
| # @param arg [String] Shell argument | ||
| def escape_arg(arg) | ||
| needs_quoting = space_chars.any? do |char| |
There was a problem hiding this comment.
More condensed form
| needs_quoting = space_chars.any? do |char| | |
| needs_quoting = space_chars.any? { |char| arg.include?(char) } |
| if @platform == 'windows' | ||
| extend(Msf::Sessions::WindowsEscaping) | ||
| else | ||
| extend(Msf::Sessions::UnixEscaping) | ||
| end |
There was a problem hiding this comment.
From my very brief analysis, the current state should easily cover all platforms except Cisco ios, Juniper, Mikrotik, Arista and vcenter - I'm not 100% sure how those shells work when it comes to character escaping. So the options are: add else for all above mentioned and just implement escaping for them later on with more thorough analysis or ignore it, hope for best and add at least else block for Unknown platform. We will probably have to create/extend tests for SSH sessions (though I'm not very familiar how they are implemented now, but I don't expect such cases are covered there).
This adds
escape_argsmethod to all CommandShells, and more specifically, finds the appropriate OS escaping routines for an SSH server (usually Unix presumably, but possibly Windows).Resolves #19636
Verification
msfconsoleauxiliary/scanner/ssh/ssh_loginirbcorrectly)use netfilter_priv_esc_ipv4set session 1(again to set upirb)irbwrite_file('/tmp/test "quotes"', 'hi')escape_argerror)