Minor updates to meterpreter persistence module. #2395
Conversation
Ensured lines < 80 characters. A couple of minor formatting changes and typos.
The windows pathname needs to have the '\' separators within the path escaped. The kill command can not be used to terminate the agent process as it is liable to change (e.g. after a system restart).
|
Thanks for your first contribution! |
| else | ||
| logs = ::File.join(Msf::Config.log_directory, 'persistence', Rex::FileUtils.clean_path(host + filenameinfo) ) | ||
| logs = ::File.join(Msf::Config.log_directory, 'persistence', \ |
There was a problem hiding this comment.
Noted, I'll remove (along with comments).
There was a problem hiding this comment.
I meant "those mentioned in other comments" .... ;-)
|
My comments are mostly about small style issues, but it looks like the bug in the generated cleanup resource file is present in both the local exploit and the (deprecated) post module of the same name. Yes, I know it sucks we have 3 different things that perform the same task. |
|
Is the deprecated module still going to be distributed ? (if not I won't fix). I will re-post changes within 24hours..... |
Indentation changed to universally use two spaces. Removed superfluous " \" at the end of wrapped lines.
Added a space after some commas (where missing). Other minor code layout changes to keep lines < 80 characters where practical.
The windows pathname needs to have the '\' separators within the path escaped. The kill command cannot be used to terminate the agent process as it is liable to change (e.g. after a system restart).
Inserted a space after commas (where it was missing). Ensured that line length < 80 characters where practical.
Clarified the return value.
The windows pathname needs to have the '\' separators within the path escaped. Also clarified the return status from the "target_exec" function; returning true/nil seems potentially counter intuitive so changed to true/false. The comment was also incorrect (no attempt was made to return the pid), and the invoking code was checking for the 'nil', which whilst safe re-enforced the belief that a non-nil PID value might be expected upon success.
|
Ok, the above commits have addressed the feedback provided on this thread. In summary: |
Need to correctly account for 'false' being returned by target_exec if the payload failed to execute on the target. Also clarified the return value from 'write_script_to_target' function.
|
Make sure you use a separate branch for your PR. It's unwise to merge from your master unless you keep it pristine. Topic branches are GOOD. |
|
Noted. Actually, I did do this on a branch at my end, then merged to my master and then pushed from there. I hadn't thought to push straight from my origin branch... (which could save me getting into a mess if I had multiple changes running concurrently). |
| @@ -73,7 +72,7 @@ def exploit | |||
| end | |||
|
|
|||
| # Initial execution of script | |||
| if target_exec(script_on_target) == nil | |||
| if not target_exec(script_on_target) | |||
There was a problem hiding this comment.
unless target_exec(script_on_target)
Clarified logic (used 'unless' rather than 'if not' where appropriate). Removed superfluous braces from if statement expression.
|
@wvu-r7 what needs to happen with this other than resolving the current merge conflict? |
|
@Meatballs1: Just the conflicts(s). |
|
Snagging this to fix the merge conflicts and just land. |
Slight cleanup (e.g. to keep lines less than 100 characters).
Clarification within the help text.
Resolve a minor bug in the cleanup resource file created by this module to escape the windows path separator and to not attempt to kill the agent process (this would not have worked as the pid is likely to have changed, e.g. as a result of a target system reboot).
NB: This is my first contribution so is a deliberately minor change so as to familiarise myself with the process.