Add support for Meterpreter logging to file

[dwelch-r7]

Companion to (and must be landed after) rapid7/metasploit-payloads#563

Exposes the datastore option MeterpreterDebugLogging for setting a file path to log debug statements to for the Windows C Meterpreter payloads

Verification

• Make sure to use a compatible debug build from here Add logging to file capabilities metasploit-payloads#563 (you can just drop the relevant dlls into metasploit-framework/data/meterpreter/)
• Boot up msfconsole
• set MeterpreterDebugBuild true
• `set MeterpreterDebugLogging rpath:/path/to/log/file.txt
• Start your handler
• Generate your payload (e.g. msf6 payload(windows/meterpreter/reverse_tcp) > generate -f exe -o win_met_rev_tcp_4444.exe)
• Drop your payload onto a windows VM
• Run payload
• Observe log file exists
• Re-do steps for x64/x86 staged/stageless payloads
• When MeterpreterDebugLogging is blank or otherwise invalid there should be no log file generated but sessions should still work
• When MeterpreterDebugBuild is false there should be no log file present

[jmartin-tech]

This seems a bit clunky, should we consider having Msf::OptMeterpreterDebugLogging set object accessors when parsed during validation so the payload writers are not thinking about parsing every time?

This may require adding a to_s to the Opt class to support displaying current datastore setting.

I envision the datastore value could then be accessed as:

Suggested change

	log_path: Msf::OptMeterpreterDebugLogging.parse_logging_options(ds['MeterpreterDebugLogging'])[:rpath]
	log_path: ds['MeterpreterDebugLogging'].rpath

[jmartin-tech]

This could also simplify the existing usage of that parser method from the previous PR.

metasploit-framework/lib/msf/core/payload/python/meterpreter_loader.rb

Lines 77 to 78 in cb9e412

	logging_options = Msf::OptMeterpreterDebugLogging.parse_logging_options(ds['MeterpreterDebugLogging'])
	met.sub!(%q|DEBUGGING_LOG_FILE_PATH = None|, %Q|DEBUGGING_LOG_FILE_PATH = "#{logging_options[:rpath]}"|) if logging_options[:rpath]

could become:

met.sub!(%q|DEBUGGING_LOG_FILE_PATH = None|, %Q|DEBUGGING_LOG_FILE_PATH = "#{ds['MeterpreterDebugLogging'].rpath}"|) if ds['MeterpreterDebugLogging'].rpath

[jmartin-tech]

These comments still apply, unrelated changes marked then as outdated.

[dwelch-r7]

@jmartin-r7 would this be a blocker for you? or would you be cool with coming back to this later just so we can get this shipped
I just think placing a more complex object into the datastore could lead to some...unexpected interactions that could delay this further

[jmartin-tech]

I consider this pattern a problem, I can see how the original proposed adjustment could have impact, however some refactor is needed.

Looking closer at this I suspect the #16451 should should have noted that direct access to datastore values inside generate* methods is problematic as this method signature takes opts and even defines ds passed as opts[:datastore] as an override to the global datastore. As is stands looking at places where ds is accessed shows that falling back to something the module writer had access to define creates consistency issues that are having to be calculated or selected from multiple possible locations. I would prefer not to see that pattern expanded.

I suggest that both MeterpreterDebugBuild and MeterpreterDebugLogging should be parsed higher in the stack and passed as values in opts for all cases in this PR. The change in this file would then become just setting the values from opts.

{
    debug_build: opts[:debug_build],
    log_path:  opts[:log_path]
}

[dwelch-r7]

defines ds passed as opts[:datastore] as an override to the global datastore.

That's certainly true in this case but not for all of the payloads looking here for example

metasploit-framework/modules/payloads/singles/windows/meterpreter_reverse_https.rb

Lines 50 to 60 in 7bfb1fb

	config_opts = {
	arch: opts[:uuid].arch,
	exitfunk: datastore['EXITFUNC'],
	expiration: datastore['SessionExpirationTimeout'].to_i,
	uuid: opts[:uuid],
	transports: [transport_config_reverse_https(opts)],
	extensions: (datastore['EXTENSIONS'] || '').split(','),
	ext_init: (datastore['EXTINIT'] || ''),
	stageless: true,
	debug_build: datastore['MeterpreterDebugBuild'],
	log_path: Msf::OptMeterpreterDebugLogging.parse_logging_options(datastore['MeterpreterDebugLogging'])[:rpath]

it's not an option and likely where the pattern was picked up from for datastore['MeterpreterDebugBuild'] initially

I agree though makes sense to parse these values out earlier in the function and pass them in, happy to do that

[jmartin-tech]

Does this represent a breaking change to payloads?

Not 100% sure where all this could be consumed however it looks like generate_config, which eventually leads to this, is used to pass config to precompiled payload generation, if the serialization and size of what is inserted extends this could mean some previously generated payloads without the extra data to unpack may not be compatible.

The concern here is just to validate that an existing target deployed payload can still connect and when/if the session enumeration reports config data it unpacks correctly.

[jmartin-tech]

I thinks some confusion of variables happened here.

Suggested change

	parsed_options = Msf::OptMeterpreterDebugLogging.parse_logging_options(opts[:debug_build])
	parsed_options = Msf::OptMeterpreterDebugLogging.parse_logging_options(datastore['MeterpreterDebugLogging'])

I wonder how putting a helper in msf/base/sessions/meterpreter_options.rb that grabs the logging options might work it looks like that mixin is already in these locations:

Something like adding to MeterpreterOptions:

      def meterpreter_logging_config(opts = {})
        ds = opts[:datastore] || datastore
        {
          debug_build: (ds[:debug_build] || datastore['MeterpreterDebugBuild']),
          log_path:    (ds[:log_path] || Msf::OptMeterpreterDebugLogging.parse_logging_options(datastore['MeterpreterDebugLogging'])[:rpath])
        }
      end

and then

   config_opts = {
      arch:       opts[:uuid].arch,
      exitfunk:   datastore['EXITFUNC'],
      expiration: datastore['SessionExpirationTimeout'].to_i,
      uuid:       opts[:uuid],
      transports: [transport_config_reverse_ipv6_tcp(opts)],
      extensions: (datastore['EXTENSIONS'] || '').split(','),
      ext_init:   (datastore['EXTINIT'] || ''),
      stageless:  true
    }.merge(meterpreter_logging_config(opts))

similar to the pattern used for transport configs

[dwelch-r7]

I thinks some confusion of variables happened here.

Oh yep bad copy paste nice catch

[adfoster-r7]

Looks like this bytes size has gone up which I wouldn't have expected 🤔

A quick bindiff shows we're compiling the init logging and log function:

Since metasploit-payloads already has extra commits, we can land this and fix it separately

[adfoster-r7]

Working for me:

use windows/x64/meterpreter_reverse_tcp
set MeterpreterDebugBuild true
set MeterpreterDebugLogging rpath:C:/test/foo.txt
generate -f exe -o shell.exe
save
to_handler

use windows/x64/meterpreter/reverse_http
set MeterpreterDebugBuild true
set MeterpreterDebugLogging rpath:C:/test/bar.txt
save
generate -f exe -o shell_http_testing.exe
to_handler

Debug logs saved to disk, and in the usual place 🎉

We'll have a fast follow for nuking the unused log functions from the production builds after the mimikatz bump is landed

[adfoster-r7]

Release Notes

The Windows Meterpreter payload now supports a MeterpreterDebugLogging datastore option for logging debug information to a file. Example usage:

use windows/x64/meterpreter_reverse_tcp
set MeterpreterDebugBuild true
set MeterpreterDebugLogging rpath:C:/test/foo.txt
save
generate -f exe -o shell.exe
to_handler