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

Check for nil res when setting echo shell #19617

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

sjanusz-r7
Copy link
Contributor

@sjanusz-r7 sjanusz-r7 commented Nov 5, 2024

This PR fixes #19615

From the code, shell_read_until_token can return nil. Now the code that broke in the linked issue explicitly checks for a nil.

I wasn't able to reproduce the issue locally using ssh_login against a Kali & Ubuntu VM, and I don't have any vCenter environments to test against.

Verification

  • Start msfconsole
  • use auxiliary/scanner/ssh/ssh_login
  • sessions -i -1
  • Verify you can get a shell using the shell command
  • Verify it doesn't crash

Example

[*] SSH session 1 opened (...:56421 -> ...:22) at 2024-11-05 13:37:01 +0000
[*] Scanned 1 of 1 hosts (100% complete)
[*] Scan completed, 1 credential was successful.

Successful logins
=================

    Host             Public  Private
    ----             ------  -------
    ...                 kali    kali


[*] 1 session was opened successfully.
[*] Auxiliary module execution completed
msf6 auxiliary(scanner/ssh/ssh_login) > sessions -i -1
[*] Starting interaction with 1...

shell

[*] Trying to find binary 'python' on the target machine
[*] Found python at /usr/bin/python
[*] Using `python` to pop up an interactive shell
[*] Trying to find binary 'bash' on the target machine
[*] Found bash at /usr/bin/bash

kali@kali:~$ whoami
whoami
kali

@@ -119,7 +119,7 @@ def set_is_echo_shell(timeout, command_separator)
cmd = "echo #{numeric_token}"
shell_write(cmd + "#{command_separator}echo #{token}#{command_termination}")
res = shell_read_until_token(token, 0, timeout)
@is_echo_shell = res.include?(cmd)
@is_echo_shell = res ? res.include?(cmd) : false
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
@is_echo_shell = res ? res.include?(cmd) : false
@is_echo_shell = res&.include?(cmd)

Copy link
Contributor

@adfoster-r7 adfoster-r7 Nov 5, 2024

Choose a reason for hiding this comment

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

Just a heads up that these would be semantically different

Before

3.2.5 :001 > res = nil
3.2.5 :002 > @is_echo_shell = res ? res.include?(cmd) : false
3.2.5 :003 > puts @is_echo_shell.inspect
 => false 

After

3.2.5 :001 > res = nil
3.2.5 :002 > @is_echo_shell = res&.include?(cmd)
3.2.5 :003 > puts @is_echo_shell.inspect
nil

Not a blocker; !!res&.include?(cmd) would be closer to the existing semantics

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, looks like if it ends up as nil, it'll just keep trying this every time; which while probably harmless, is probably not intended. Going with false seems better.

@adfoster-r7 adfoster-r7 marked this pull request as ready for review November 11, 2024 10:21
@adfoster-r7 adfoster-r7 merged commit 2206b0c into rapid7:master Nov 11, 2024
69 checks passed
@adfoster-r7
Copy link
Contributor

Release Notes

Fixes a crash when running against a shell session which does not echo the executed commands

@jheysel-r7 jheysel-r7 added the rn-fix release notes fix label Nov 15, 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.

single_command_shell.rb crash
5 participants