Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web rce framework #159

Merged
merged 14 commits into from Aug 22, 2018

Conversation

@VakarisZ
Copy link
Contributor

commented Jul 19, 2018

Feature / Fixes

Created a framework for web rce implementation. It saves code and makes logging more standard.
Created a locked http server to avoid race conditions

Tested with struts2 and oracle web logic blind vulnerability. Framework is more or less usefull in both of them

@@ -182,6 +183,36 @@ def stop(self, timeout=60):
self._stopped = True
self.join(timeout)

class LockedHTTPServer(threading.Thread):

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 7, 2018

Contributor

I think the purpose of this class is not super intuitive. Please add basic documentation to the class (no need to document the functions)

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 14, 2018

Contributor

The documentation still doesn't say too much. The race conditions avoided are due to the choice of use in the class.
It should be mentioned that 'lock' gets released once the server is up.


def stop(self, timeout=5):
self._stopped = True
self.join(timeout)

class HTTPConnectProxy(TransportProxyBase):

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 7, 2018

Contributor

PEP8: 2 blank lines between classes

@@ -182,6 +183,36 @@ def stop(self, timeout=60):
self._stopped = True
self.join(timeout)

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 7, 2018

Contributor

PEP 8: 2 blank lines between clasees


self._stopped = True

def stop(self, timeout=5):

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 7, 2018

Contributor

save 5 in a static constant

@@ -386,6 +386,23 @@ def create_transfer(host, src_path, local_ip=None, local_port=None):

return "http://%s:%s/%s" % (local_ip, local_port, urllib.quote(os.path.basename(src_path))), httpd

@staticmethod
def create_locked_transfer(host, src_path, lock, local_ip=None, local_port=None):

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 7, 2018

Contributor

add brief documentation to function

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 14, 2018

Contributor

documentation left unfinished

return True

@staticmethod
def check_if_exploitable(exploiter, url):

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 7, 2018

Contributor

From my understanding, exploiter should always be the 'exploit' method. If so, I think it'll be more convenient and readable to make the functions receiving 'exploiter' as a param non-static. and directly use the exploit method. (Do this for all functions receiving exploit). Same thing with the 'host' parameter

# To avoid race conditions we pass a locked lock to http servers thread
LOCK.acquire()
# Create server for http download and wait for it's startup.
http_path, http_thread = HTTPTools.create_locked_transfer(host, src_path, LOCK)

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 7, 2018

Contributor

I think using the LOCK.ackquire (both calls) inside the create_locked_transfer is better. I think in this context we'll always want them coupled.

if not host.os['type']:
LOG.error("Unknown target's os type. Skipping.")
return False
if 'linux' in host.os['type']:

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 7, 2018

Contributor

use the if-else clause to determine the primary part of the command, afterwards do 'command = prim_command % {'monkey_path' ....}'

if not host.os['type']:
LOG.error("Unknown target's os type. Skipping.")
return False
if 'linux' in host.os['type']:

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 7, 2018

Contributor

Export the whole command calculating to another function

A reference to a method which implements web exploit logic.
:param url: Url where to send maliciuos packet
:param command: Command which will be executed on remote host
:return: Command's output string. Or True/False if it's a blind exploit

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 7, 2018

Contributor

Is there a reason why a blind exploit can return True or False? why not just True? makes things simpler and more precise.

This comment has been minimized.

Copy link
@VakarisZ

VakarisZ Aug 9, 2018

Author Contributor

Yes if we get an error of some sorts in code execution. Framework was built on getting True/False or Response/False from exploit function passed to the framework, because it was usefull in testing if target is exploitable on blind exploits(checker exploiter returns True/False and acctual exploiter mainly True, if code executed successfully). But now when you can't have more than one exploit without overriding it's useless in most casses

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 14, 2018

Contributor

That's good. So just change the documentation to say it returns False if failed and True/response otherwise (depending on type of exploit).

Vakaris added 2 commits Aug 7, 2018
@VakarisZ VakarisZ force-pushed the VakarisZ:WebRCE_Framework branch from 375c426 to d1a2987 Aug 9, 2018
@@ -386,6 +386,23 @@ def create_transfer(host, src_path, local_ip=None, local_port=None):

return "http://%s:%s/%s" % (local_ip, local_port, urllib.quote(os.path.basename(src_path))), httpd

@staticmethod
def create_locked_transfer(host, src_path, lock, local_ip=None, local_port=None):

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 14, 2018

Contributor

documentation left unfinished

"Check upload_monkey function docs.")
return False
# Choose command:
if commands:

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 14, 2018

Contributor

To reduce code duplication:
if not commands, commands = {'windows': POWERSHELL_HTTP_UPLOAD, 'linux': WGET_HTTP_UPLOAD}
then just
command = WebRCE.get_command(self.host, path, http_path, commands)

@@ -182,6 +183,36 @@ def stop(self, timeout=60):
self._stopped = True
self.join(timeout)

class LockedHTTPServer(threading.Thread):

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 14, 2018

Contributor

The documentation still doesn't say too much. The race conditions avoided are due to the choice of use in the class.
It should be mentioned that 'lock' gets released once the server is up.

@@ -507,3 +512,30 @@ def get_binaries_dir_path():
def get_monkey_depth():
from config import WormConfiguration
return WormConfiguration.depth


def get_monkey_dest_path(src_path):

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 14, 2018

Contributor

src_path is a confusing name for a url to download from

CHECK_LINUX = "echo %s && lscpu" % ID_STRING
CHECK_COMMAND = "echo %s" % ID_STRING
# Architecture checking commands
ARCH_WINDOWS = "wmic os get osarchitecture"

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 14, 2018

Contributor

consider renaming to CHECK_ARCH_WINDOWS or GET_ARCH_WINDOWS, same with ARCH_LINUX

A reference to a method which implements web exploit logic.
:param url: Url where to send maliciuos packet
:param command: Command which will be executed on remote host
:return: Command's output string. Or True/False if it's a blind exploit

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 14, 2018

Contributor

That's good. So just change the documentation to say it returns False if failed and True/response otherwise (depending on type of exploit).

self.skip_exist = self._config.skip_exploit_if_file_exist

@abstractmethod
def exploit_host(self):

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 14, 2018

Contributor

Try to implement this in a generic way. Add abstract functions if needed

def __init__(self, host, monkey_target_paths=None):
"""
:param host: Host that we'll attack
:param monkey_target_paths: Dict in format {'linux': '/tmp/monkey.sh', 'win32': './monkey32.exe', 'win64':... }

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 19, 2018

Contributor

It's good that you specify the expected format when it's unclear. However you should also document the purpose of the parameter

"custom dict of monkey's destination paths")
return False

def custom_to_dropper_path(self, path):

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 19, 2018

Contributor

document method


resp = self.exploit(url, command)

if not isinstance(resp, bool) and 'owershell is not recognized' in resp:

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 19, 2018

Contributor

save 'owershell is not recognized' as a const


def custom_to_dropper_path(self, path):
try:
key = self.monkey_target_paths.keys()[self.monkey_target_paths.values().index(path)]

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 19, 2018

Contributor

TL;DR: Try to see if you can get the target OS/arch using more specific info instead of assuming the target paths are unique and doing a reverse lookup. If this is too complex given the current situation, I can let this one slide...

This is bad practice.
In general no one promises there'll be only 1 key that fits each value. In the case of the monkey it is the case.
Moreover, you have everything you need to determine the target OS/arch based on more specific info, but the current flow might make it hard to get it.

…ework's workflow according to some flags/params
def get_default_dropper_path(self):
"""
Gets default dropper path for the host.
:return: Default monkey's destination path for corresponding host.

This comment has been minimized.

Copy link
@itaymmguardicore

itaymmguardicore Aug 21, 2018

Contributor

Returns False if failed

@itaymmguardicore itaymmguardicore merged commit fc2929e into guardicore:develop Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.