From 5bc061a6585d9e1ba29145cd1cdcf1833a0091b9 Mon Sep 17 00:00:00 2001 From: Shay Nehmad Date: Fri, 11 Oct 2019 21:09:34 +0300 Subject: [PATCH 1/6] Fixed cause for exception - bad JSON field access. Guid instead of id. --- .../cc/services/telemetry/processing/system_info.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/services/telemetry/processing/system_info.py b/monkey/monkey_island/cc/services/telemetry/processing/system_info.py index a970c0cd4..17d494729 100644 --- a/monkey/monkey_island/cc/services/telemetry/processing/system_info.py +++ b/monkey/monkey_island/cc/services/telemetry/processing/system_info.py @@ -102,4 +102,4 @@ def process_aws_data(telemetry_json): def update_db_with_new_hostname(telemetry_json): - Monkey.get_single_monkey_by_id(telemetry_json['_id']).set_hostname(telemetry_json['data']['hostname']) + Monkey.get_single_monkey_by_guid(telemetry_json['monkey_guid']).set_hostname(telemetry_json['data']['hostname']) From ef04c341f52fae1cab5f9fd10e789402c4995214 Mon Sep 17 00:00:00 2001 From: Shay Nehmad Date: Fri, 11 Oct 2019 21:10:31 +0300 Subject: [PATCH 2/6] Added "safe_process_telemetry" so if one stage of processing fails (in the future), it won't fail the other stages. --- .../telemetry/processing/system_info.py | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/monkey/monkey_island/cc/services/telemetry/processing/system_info.py b/monkey/monkey_island/cc/services/telemetry/processing/system_info.py index 17d494729..0af9a4315 100644 --- a/monkey/monkey_island/cc/services/telemetry/processing/system_info.py +++ b/monkey/monkey_island/cc/services/telemetry/processing/system_info.py @@ -1,3 +1,5 @@ +import logging + from monkey_island.cc.database import mongo from monkey_island.cc.models import Monkey from monkey_island.cc.services import mimikatz_utils @@ -7,14 +9,32 @@ from monkey_island.cc.services.telemetry.zero_trust_tests.antivirus_existence im from monkey_island.cc.services.wmi_handler import WMIHandler from monkey_island.cc.encryptor import encryptor +logger = logging.getLogger(__name__) + def process_system_info_telemetry(telemetry_json): - process_ssh_info(telemetry_json) - process_credential_info(telemetry_json) - process_mimikatz_and_wmi_info(telemetry_json) - process_aws_data(telemetry_json) - update_db_with_new_hostname(telemetry_json) - test_antivirus_existence(telemetry_json) + telemetry_processing_stages = [ + process_ssh_info, + process_credential_info, + process_mimikatz_and_wmi_info, + process_aws_data, + update_db_with_new_hostname, + test_antivirus_existence, + ] + + # Calling safe_process_telemetry so if one of the stages fail, we log and move on instead of failing the rest of + # them, as they are independent. + for stage in telemetry_processing_stages: + safe_process_telemetry(stage, telemetry_json) + + +def safe_process_telemetry(processing_function, telemetry_json): + # noinspection PyBroadException + try: + processing_function(telemetry_json) + except Exception as err: + logger.error("Error while in {} stage of processing telemetry.".format(processing_function.func_name), + exc_info=True) def process_ssh_info(telemetry_json): From 53361e3812dca73dc39c16710f9949d9965c96a8 Mon Sep 17 00:00:00 2001 From: Shay Nehmad Date: Sun, 13 Oct 2019 10:48:23 +0300 Subject: [PATCH 3/6] Contain exception if exporter init fails on the island --- .../cc/services/reporting/exporter_init.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/monkey/monkey_island/cc/services/reporting/exporter_init.py b/monkey/monkey_island/cc/services/reporting/exporter_init.py index bd4e82f3e..de478bfe7 100644 --- a/monkey/monkey_island/cc/services/reporting/exporter_init.py +++ b/monkey/monkey_island/cc/services/reporting/exporter_init.py @@ -9,11 +9,18 @@ logger = logging.getLogger(__name__) def populate_exporter_list(): manager = ReportExporterManager() - RemoteRunAwsService.init() - if RemoteRunAwsService.is_running_on_aws() and ('aws' == env.get_deployment()): - manager.add_exporter_to_list(AWSExporter) + try_add_aws_exporter_to_manager(manager) if len(manager.get_exporters_list()) != 0: logger.debug( "Populated exporters list with the following exporters: {0}".format(str(manager.get_exporters_list()))) + +def try_add_aws_exporter_to_manager(manager): + # noinspection PyBroadException + try: + RemoteRunAwsService.init() + if RemoteRunAwsService.is_running_on_aws() and ('aws' == env.get_deployment()): + manager.add_exporter_to_list(AWSExporter) + except Exception as err: + logger.error("Failed adding aws exporter to manager.", exc_info=True) From 74dbb053a6a2f98beb202cabceba1a86f59eefe8 Mon Sep 17 00:00:00 2001 From: Shay Nehmad Date: Sun, 13 Oct 2019 10:48:50 +0300 Subject: [PATCH 4/6] Contain network exceptions if initialized AwsInstance has network issues --- monkey/common/cloud/aws_instance.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/monkey/common/cloud/aws_instance.py b/monkey/common/cloud/aws_instance.py index f113ca894..8cedaa4f3 100644 --- a/monkey/common/cloud/aws_instance.py +++ b/monkey/common/cloud/aws_instance.py @@ -29,14 +29,14 @@ class AwsInstance(object): AWS_LATEST_METADATA_URI_PREFIX + 'meta-data/instance-id', timeout=2).read() self.region = self._parse_region( urllib2.urlopen(AWS_LATEST_METADATA_URI_PREFIX + 'meta-data/placement/availability-zone').read()) - except urllib2.URLError as e: - logger.debug("Failed init of AwsInstance while getting metadata: {}".format(e.message)) + except (urllib2.URLError, IOError) as e: + logger.debug("Failed init of AwsInstance while getting metadata: {}".format(e.message), exc_info=True) try: self.account_id = self._extract_account_id( urllib2.urlopen( AWS_LATEST_METADATA_URI_PREFIX + 'dynamic/instance-identity/document', timeout=2).read()) - except urllib2.URLError as e: + except (urllib2.URLError, IOError) as e: logger.debug("Failed init of AwsInstance while getting dynamic instance data: {}".format(e.message)) @staticmethod From 177e1ea990844724bbf22e93b1c763472f07f8b4 Mon Sep 17 00:00:00 2001 From: Shay Nehmad Date: Sun, 13 Oct 2019 10:55:55 +0300 Subject: [PATCH 5/6] Now suppressing exceptions in cloud info collection as well --- .../infection_monkey/system_info/__init__.py | 54 +++++++++++-------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/monkey/infection_monkey/system_info/__init__.py b/monkey/infection_monkey/system_info/__init__.py index 56d7fca8b..7c0187d12 100644 --- a/monkey/infection_monkey/system_info/__init__.py +++ b/monkey/infection_monkey/system_info/__init__.py @@ -113,7 +113,7 @@ class InfoCollector(object): :return: None. Updates class information """ LOG.debug("Reading subnets") - self.info['network_info'] =\ + self.info['network_info'] = \ { 'networks': get_host_subnets(), 'netstat': NetstatCollector.get_netstat_info() @@ -122,28 +122,38 @@ class InfoCollector(object): def get_azure_info(self): """ Adds credentials possibly stolen from an Azure VM instance (if we're on one) - Updates the credentials structure, creating it if neccesary (compat with mimikatz) + Updates the credentials structure, creating it if necessary (compat with mimikatz) :return: None. Updates class information """ - from infection_monkey.config import WormConfiguration - if not WormConfiguration.extract_azure_creds: - return - LOG.debug("Harvesting creds if on an Azure machine") - azure_collector = AzureCollector() - if 'credentials' not in self.info: - self.info["credentials"] = {} - azure_creds = azure_collector.extract_stored_credentials() - for cred in azure_creds: - username = cred[0] - password = cred[1] - if username not in self.info["credentials"]: - self.info["credentials"][username] = {} - # we might be losing passwords in case of multiple reset attempts on same username - # or in case another collector already filled in a password for this user - self.info["credentials"][username]['password'] = password - if len(azure_creds) != 0: - self.info["Azure"] = {} - self.info["Azure"]['usernames'] = [cred[0] for cred in azure_creds] + # noinspection PyBroadException + try: + from infection_monkey.config import WormConfiguration + if not WormConfiguration.extract_azure_creds: + return + LOG.debug("Harvesting creds if on an Azure machine") + azure_collector = AzureCollector() + if 'credentials' not in self.info: + self.info["credentials"] = {} + azure_creds = azure_collector.extract_stored_credentials() + for cred in azure_creds: + username = cred[0] + password = cred[1] + if username not in self.info["credentials"]: + self.info["credentials"][username] = {} + # we might be losing passwords in case of multiple reset attempts on same username + # or in case another collector already filled in a password for this user + self.info["credentials"][username]['password'] = password + if len(azure_creds) != 0: + self.info["Azure"] = {} + self.info["Azure"]['usernames'] = [cred[0] for cred in azure_creds] + except Exception: + # If we failed to collect azure info, no reason to fail all the collection. Log and continue. + LOG.error("Failed collecting Azure info.", exc_info=True) def get_aws_info(self): - self.info['aws'] = AwsCollector().get_aws_info() + # noinspection PyBroadException + try: + self.info['aws'] = AwsCollector().get_aws_info() + except Exception: + # If we failed to collect aws info, no reason to fail all the collection. Log and continue. + LOG.error("Failed collecting AWS info.", exc_info=True) From 177f902838df8656e389fa866b751354124c69f7 Mon Sep 17 00:00:00 2001 From: Shay Nehmad Date: Sun, 13 Oct 2019 11:10:42 +0300 Subject: [PATCH 6/6] Added error suppression where required and documented where it isn't. --- monkey/monkey_island/cc/environment/aws.py | 1 + monkey/monkey_island/cc/services/remote_run_aws.py | 14 +++++++++++++- .../cc/services/reporting/aws_exporter.py | 2 ++ .../cc/services/reporting/exporter_init.py | 4 ++-- 4 files changed, 18 insertions(+), 3 deletions(-) diff --git a/monkey/monkey_island/cc/environment/aws.py b/monkey/monkey_island/cc/environment/aws.py index 3d3544a32..797a395aa 100644 --- a/monkey/monkey_island/cc/environment/aws.py +++ b/monkey/monkey_island/cc/environment/aws.py @@ -9,6 +9,7 @@ __author__ = 'itay.mizeretz' class AwsEnvironment(Environment): def __init__(self): super(AwsEnvironment, self).__init__() + # Not suppressing error here on purpose. This is critical if we're on AWS env. self.aws_info = AwsInstance() self._instance_id = self._get_instance_id() self.region = self._get_region() diff --git a/monkey/monkey_island/cc/services/remote_run_aws.py b/monkey/monkey_island/cc/services/remote_run_aws.py index 78df00721..77b6d95ea 100644 --- a/monkey/monkey_island/cc/services/remote_run_aws.py +++ b/monkey/monkey_island/cc/services/remote_run_aws.py @@ -1,3 +1,5 @@ +import logging + from monkey_island.cc.services.config import ConfigService from common.cloud.aws_instance import AwsInstance from common.cloud.aws_service import AwsService @@ -7,6 +9,8 @@ from common.cmd.cmd_runner import CmdRunner __author__ = "itay.mizeretz" +logger = logging.getLogger(__name__) + class RemoteRunAwsService: aws_instance = None @@ -23,7 +27,15 @@ class RemoteRunAwsService: :return: None """ if RemoteRunAwsService.aws_instance is None: + RemoteRunAwsService.try_init_aws_instance() + + @staticmethod + def try_init_aws_instance(): + # noinspection PyBroadException + try: RemoteRunAwsService.aws_instance = AwsInstance() + except Exception: + logger.error("Failed init aws instance. Exception info: ", exc_info=True) @staticmethod def run_aws_monkeys(instances, island_ip): @@ -119,7 +131,7 @@ class RemoteRunAwsService: return r"[System.Net.ServicePointManager]::ServerCertificateValidationCallback = {" \ r"$true}; (New-Object System.Net.WebClient).DownloadFile('https://" + island_ip + \ r":5000/api/monkey/download/monkey-windows-" + bit_text + r".exe','.\\monkey.exe'); " \ - r";Start-Process -FilePath '.\\monkey.exe' -ArgumentList 'm0nk3y -s " + island_ip + r":5000'; " + r";Start-Process -FilePath '.\\monkey.exe' -ArgumentList 'm0nk3y -s " + island_ip + r":5000'; " @staticmethod def _get_run_monkey_cmd_line(is_linux, is_64bit, island_ip): diff --git a/monkey/monkey_island/cc/services/reporting/aws_exporter.py b/monkey/monkey_island/cc/services/reporting/aws_exporter.py index 84940df56..b01f349d4 100644 --- a/monkey/monkey_island/cc/services/reporting/aws_exporter.py +++ b/monkey/monkey_island/cc/services/reporting/aws_exporter.py @@ -24,6 +24,7 @@ class AWSExporter(Exporter): logger.info('No issues were found by the monkey, no need to send anything') return True + # Not suppressing error here on purpose. current_aws_region = AwsInstance().get_region() for machine in issues_list: @@ -70,6 +71,7 @@ class AWSExporter(Exporter): configured_product_arn = load_server_configuration_from_file()['aws'].get('sec_hub_product_arn', '') product_arn = 'arn:aws:securityhub:{region}:{arn}'.format(region=region, arn=configured_product_arn) instance_arn = 'arn:aws:ec2:' + str(region) + ':instance:{instance_id}' + # Not suppressing error here on purpose. account_id = AwsInstance().get_account_id() logger.debug("aws account id acquired: {}".format(account_id)) diff --git a/monkey/monkey_island/cc/services/reporting/exporter_init.py b/monkey/monkey_island/cc/services/reporting/exporter_init.py index de478bfe7..f64d4b4aa 100644 --- a/monkey/monkey_island/cc/services/reporting/exporter_init.py +++ b/monkey/monkey_island/cc/services/reporting/exporter_init.py @@ -22,5 +22,5 @@ def try_add_aws_exporter_to_manager(manager): RemoteRunAwsService.init() if RemoteRunAwsService.is_running_on_aws() and ('aws' == env.get_deployment()): manager.add_exporter_to_list(AWSExporter) - except Exception as err: - logger.error("Failed adding aws exporter to manager.", exc_info=True) + except Exception: + logger.error("Failed adding aws exporter to manager. Exception info:", exc_info=True)