Address review feedback (payload handler, arch, SessionTypes, DLL_NAME validation, doc cleanup) - #14
Open
prithvee07 wants to merge 10 commits into
Open
Address review feedback (payload handler, arch, SessionTypes, DLL_NAME validation, doc cleanup)#14prithvee07 wants to merge 10 commits into
prithvee07 wants to merge 10 commits into
Conversation
Applies the requested changes from dledda-r7 and h00die's review on rapid7#21128 (Nayeraneru's win-port-monitor branch): - Removed the DefaultOptions block disabling the payload handler and hardcoding a meterpreter/reverse_tcp payload; persistence modules need the handler active to catch the callback when the payload fires later. - Dropped ARCH_AARCH64 from Arch, since Metasploit has no meterpreter or shell payload for that architecture yet. - Restricted SessionTypes to meterpreter only. - DLL_NAME no longer fails validation when missing a .dll extension; payload_name now appends it automatically. - MONITOR_NAME and DLL_NAME now default to randomized 8-character alpha strings instead of static "Hadess"/"persist.dll" values, and are marked required to match. - Documentation: restructured Scenarios headings (### OS / #### scenario per h00die's convention), removed the unrelated local_exploit_suggester detour, fixed an escaped   entity and a doubled backslash in a code span, and updated the Options section for the new randomized defaults. Passes tools/dev/msftidy_docs.rb clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ae2LTwjw9tvJqqGqqwjerz
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Addresses the outstanding review feedback from dledda-r7 and h00die on rapid7#21128 ("Add Persistence Technique: Windows Port Monitor"), applied directly on top of
win-port-monitorso it can be merged into that PR before it lands upstream.Changes:
Removed the
DefaultOptionsblock that disabled the payload handler and hardcodedwindows/x64/meterpreter/reverse_tcp. Persistence modules install a payload that fires later (on Spooler restart/reboot), so the handler needs to stay up to catch that callback — disabling it defeats the module's purpose.Removed
ARCH_AARCH64fromArch, since Metasploit doesn't yet have a meterpreter or shell payload for that architecture.Restricted
SessionTypestometerpreteronly (was['meterpreter', 'shell']).DLL_NAMEno longer fails withFailure::BadConfigwhen it's missing a.dllextension —payload_namenow appends.dllautomatically instead.MONITOR_NAMEandDLL_NAMEnow default to randomized 8-character alpha strings (Rex::Text.rand_text_alpha(8)) instead of the staticHadess/persist.dll, and are markedrequiredto match.Documentation: restructured
## Scenariosto use### <OS>/#### <scenario>heading levels, removed the unrelatedlocal_exploit_suggesterdetour from the console transcript, fixed an escaped HTML entity and a doubled backslash inside a code span, and updated the Options section text for the new randomized defaults.tools/dev/msftidy_docs.rbpasses clean.Related Issue: Addresses review comments on rapid7#21128
Breaking Changes
None — no datastore option names were removed, only default values and required-ness changed (
MONITOR_NAME/DLL_NAMEdefaults are now randomized instead of static, and both are now required instead of optional).Reviewer Notes
This is scoped to exactly the changes requested in review — no unrelated module edits. Diff touches only
modules/exploits/windows/persistence/port_monitor.rband its documentation file.Verification Steps
[ ]
ruby -c modules/exploits/windows/persistence/port_monitor.rb— confirms syntax (already run, passes).[ ]
ruby tools/dev/msftidy_docs.rb documentation/modules/exploit/windows/persistence/port_monitor.md— confirms doc formatting (already run, passes clean).[ ]
use exploit/windows/persistence/port_monitor; show options— confirmMONITOR_NAME/DLL_NAMEnow show randomized defaults andRequired: yes.[ ]
set DLL_NAME foo(no extension) thenrunagainst a live/lab target — confirm it installs asfoo.dllinstead of erroring out.[ ] Full end-to-end run against a Windows target: install persistence, restart/reboot, confirm the handler receives a new meterpreter session (this step still needs to be performed and recorded — see the
<!-- TODO -->left in the doc's transcript).Test Evidence
ruby -candmsftidy_docs.rbboth pass clean on the changed files (see Verification Steps 1–2). I have not run this against a live Windows/Print Spooler target — steps 3–5 above still need real console output from testing against a lab target before this should be considered fully verified.Environment
Field Details
Operating System (fill in your dev OS)
Target Software/Hardware Windows Print Spooler — (fill in Windows version(s) tested)
Docker Image / Vagrant Setup (fill in if used, otherwise remove this row)
AI Usage Disclosure
Claude (Anthropic) was used to identify and implement the fixes requested in review: removing the payload-handler/meterpreter defaults, dropping aarch64 support, restricting SessionTypes, the DLL_NAME auto-extension change, randomized option defaults, and the documentation restructuring/cleanup. Per CONTRIBUTING.md, this code has been syntax- and lint-checked but has not yet been exercised against a live target — see Test Evidence above.
Pre-Submission Checklist
[x] No sensitive information (IP addresses, credentials, API keys, hashes) in code or documentation
[ ] Tested on the target environment specified in the Environment section above
[ ] Included RSpec tests for library changes (encouraged for
lib/changes — n/a, nolib/changes here)[ ] Read the CONTRIBUTING.md and module acceptance guidelines