Fix handling of UUIDs in Meterpreter

rapid7 / meterpreter

THIS REPO IS OBSOLETE. USE https://github.com/rapid7/metasploit-payloads INSTEAD

Other

326 stars 144 forks source link

Fix handling of UUIDs in Meterpreter #162

Closed OJ closed 9 years ago

OJ commented 9 years ago

The original implementation assumed that the UUIDs were coming through a strings, but this was changed at some point to use the 16-byte UUID format straight out of MSF.

This was causing issues when UUIDs had null bytes in them because the UUID was being truncated and the result was that UUIDs that were being parsed in MSF were too small, resulting in exceptions.

This PR fixes this by:

Making UUIDs exactly 16 bytes.
Changing the TLV type to raw.
Not use string functions when sending the UUID to Metasploit.

Please see the accompanying MSF PR for more details: https://github.com/rapid7/metasploit-framework/pull/5357

metasploit-public-bot commented 9 years ago

Test PASSED. Refer to this link for build results (access rights to CI server needed): https://ci.metasploit.com//job/GPR-MeterpreterWin/198/ Test PASSED.

bcook-r7 commented 9 years ago

Thanks, landed with a minor tweak 3f58378725c74bd5cbea0edcd33accbf398d42c3

OJ commented 9 years ago

Was the minor tweak removing the debug output?

bcook-r7 commented 9 years ago

Since the UUID is not a string, this removes printing it as a string. It was printed again on Windows two lines later.

OJ commented 9 years ago

Thanks!

bcook-r7 commented 9 years ago

Anything we can do to help make a mixed configuration more apparent? I've contemplating making metasploit-payloads display a notice message when it diverts to a development file rather than the embedded gem binary.

OJ commented 9 years ago

Yeah I've been pondering this as well. I'm not sure what the best approach here is. Perhaps we could throw something up in the console to warn people?

bcook-r7 commented 9 years ago

Sure, a simple console warning like 'foo.dll loaded from non-default location /user/bob/metasploit/data/meterpreter'.

I don't think we have access to the framework instance from within the gem, but even a 'STDERR.puts' would probably be an improvement for logging what is likely an unusual situation for most users. Though, I'm not sure if maybe a lot of third parties take advantage of the gem bypass mode on a regular basis that it would get annoying without a way to turn it off.

OJ commented 9 years ago

Yeah this gets a +1 from me, at least for POSIX and Windows meterp.