Fix issue with double-start of clipboard monitor

[OJ]

If a user attempts to start the clipboard monitor when it is already started then the code path that is taken results in the current clipboard monitor state pointers being lost. The net effect of this is that the existing monitor thread will never be shut down. Not a good thing!

This code fixes that case so that the monitor doesn't create a new monitor thread and doesn't reset important pointers to NULL.

This change also results in a "success" status being returned to the caller. This means it looks like the clipboard monitor has been started even if it was already running. I think this is acceptable and is better than an obscure error.

@wchen-r7 reported this as an issue. Fixes #120.

Verification

• Make sure the build is clean.
• Make sure that calling clipboard_monitor_start multiple times results in an indication of success.
• Make sure that after calling clipboard_monitor_start multiple times that the user is able to successfully shut down the monitor using clipboard_monitor_stop.
• Both x64 and x86 function correctly.

Sample run

msf exploit(handler) > run

[*] Started reverse handler on 172.16.52.1:8000
[*] Starting the payload handler...
[*] Sending stage (972288 bytes) to 172.16.52.1
[*] Meterpreter session 2 opened (172.16.52.1:8000 -> 172.16.52.1:61173) at 2015-01-31 06:57:20 +1000

meterpreter > use extapi
Loading extension extapi...success.
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_stop
[+] Clipboard monitor stopped
meterpreter > exit
[*] Shutting down Meterpreter...

[*] 172.16.255.143 - Meterpreter session 2 closed.  Reason: User exit

[metasploit-public-bot]

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

[bcook-r7]

Works well, but shouldn't we set pState to NULL after freeing it too in destroy_clipboard_monitor_state()?

[OJ]

pState is a local variable, so no it's not necessary. Once it's freed and the function exists it's no longer in scope.

[OJ]

Ah I see what you're saying. Sorry, I misunderstood.

Potentially, and that would require the func signature changing. Could be good for the sake of being thorough!

[OJ]

Thanks @bcook-r7. Made an adjustment based on your suggestion. Would you mind retesting please?

[metasploit-public-bot]

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

[bcook-r7]

Of course - just a sec.

[bcook-r7]

Looking good:

meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_stop
[+] Clipboard monitor stopped
meterpreter > clipboard_monitor_stop
[-] extapi_clipboard_monitor_stop: Operation failed: 758
meterpreter > clipboard_monitor_stop
[-] extapi_clipboard_monitor_stop: Operation failed: 758
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_stop
[+] Clipboard monitor stopped
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_stop
Text captured at 2015-01-30 22:42:45.0198
=========================================

ntributions to the Metasploit
offering your code under the M
SD-compatible license.  MIT an
ecifically cannot include GPL
 by case basis for libraries o


=========================================

[+] Clipboard monitor stopped

[OJ]

Cheers!

[OJ]

Seems legit:

msf exploit(handler) > run

[*] Started reverse handler on 172.16.52.1:8000
[*] Starting the payload handler...
[*] Sending stage (972288 bytes) to 172.16.52.1
[*] Meterpreter session 1 opened (172.16.52.1:8000 -> 172.16.52.1:52371) at 2015-02-11 08:08:54 +1000

meterpreter > shell
Process 3060 created.
Channel 1 created.
Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

Z:\scratch>whoami
whoami
win-tv01i7gg7jk\oj

Z:\scratch>exit
meterpreter > upload /tmp/zerobytes C:\\temp
[*] uploading  : /tmp/zerobytes -> C:\temp
[*] uploaded   : /tmp/zerobytes -> C:\temp\zerobytes
meterpreter > use extapi
Loading extension extapi...success.
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_stop
[+] Clipboard monitor stopped
meterpreter > clipboard_monitor_start
[+] Clipboard monitor started
meterpreter > clipboard_monitor_stop
[+] Clipboard monitor stopped
meterpreter >

Behaving fine for me on Windoze.

[OJ]

Same on the nixes

msf exploit(handler) > run

[*] Started reverse handler on 10.1.10.38:8000
[*] Starting the payload handler...
[*] Transmitting intermediate stager for over-sized stage...(100 bytes)
[*] Sending stage (1241088 bytes) to 10.1.10.40
[*] Meterpreter session 2 opened (10.1.10.38:8000 -> 10.1.10.40:40878) at 2015-02-11 08:14:20 +1000

meterpreter > shell
Process 22067 created.
Channel 1 created.
id
uid=1000(oj) gid=1000(oj) groups=1000(oj),10(wheel),1001(promiscuous) context=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
sh-4.2$ whoami
oj
sh-4.2$ exit
exit
^C
Terminate channel 1? [y/N]  y
meterpreter >

We really should sort out that channel closing issue on *nix too. Landing!