`directory?` method fails properly under certain circumstances but places 'nix command-shell session in a unusable state

[bwatters-r7]

I found this testing https://github.com/rapid7/metasploit-framework/pull/17385. To recreate the failure, get a shell session on a 'nix VM and run the module post/test/file module:

       =[ metasploit v6.2.35-dev-07231a6a8e               ]
+ -- --=[ 2276 exploits - 1192 auxiliary - 406 post       ]
+ -- --=[ 951 payloads - 45 encoders - 11 nops            ]
+ -- --=[ 9 evasion                                       ]

Metasploit tip: When in a module, use back to go 
back to the top level prompt
Metasploit Documentation: https://docs.metasploit.com/

msf6 payload(linux/x64/shell/reverse_tcp) > to_handler
[*] Payload Handler Started as Job 0

[*] Started reverse TCP handler on 10.5.135.201:7878 
msf6 payload(linux/x64/shell/reverse_tcp) > [*] Sending stage (38 bytes) to 10.5.136.138
[*] Command shell session 1 opened (10.5.135.201:7878 -> 10.5.136.138:38576) at 2023-01-06 14:29:29 -0600

msf6 payload(linux/x64/shell/reverse_tcp) > loadpath test/modules/
Loaded 38 modules:
    14 auxiliary modules
    13 exploit modules
    11 post modules
msf6 payload(linux/x64/shell/reverse_tcp) > use post/test/file 
msf6 post(test/file) > set session 1
session => 1
msf6 post(test/file) > set verbose true
verbose => true
msf6 post(test/file) > run

[*] Setup: changing working directory to /tmp
[*] Running against session 1
[*] Session type is shell and platform is linux
[+] should test for file existence
[*] Max line length is 65537
[*] Writing 3 bytes in 1 chunks of 12 bytes (octal-encoded), using printf
[+] should create text files
[+] should read the text we just wrote
[*] Max line length is 65537
[*] Writing 3 bytes in 1 chunks of 12 bytes (octal-encoded), using printf
[*] Max line length is 65537
[*] Writing 3 bytes in 1 chunks of 12 bytes (octal-encoded), using printf
[+] should append text files
[+] should delete text files
[+] should move files
[-] FAILED: should test for directory existence
[*] Creating directory test-dir
[*] test-dir created
[-] FAILED: should create directories
[-] FAILED: should list the directory we just made
[-] Exception: NoMethodError: undefined method `split' for nil:NilClass
[+] should recursively delete the directory we just made
[!] skipping link related checks because the target is incompatible
[*] Writing 35120 bytes
[*] Max line length is 4096
[*] /usr/bin/printf '\0\377\376\127\123\123\104\177\45\45\15\12' Failed: nil != "\x00\xFF\xFEWSSD\x7F%%\r\n"
[*] printf '\0\377\376\122\111\127\126\177\45\45\15\12' Failed: nil != "\x00\xFF\xFERIWV\x7F%%\r\n"
[*] /usr/bin/printf %b '\0\377\376\124\110\127\130\177\45\45\15\12' Failed: nil != "\x00\xFF\xFETHWX\x7F%%\r\n"
[*] printf %b '\0\377\376\106\121\122\124\177\45\45\15\12' Failed: nil != "\x00\xFF\xFEFQRT\x7F%%\r\n"
[*] perl -e 'print("\0\377\376\122\105\121\107\177\45\45\15\12")' Failed: nil != "\x00\xFF\xFEREQG\x7F%%\r\n"
[*] gawk 'BEGIN {ORS="";print "\x00\xff\xfe\x49\x4c\x43\x4b\x7f\x25\x25\x0d\x0a"}' </dev/null Failed: nil != "\x00\xFF\xFEILCK\x7F%%\r\n"
[*] echo '00fffe5246425a7f25250d0a'|xxd -p -r Failed: nil != "\x00\xFF\xFERFBZ\x7F%%\r\n"
[*] echo -ne '\x00\xff\xfe\x5a\x4b\x51\x53\x7f\x25\x25\x0d\x0a' Failed: nil != "\x00\xFF\xFEZKQS\x7F%%\r\n"
[-] FAILED: should write binary data
[-] Exception: RuntimeError: Can't find command on the victim for writing binary data
[-] FAILED: should read the binary data we just wrote
[-] Exception: NoMethodError: undefined method `length' for nil:NilClass
[+] should delete binary files

The reason for this is because while testing the existence of a directory, the post/test/file module checks for the existence of three directories, assuming at least one will be on every target:

    it 'should test for directory existence' do
      ret = false
      [
        'c:\\',
        '/etc/',
        '/tmp'
      ].each do |path|
        ret = true if directory?(path)
      end

Unfortunately, when the C:\ directory is fed into the directory? method, the code execution lands on f = session.shell_command_token("test -d \"#{path}\" && echo true") which means we try to execute test -d "c:\" && echo true and the backslash in c:\ escapes the double quote, so the command line we feed it into does not know there's a functional return, so it hangs until the next command sends a carriage return, and our tokens are shot.

Sometimes, I think this is caught and processes properly by a timeout, but I can get it to happen most of the time at least on the current version of master.

An obvious solution is to replace the double quotes with single quotes, which is exactly what we were doing before a commit 8 years ago: https://github.com/rapid7/metasploit-framework/commit/05279ef02ad2a432ef3365b71bfebb49599f76cf Unfortunately, I don't see that there was a functional reason to choose the double over the single, so I don't know if dropping back to the single quotes would be a problem or not. Certainly, we should likely change all of them to single quotes if we change one back....

[bwatters-r7]

Let's look into finding a proper escape method.

[zeroSteiner]

Sort of related https://github.com/rapid7/metasploit-framework/issues/14742

[github-actions[bot]]

Hi!

This issue has been left open with no activity for a while now.

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 30 days since the last update here. If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request.

[bwatters-r7]

We should probably leave this open.

[gwillcox-r7]

@bwatters-r7 Marking confirmed since you reported it and adding this tag will prevent the bot chasing on this.

[smcintyre-r7]

Well I changed this back to single quotes in https://github.com/rapid7/metasploit-framework/pull/17914 without realizing we had an issue open for it already. The only reason I can think that we'd want to include double quotes would be for shell expansion but that'd be a possible discrepancy with the Meterpters and how they handle paths if they're not all expanding them first for every operation.